linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Scott Branden <scott.branden@broadcom.com>
To: Kees Cook <keescook@chromium.org>, James Morris <jmorris@namei.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Jessica Yu <jeyu@kernel.org>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Matthew Garrett <matthewgarrett@google.com>,
	David Howells <dhowells@redhat.com>,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	"Joel Fernandes (Google)" <joel@joelfernandes.org>,
	KP Singh <kpsingh@google.com>, Dave Olsthoorn <dave@bewaar.me>,
	Hans de Goede <hdegoede@redhat.com>,
	Peter Jones <pjones@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Stephen Boyd <stephen.boyd@linaro.org>,
	Paul Moore <paul@paul-moore.com>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
Date: Tue, 7 Jul 2020 09:42:02 -0700	[thread overview]
Message-ID: <0a5e2c2e-507c-9114-5328-5943f63d707e@broadcom.com> (raw)
In-Reply-To: <20200707081926.3688096-3-keescook@chromium.org>



On 2020-07-07 1:19 a.m., Kees Cook wrote:
> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> that are interested in filtering between types of things. The "how"
> should be an internal detail made uninteresting to the LSMs.
>
> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   drivers/base/firmware_loader/main.c | 5 ++---
>   fs/exec.c                           | 7 ++++---
>   include/linux/fs.h                  | 2 +-
>   security/integrity/ima/ima_main.c   | 6 ++----
>   4 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b13524e..c2f57cedcd6f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>   	int i, len;
>   	int rc = -ENOENT;
>   	char *path;
> -	enum kernel_read_file_id id = READING_FIRMWARE;
>   	size_t msize = INT_MAX;
>   	void *buffer = NULL;
>   
>   	/* Already populated data member means we're loading into a buffer */
>   	if (!decompress && fw_priv->data) {
>   		buffer = fw_priv->data;
> -		id = READING_FIRMWARE_PREALLOC_BUFFER;
>   		msize = fw_priv->allocated_size;
>   	}
>   
> @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>   
>   		/* load firmware files from the mount namespace of init */
>   		rc = kernel_read_file_from_path_initns(path, &buffer,
> -						       &size, msize, id);
> +						       &size, msize,
> +						       READING_FIRMWARE);
>   		if (rc) {
>   			if (rc != -ENOENT)
>   				dev_warn(device, "loading %s failed with error %d\n",
> diff --git a/fs/exec.c b/fs/exec.c
> index e6e8a9a70327..2bf549757ce7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   {
>   	loff_t i_size, pos;
>   	ssize_t bytes = 0;
> +	void *allocated = NULL;
>   	int ret;
>   
>   	if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   		goto out;
>   	}
>   
> -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> -		*buf = vmalloc(i_size);
> +	if (!*buf)
> +		*buf = allocated = vmalloc(i_size);
>   	if (!*buf) {
>   		ret = -ENOMEM;
>   		goto out;
> @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   
>   out_free:
>   	if (ret < 0) {
> -		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> +		if (allocated) {
>   			vfree(*buf);
>   			*buf = NULL;
>   		}
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3f881a892ea7..95fc775ed937 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
>   #endif
>   extern int do_pipe_flags(int *, int);
>   
> +/* This is a list of *what* is being read, not *how*. */
>   #define __kernel_read_file_id(id) \
>   	id(UNKNOWN, unknown)		\
>   	id(FIRMWARE, firmware)		\
With this change, I'm trying to figure out how the partial firmware read 
is going to work on top of this reachitecture.
Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a 
"what"?
> -	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
My patch series gets rejected any time I make a change to the 
kernel_read_file* region in linux/fs.h.
The requirement is for this api to move to another header file outside 
of linux/fs.h
It seems the same should apply to your change.
Could you please add the following patch to the start of you patch 
series to move the kernel_read_file* to its own include file?
https://patchwork.kernel.org/patch/11647063/
>   	id(FIRMWARE_EFI_EMBEDDED, firmware)	\
>   	id(MODULE, kernel-module)		\
>   	id(KEXEC_IMAGE, kexec-image)		\
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..f80ee4ce4669 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
>   int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>   {
>   	/*
> -	 * READING_FIRMWARE_PREALLOC_BUFFER
> -	 *
>   	 * Do devices using pre-allocated memory run the risk of the
>   	 * firmware being accessible to the device prior to the completion
>   	 * of IMA's signature verification any more than when using two
> -	 * buffers?
> +	 * buffers? It may be desirable to include the buffer address
> +	 * in this API and walk all the dma_map_single() mappings to check.
>   	 */
>   	return 0;
>   }
>   
>   const int read_idmap[READING_MAX_ID] = {
>   	[READING_FIRMWARE] = FIRMWARE_CHECK,
> -	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
>   	[READING_MODULE] = MODULE_CHECK,
>   	[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
>   	[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,


  reply	other threads:[~2020-07-07 16:42 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-07  8:19 [PATCH 0/4] Fix misused " Kees Cook
2020-07-07  8:19 ` [PATCH 1/4] firmware_loader: EFI firmware loader must handle pre-allocated buffer Kees Cook
2020-07-07  8:19 ` [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums Kees Cook
2020-07-07 16:42   ` Scott Branden [this message]
2020-07-07 21:55     ` Kees Cook
2020-07-08  3:06       ` Scott Branden
2020-07-08  3:14         ` Kees Cook
2020-07-08  1:37   ` [fs] 676800b78f: BUG:unable_to_handle_page_fault_for_address kernel test robot
2020-07-10 21:00   ` [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums Scott Branden
2020-07-10 22:04     ` Matthew Wilcox
2020-07-10 22:10       ` Scott Branden
2020-07-10 22:44         ` Kees Cook
2020-07-10 22:58           ` Scott Branden
2020-07-16 20:35           ` Scott Branden
2020-07-16 21:16             ` Kees Cook
2020-07-07  8:19 ` [PATCH 3/4] fs: Remove FIRMWARE_EFI_EMBEDDED " Kees Cook
2020-07-07  8:19 ` [PATCH 4/4] module: Add hook for security_kernel_post_read_file() Kees Cook
2020-07-08  0:47   ` Mimi Zohar
2020-07-08  3:10     ` Kees Cook
2020-07-08 13:47       ` Mimi Zohar
2020-07-07  9:31 ` [PATCH 0/4] Fix misused kernel_read_file() enums Greg Kroah-Hartman
2020-07-07 15:36 ` Mimi Zohar
2020-07-07 21:45   ` Kees Cook
2020-07-08 11:01 ` Hans de Goede
2020-07-08 11:37   ` Hans de Goede
2020-07-08 11:55     ` Luis Chamberlain
2020-07-08 11:58       ` Hans de Goede
2020-07-08 13:30         ` Luis Chamberlain
2020-07-09  2:00           ` Kees Cook

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=0a5e2c2e-507c-9114-5328-5943f63d707e@broadcom.com \
    --to=scott.branden@broadcom.com \
    --cc=akpm@linux-foundation.org \
    --cc=casey@schaufler-ca.com \
    --cc=dave@bewaar.me \
    --cc=dhowells@redhat.com \
    --cc=dmitry.kasatkin@gmail.com \
    --cc=ebiederm@xmission.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@lst.de \
    --cc=hdegoede@redhat.com \
    --cc=jeyu@kernel.org \
    --cc=jmorris@namei.org \
    --cc=joel@joelfernandes.org \
    --cc=keescook@chromium.org \
    --cc=kpsingh@google.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=matthewgarrett@google.com \
    --cc=mcgrof@kernel.org \
    --cc=mchehab+huawei@kernel.org \
    --cc=paul@paul-moore.com \
    --cc=peterz@infradead.org \
    --cc=pjones@redhat.com \
    --cc=rafael@kernel.org \
    --cc=rdunlap@infradead.org \
    --cc=serge@hallyn.com \
    --cc=stephen.boyd@linaro.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=zohar@linux.ibm.com \
    --subject='Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums' \
    /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).