LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Casey Schaufler <casey@schaufler-ca.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>,
	 linux-integrity@vger.kernel.org,
	 linux-security-module@vger.kernel.org,
	 linux-kernel@vger.kernel.org,
	 David Howells <dhowells@redhat.com>,
	 "Luis R . Rodriguez" <mcgrof@kernel.org>,
	 kexec@lists.infradead.org,
	 Andres Rodriguez <andresx7@gmail.com>,
	 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	 Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	 Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper
Date: Thu, 17 May 2018 22:37:23 -0500	[thread overview]
Message-ID: <87y3ghhbws.fsf@xmission.com> (raw)
In-Reply-To: <74c096ca-1ad1-799e-df3d-7b1b099333a7@schaufler-ca.com> (Casey Schaufler's message of "Thu, 17 May 2018 17:24:36 -0700")

Casey Schaufler <casey@schaufler-ca.com> writes:

> On 5/17/2018 7:48 AM, Mimi Zohar wrote:
>> In order for LSMs and IMA-appraisal to differentiate between the original
>> and new syscalls (eg. kexec, kernel modules, firmware), both the original
>> and new syscalls must call an LSM hook.
>>
>> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook")
>> introduced calling security_kernel_module_from_file() in both the original
>> and new syscalls.  Commit a1db74209483 ("module: replace
>> copy_module_from_fd with kernel version") replaced these LSM calls with
>> security_kernel_read_file().
>>
>> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents()
>> with kernel version") and commit b804defe4297  ("kexec: replace call to
>> copy_file_from_fd() with kernel version") replaced their own version of
>> reading a file from the kernel with the generic
>> kernel_read_file_from_path/fd() versions, which call the pre and post
>> security_kernel_read_file LSM hooks.
>>
>> Missing are LSM calls in the original kexec syscall and firmware sysfs
>> fallback method.  From a technical perspective there is no justification
>> for defining a new LSM hook, as the existing security_kernel_read_file()
>> works just fine.  The original syscalls, however, do not read a file, so
>> the security hook name is inappropriate.  Instead of defining a new LSM
>> hook, this patch defines security_kernel_read_blob() as a wrapper for
>> the existing LSM security_kernel_file_read() hook.
>
> What a marvelous opportunity to bikeshed!
>
> I really dislike adding another security_ interface just because
> the name isn't quite right. Especially a wrapper, which is just
> code and execution overhead. Why not change security_kernel_read_file()
> to security_kernel_read_blob() everywhere and be done?

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Nack on this sharing nonsense.  These two interfaces do not share any
code in their implementations other than the if statement to distinguish
between the two cases.

Casey you are wrong.  We need something different here.

Mimi a wrapper does not cut it.   The code is not shared.  Despite using
a single function call today.

If we want comprehensible and maintainable code in the security modules
we need to split these two pieces of functionality apart.

Eric

  reply	other threads:[~2018-05-18  3:37 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-17 14:48 [PATCH v2 0/9] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 1/9] ima: based on policy verify firmware signatures (pre-allocated buffer) Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 2/9] ima: fix updating the ima_appraise flag Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper Mimi Zohar
2018-05-18  0:24   ` Casey Schaufler
2018-05-18  3:37     ` Eric W. Biederman [this message]
2018-05-18 11:30       ` Mimi Zohar
2018-05-18 14:58         ` Casey Schaufler
2018-05-18 15:29           ` Mimi Zohar
2018-05-18 17:13       ` James Morris
2018-05-18 17:55         ` Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 4/9] kexec: add call to LSM hook in original kexec_load syscall Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 5/9] ima: based on policy require signed kexec kernel images Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 6/9] firmware: add call to LSM hook before firmware sysfs fallback Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 7/9] ima: based on policy require signed firmware (sysfs fallback) Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 8/9] ima: add build time policy Mimi Zohar
2018-05-17 14:48 ` [PATCH v2 9/9] ima: based on policy prevent loading firmware (pre-allocated buffer) Mimi Zohar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y3ghhbws.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=andresx7@gmail.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=casey@schaufler-ca.com \
    --cc=dhowells@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=keescook@chromium.org \
    --cc=kexec@lists.infradead.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=zohar@linux.vnet.ibm.com \
    --subject='Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).