| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5203c5a1-1ced-f6b1-1086-df65479901a9@broadcom.com>
Date: Fri, 17 Jul 2020 12:09:10 -0700
From: Scott Branden <scott.branden@...adcom.com>
To: Kees Cook <keescook@...omium.org>
Cc: stable@...r.kernel.org, Mimi Zohar <zohar@...ux.ibm.com>,
Matthew Wilcox <willy@...radead.org>,
James Morris <jmorris@...ei.org>,
Luis Chamberlain <mcgrof@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
Jessica Yu <jeyu@...nel.org>,
Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
"Serge E. Hallyn" <serge@...lyn.com>,
Casey Schaufler <casey@...aufler-ca.com>,
"Eric W. Biederman" <ebiederm@...ssion.com>,
Peter Zijlstra <peterz@...radead.org>,
Matthew Garrett <matthewgarrett@...gle.com>,
David Howells <dhowells@...hat.com>,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Randy Dunlap <rdunlap@...radead.org>,
"Joel Fernandes (Google)" <joel@...lfernandes.org>,
KP Singh <kpsingh@...gle.com>, Dave Olsthoorn <dave@...aar.me>,
Hans de Goede <hdegoede@...hat.com>,
Peter Jones <pjones@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Paul Moore <paul@...l-moore.com>,
Stephen Smalley <stephen.smalley.work@...il.com>,
linux-security-module@...r.kernel.org,
linux-integrity@...r.kernel.org, selinux@...r.kernel.org,
linux-fsdevel@...r.kernel.org, kexec@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/13] fs/kernel_read_file: Remove
FIRMWARE_PREALLOC_BUFFER enum
On 2020-07-17 10:42 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)")
> Cc: stable@...r.kernel.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
Acked-by: Scott Branden <scott.branden@...adcom.com>
> ---
> To aid in backporting, this change is made before moving
> kernel_read_file() to separate header/source files.
> ---
> drivers/base/firmware_loader/main.c | 5 ++---
> fs/exec.c | 7 ++++---
> include/linux/fs.h | 2 +-
> kernel/module.c | 2 +-
> security/integrity/digsig.c | 2 +-
> security/integrity/ima/ima_fs.c | 2 +-
> security/integrity/ima/ima_main.c | 6 ++----
> 7 files changed, 12 insertions(+), 14 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) \
> - id(FIRMWARE_PREALLOC_BUFFER, firmware) \
> id(FIRMWARE_EFI_EMBEDDED, firmware) \
> id(MODULE, kernel-module) \
> id(KEXEC_IMAGE, kexec-image) \
> diff --git a/kernel/module.c b/kernel/module.c
> index 0c6573b98c36..26105148f4d2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
> {
> struct load_info info = { };
> loff_t size;
> - void *hdr;
> + void *hdr = NULL;
> int err;
>
> err = may_init_module();
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index e9cbadade74b..ac02b7632353 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const void *data,
>
> int __init integrity_load_x509(const unsigned int id, const char *path)
> {
> - void *data;
> + void *data = NULL;
> loff_t size;
> int rc;
> key_perm_t perm;
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index e3fcad871861..15a44c5022f7 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -272,7 +272,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
>
> static ssize_t ima_read_policy(char *path)
> {
> - void *data;
> + void *data = NULL;
> char *datap;
> loff_t size;
> int rc, pathlen = strlen(path);
> 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,
Powered by blists - more mailing lists