This patch series adds partial read support via a new call request_partial_firmware_into_buf. Such support is needed when the whole file is not needed and/or only a smaller portion of the file will fit into allocated memory at any one time. In order to accept the enhanced API it has been requested that kernel selftests and upstreamed driver utilize the API enhancement and so are included in this patch series.
Also in this patch series is the addition of a new Broadcom VK driver utilizing the new request_firmware_into_buf enhanced API.
Further comment followed to add IMA support of the partial reads originating from request_firmware_into_buf calls. And another request to move existing kernel_read_file* functions to its own include file.
Changes from v9: - add patch to move existing kernel_read_file* to its own include file - driver fixes Changes from v8: - correct compilation error when CONFIG_FW_LOADER not defined Changes from v7: - removed swiss army knife kernel_pread_* style approach and simply add offset parameter in addition to those needed in kernel_read_* functions thus removing need for kernel_pread enum Changes from v6: - update ima_post_read_file check on IMA_FIRMWARE_PARTIAL_READ - adjust new driver i2c-slave-eeprom.c use of request_firmware_into_buf - remove an extern Changes from v5: - add IMA FIRMWARE_PARTIAL_READ support - change kernel pread flags to enum - removed legacy support from driver - driver fixes Changes from v4: - handle reset issues if card crashes - allow driver to have min required msix - add card utilization information Changes from v3: - fix sparse warnings - fix printf format specifiers for size_t - fix 32-bit cross-compiling reports 32-bit shifts - use readl/writel,_relaxed to access pci ioremap memory, removed memory barriers and volatile keyword with such change - driver optimizations for interrupt/poll functionalities Changes from v2: - remove unnecessary code and mutex locks in lib/test_firmware.c - remove VK_IOCTL_ACCESS_BAR support from driver and use pci sysfs instead - remove bitfields - remove Kconfig default m - adjust formatting and some naming based on feedback - fix error handling conditions - use appropriate return codes - use memcpy_toio instead of direct access to PCIE bar
Scott Branden (9): fs: move kernel_read_file* to its own include file fs: introduce kernel_pread_file* support firmware: add request_partial_firmware_into_buf test_firmware: add partial read support for request_firmware_into_buf firmware: test partial file reads of request_partial_firmware_into_buf bcm-vk: add bcm_vk UAPI misc: bcm-vk: add Broadcom VK driver MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver ima: add FIRMWARE_PARTIAL_READ support
MAINTAINERS | 7 + drivers/base/firmware_loader/firmware.h | 5 + drivers/base/firmware_loader/main.c | 80 +- drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/bcm-vk/Kconfig | 29 + drivers/misc/bcm-vk/Makefile | 11 + drivers/misc/bcm-vk/bcm_vk.h | 419 +++++ drivers/misc/bcm-vk/bcm_vk_dev.c | 1357 +++++++++++++++ drivers/misc/bcm-vk/bcm_vk_msg.c | 1504 +++++++++++++++++ drivers/misc/bcm-vk/bcm_vk_msg.h | 211 +++ drivers/misc/bcm-vk/bcm_vk_sg.c | 275 +++ drivers/misc/bcm-vk/bcm_vk_sg.h | 61 + drivers/misc/bcm-vk/bcm_vk_tty.c | 352 ++++ fs/exec.c | 92 +- include/linux/firmware.h | 12 + include/linux/fs.h | 39 - include/linux/ima.h | 1 + include/linux/kernel_read_file.h | 69 + include/linux/security.h | 1 + include/uapi/linux/misc/bcm_vk.h | 99 ++ kernel/kexec_file.c | 1 + kernel/module.c | 1 + lib/test_firmware.c | 154 +- security/integrity/digsig.c | 1 + security/integrity/ima/ima_fs.c | 1 + security/integrity/ima/ima_main.c | 25 +- security/integrity/ima/ima_policy.c | 1 + security/loadpin/loadpin.c | 1 + security/security.c | 1 + security/selinux/hooks.c | 1 + .../selftests/firmware/fw_filesystem.sh | 80 + 32 files changed, 4802 insertions(+), 91 deletions(-) create mode 100644 drivers/misc/bcm-vk/Kconfig create mode 100644 drivers/misc/bcm-vk/Makefile create mode 100644 drivers/misc/bcm-vk/bcm_vk.h create mode 100644 drivers/misc/bcm-vk/bcm_vk_dev.c create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.c create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.h create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.c create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.h create mode 100644 drivers/misc/bcm-vk/bcm_vk_tty.c create mode 100644 include/linux/kernel_read_file.h create mode 100644 include/uapi/linux/misc/bcm_vk.h
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h include file. That header gets pulled in just about everywhere and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig hch@lst.de Signed-off-by: Scott Branden scott.branden@broadcom.com Reviewed-by: Christoph Hellwig hch@lst.de Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/base/firmware_loader/main.c | 1 + fs/exec.c | 1 + include/linux/fs.h | 39 ---------------------- include/linux/ima.h | 1 + include/linux/kernel_read_file.h | 52 +++++++++++++++++++++++++++++ include/linux/security.h | 1 + kernel/kexec_file.c | 1 + kernel/module.c | 1 + security/integrity/digsig.c | 1 + security/integrity/ima/ima_fs.c | 1 + security/integrity/ima/ima_main.c | 1 + security/integrity/ima/ima_policy.c | 1 + security/loadpin/loadpin.c | 1 + security/security.c | 1 + security/selinux/hooks.c | 1 + 15 files changed, 65 insertions(+), 39 deletions(-) create mode 100644 include/linux/kernel_read_file.h
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 9da0c9d5f538..7ca229760dfc 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -12,6 +12,7 @@
#include <linux/capability.h> #include <linux/device.h> +#include <linux/kernel_read_file.h> #include <linux/module.h> #include <linux/init.h> #include <linux/timer.h> diff --git a/fs/exec.c b/fs/exec.c index 7b7cbb180785..4ea87db5e4d5 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -23,6 +23,7 @@ * formats. */
+#include <linux/kernel_read_file.h> #include <linux/slab.h> #include <linux/file.h> #include <linux/fdtable.h> diff --git a/include/linux/fs.h b/include/linux/fs.h index f15848899945..7ea4709a1298 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2857,45 +2857,6 @@ static inline void i_readcount_inc(struct inode *inode) #endif extern int do_pipe_flags(int *, int);
-#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) \ - id(KEXEC_INITRAMFS, kexec-initramfs) \ - id(POLICY, security-policy) \ - id(X509_CERTIFICATE, x509-certificate) \ - id(MAX_ID, ) - -#define __fid_enumify(ENUM, dummy) READING_ ## ENUM, -#define __fid_stringify(dummy, str) #str, - -enum kernel_read_file_id { - __kernel_read_file_id(__fid_enumify) -}; - -static const char * const kernel_read_file_str[] = { - __kernel_read_file_id(__fid_stringify) -}; - -static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) -{ - if ((unsigned)id >= READING_MAX_ID) - return kernel_read_file_str[READING_UNKNOWN]; - - return kernel_read_file_str[id]; -} - -extern int kernel_read_file(struct file *, void **, loff_t *, loff_t, - enum kernel_read_file_id); -extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t, - enum kernel_read_file_id); -extern int kernel_read_file_from_path_initns(const char *, void **, loff_t *, loff_t, - enum kernel_read_file_id); -extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t, - enum kernel_read_file_id); extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *); extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *); extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *); diff --git a/include/linux/ima.h b/include/linux/ima.h index 9164e1534ec9..148636bfcc8f 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -7,6 +7,7 @@ #ifndef _LINUX_IMA_H #define _LINUX_IMA_H
+#include <linux/kernel_read_file.h> #include <linux/fs.h> #include <linux/security.h> #include <linux/kexec.h> diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h new file mode 100644 index 000000000000..53f5ca41519a --- /dev/null +++ b/include/linux/kernel_read_file.h @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_KERNEL_READ_FILE_H +#define _LINUX_KERNEL_READ_FILE_H + +#include <linux/file.h> +#include <linux/types.h> + +#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) \ + id(KEXEC_INITRAMFS, kexec-initramfs) \ + id(POLICY, security-policy) \ + id(X509_CERTIFICATE, x509-certificate) \ + id(MAX_ID, ) + +#define __fid_enumify(ENUM, dummy) READING_ ## ENUM, +#define __fid_stringify(dummy, str) #str, + +enum kernel_read_file_id { + __kernel_read_file_id(__fid_enumify) +}; + +static const char * const kernel_read_file_str[] = { + __kernel_read_file_id(__fid_stringify) +}; + +static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) +{ + if ((unsigned int)id >= READING_MAX_ID) + return kernel_read_file_str[READING_UNKNOWN]; + + return kernel_read_file_str[id]; +} + +int kernel_read_file(struct file *file, + void **buf, loff_t *size, loff_t max_size, + enum kernel_read_file_id id); +int kernel_read_file_from_path(const char *path, + void **buf, loff_t *size, loff_t max_size, + enum kernel_read_file_id id); +int kernel_read_file_from_path_initns(const char *path, + void **buf, loff_t *size, loff_t max_size, + enum kernel_read_file_id id); +int kernel_read_file_from_fd(int fd, + void **buf, loff_t *size, loff_t max_size, + enum kernel_read_file_id id); + +#endif /* _LINUX_KERNEL_READ_FILE_H */ diff --git a/include/linux/security.h b/include/linux/security.h index 2797e7f6418e..fc1c6af331bd 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -23,6 +23,7 @@ #ifndef __LINUX_SECURITY_H #define __LINUX_SECURITY_H
+#include <linux/kernel_read_file.h> #include <linux/key.h> #include <linux/capability.h> #include <linux/fs.h> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index 09cc78df53c6..1358069ce9e9 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -24,6 +24,7 @@ #include <linux/elf.h> #include <linux/elfcore.h> #include <linux/kernel.h> +#include <linux/kernel_read_file.h> #include <linux/syscalls.h> #include <linux/vmalloc.h> #include "kexec_internal.h" diff --git a/kernel/module.c b/kernel/module.c index aa183c9ac0a2..97da0e97c0a0 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -18,6 +18,7 @@ #include <linux/fs.h> #include <linux/sysfs.h> #include <linux/kernel.h> +#include <linux/kernel_read_file.h> #include <linux/slab.h> #include <linux/vmalloc.h> #include <linux/elf.h> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index e9cbadade74b..d09602aab7bd 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -10,6 +10,7 @@ #include <linux/sched.h> #include <linux/slab.h> #include <linux/cred.h> +#include <linux/kernel_read_file.h> #include <linux/key-type.h> #include <linux/digsig.h> #include <linux/vmalloc.h> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index e3fcad871861..57ecbf285fc7 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -13,6 +13,7 @@ */
#include <linux/fcntl.h> +#include <linux/kernel_read_file.h> #include <linux/slab.h> #include <linux/init.h> #include <linux/seq_file.h> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index c1583d98c5e5..15f29fed6d9f 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/file.h> #include <linux/binfmts.h> +#include <linux/kernel_read_file.h> #include <linux/mount.h> #include <linux/mman.h> #include <linux/slab.h> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index e493063a3c34..f8390f6081f0 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -9,6 +9,7 @@
#include <linux/init.h> #include <linux/list.h> +#include <linux/kernel_read_file.h> #include <linux/fs.h> #include <linux/security.h> #include <linux/magic.h> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index 670a1aebb8a1..163c48216d13 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -11,6 +11,7 @@
#include <linux/module.h> #include <linux/fs.h> +#include <linux/kernel_read_file.h> #include <linux/lsm_hooks.h> #include <linux/mount.h> #include <linux/blkdev.h> diff --git a/security/security.c b/security/security.c index 3ec3216c7d1f..7ff16c56df91 100644 --- a/security/security.c +++ b/security/security.c @@ -16,6 +16,7 @@ #include <linux/export.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/kernel_read_file.h> #include <linux/lsm_hooks.h> #include <linux/integrity.h> #include <linux/ima.h> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index ca901025802a..2f1809ae0e3e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -24,6 +24,7 @@ #include <linux/init.h> #include <linux/kd.h> #include <linux/kernel.h> +#include <linux/kernel_read_file.h> #include <linux/tracehook.h> #include <linux/errno.h> #include <linux/sched/signal.h>
On Mon, Jul 06, 2020 at 04:23:01PM -0700, Scott Branden wrote:
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h include file. That header gets pulled in just about everywhere and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig hch@lst.de Signed-off-by: Scott Branden scott.branden@broadcom.com Reviewed-by: Christoph Hellwig hch@lst.de Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/base/firmware_loader/main.c | 1 + fs/exec.c | 1 + include/linux/fs.h | 39 ---------------------- include/linux/ima.h | 1 + include/linux/kernel_read_file.h | 52 +++++++++++++++++++++++++++++ include/linux/security.h | 1 + kernel/kexec_file.c | 1 + kernel/module.c | 1 + security/integrity/digsig.c | 1 + security/integrity/ima/ima_fs.c | 1 + security/integrity/ima/ima_main.c | 1 + security/integrity/ima/ima_policy.c | 1 + security/loadpin/loadpin.c | 1 + security/security.c | 1 + security/selinux/hooks.c | 1 + 15 files changed, 65 insertions(+), 39 deletions(-) create mode 100644 include/linux/kernel_read_file.h
This looks like too many files are getting touched. If it got added to security.h, very few of the above .c files will need it explicitly added (maybe none). You can test future versions of this change with an allmodconfig build and make sure you have a matching .o for each .c file that calls kernel_read_file(). :)
But otherwise, sure, seems good.
On 2020-07-07 4:40 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:01PM -0700, Scott Branden wrote:
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h include file. That header gets pulled in just about everywhere and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig hch@lst.de Signed-off-by: Scott Branden scott.branden@broadcom.com Reviewed-by: Christoph Hellwig hch@lst.de Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/base/firmware_loader/main.c | 1 + fs/exec.c | 1 + include/linux/fs.h | 39 ---------------------- include/linux/ima.h | 1 + include/linux/kernel_read_file.h | 52 +++++++++++++++++++++++++++++ include/linux/security.h | 1 + kernel/kexec_file.c | 1 + kernel/module.c | 1 + security/integrity/digsig.c | 1 + security/integrity/ima/ima_fs.c | 1 + security/integrity/ima/ima_main.c | 1 + security/integrity/ima/ima_policy.c | 1 + security/loadpin/loadpin.c | 1 + security/security.c | 1 + security/selinux/hooks.c | 1 + 15 files changed, 65 insertions(+), 39 deletions(-) create mode 100644 include/linux/kernel_read_file.h
This looks like too many files are getting touched. If it got added to security.h, very few of the above .c files will need it explicitly added (maybe none).
Some people want the header file added to each file that uses it, others want it in a common header file. I tried to add it to each file that uses it. But if the other approach is to be followed that could be done.
You can test future versions of this change with an allmodconfig build and make sure you have a matching .o for each .c file that calls kernel_read_file(). :)
But otherwise, sure, seems good.
Add kernel_pread_file* support to kernel to allow for partial read of files with an offset into the file.
Signed-off-by: Scott Branden scott.branden@broadcom.com --- fs/exec.c | 93 ++++++++++++++++++++++++-------- include/linux/kernel_read_file.h | 17 ++++++ 2 files changed, 87 insertions(+), 23 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c index 4ea87db5e4d5..e6a8a65f7478 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) } EXPORT_SYMBOL(open_exec);
-int kernel_read_file(struct file *file, void **buf, loff_t *size, - loff_t max_size, enum kernel_read_file_id id) -{ - loff_t i_size, pos; +int kernel_pread_file(struct file *file, void **buf, loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) +{ + loff_t alloc_size; + loff_t buf_pos; + loff_t read_end; + loff_t i_size; ssize_t bytes = 0; int ret;
@@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, ret = -EINVAL; goto out; } - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { + + /* Default read to end of file */ + read_end = i_size; + + /* Allow reading partial portion of file */ + if ((id == READING_FIRMWARE_PARTIAL_READ) && + (i_size > (pos + max_size))) + read_end = pos + max_size; + + alloc_size = read_end - pos; + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { ret = -EFBIG; goto out; }
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) - *buf = vmalloc(i_size); + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) + *buf = vmalloc(alloc_size); if (!*buf) { ret = -ENOMEM; goto out; }
- pos = 0; - while (pos < i_size) { - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos); + buf_pos = 0; + while (pos < read_end) { + bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos); if (bytes < 0) { ret = bytes; goto out_free; @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
if (bytes == 0) break; + + buf_pos += bytes; }
- if (pos != i_size) { + if (pos != read_end) { ret = -EIO; goto out_free; }
- ret = security_kernel_post_read_file(file, *buf, i_size, id); + ret = security_kernel_post_read_file(file, *buf, alloc_size, id); if (!ret) *size = pos;
out_free: if (ret < 0) { - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) { vfree(*buf); *buf = NULL; } @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, allow_write_access(file); return ret; } + +int kernel_read_file(struct file *file, void **buf, loff_t *size, + loff_t max_size, enum kernel_read_file_id id) +{ + return kernel_pread_file(file, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, - loff_t max_size, enum kernel_read_file_id id) +int kernel_pread_file_from_path(const char *path, void **buf, + loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct file *file; int ret; @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, if (IS_ERR(file)) return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id); + ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; } + +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, + loff_t max_size, enum kernel_read_file_id id) +{ + return kernel_pread_file_from_path(path, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
-int kernel_read_file_from_path_initns(const char *path, void **buf, - loff_t *size, loff_t max_size, - enum kernel_read_file_id id) +int kernel_pread_file_from_path_initns(const char *path, void **buf, + loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct file *file; struct path root; @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, if (IS_ERR(file)) return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id); + ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; } + +int kernel_read_file_from_path_initns(const char *path, void **buf, + loff_t *size, loff_t max_size, + enum kernel_read_file_id id) +{ + return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, - enum kernel_read_file_id id) +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct fd f = fdget(fd); int ret = -EBADF; @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, if (!f.file) goto out;
- ret = kernel_read_file(f.file, buf, size, max_size, id); + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id); out: fdput(f); return ret; } + +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, + enum kernel_read_file_id id) +{ + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h index 53f5ca41519a..f061ccb8d0b4 100644 --- a/include/linux/kernel_read_file.h +++ b/include/linux/kernel_read_file.h @@ -8,6 +8,7 @@ #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown) \ id(FIRMWARE, firmware) \ + id(FIRMWARE_PARTIAL_READ, firmware) \ id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(FIRMWARE_EFI_EMBEDDED, firmware) \ id(MODULE, kernel-module) \ @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) return kernel_read_file_str[id]; }
+int kernel_pread_file(struct file *file, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path(const char *path, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path_initns(const char *path, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_path_initns(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_fd(int fd, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id);
On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
Add kernel_pread_file* support to kernel to allow for partial read of files with an offset into the file.
Signed-off-by: Scott Branden scott.branden@broadcom.com
fs/exec.c | 93 ++++++++++++++++++++++++-------- include/linux/kernel_read_file.h | 17 ++++++ 2 files changed, 87 insertions(+), 23 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c index 4ea87db5e4d5..e6a8a65f7478 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) } EXPORT_SYMBOL(open_exec); -int kernel_read_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)-{
- loff_t i_size, pos;
+int kernel_pread_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, loff_t pos,enum kernel_read_file_id id)+{
- loff_t alloc_size;
- loff_t buf_pos;
- loff_t read_end;
- loff_t i_size; ssize_t bytes = 0; int ret;
@@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, ret = -EINVAL; goto out; }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- /* Default read to end of file */
- read_end = i_size;
- /* Allow reading partial portion of file */
- if ((id == READING_FIRMWARE_PARTIAL_READ) &&
(i_size > (pos + max_size)))read_end = pos + max_size;
There's no need to involve "id" here. There are other signals about what's happening (i.e. pos != 0, max_size != i_size, etc).
- alloc_size = read_end - pos;
- if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { ret = -EFBIG; goto out; }
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
*buf = vmalloc(i_size);
- if ((id != READING_FIRMWARE_PARTIAL_READ) &&
(id != READING_FIRMWARE_PREALLOC_BUFFER))*buf = vmalloc(alloc_size);
The id usage here was a mistake in upstream, and the series I sent is trying to clean that up.
Greg, it seems this series is going to end up in your tree due to it being drivers/misc? I guess I need to direct my series to Greg then, but get LSM folks Acks.
- pos = 0;
- while (pos < i_size) {
bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
- buf_pos = 0;
- while (pos < read_end) {
bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos);@@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, if (bytes == 0) break;
buf_pos += bytes;
- if (pos != i_size) {
- if (pos != read_end) { ret = -EIO; goto out_free; }
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- ret = security_kernel_post_read_file(file, *buf, alloc_size, id); if (!ret) *size = pos;
This call cannot be inside kernel_pread_file(): any future LSMs will see a moving window of contents, etc. It'll need to be in kernel_read_file() proper.
out_free: if (ret < 0) {
if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
if ((id != READING_FIRMWARE_PARTIAL_READ) &&(id != READING_FIRMWARE_PREALLOC_BUFFER)) { vfree(*buf); *buf = NULL;@@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, allow_write_access(file); return ret; }
+int kernel_read_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)+{
- return kernel_pread_file(file, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file); -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)+int kernel_pread_file_from_path(const char *path, void **buf,
loff_t *size,loff_t max_size, loff_t pos,enum kernel_read_file_id id){ struct file *file; int ret; @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, if (IS_ERR(file)) return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
- ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret;
}
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)+{
- return kernel_pread_file_from_path(path, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file_from_path); -int kernel_read_file_from_path_initns(const char *path, void **buf,
loff_t *size, loff_t max_size,enum kernel_read_file_id id)+int kernel_pread_file_from_path_initns(const char *path, void **buf,
loff_t *size,loff_t max_size, loff_t pos,enum kernel_read_file_id id){ struct file *file; struct path root; @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, if (IS_ERR(file)) return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
- ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret;
}
+int kernel_read_file_from_path_initns(const char *path, void **buf,
loff_t *size, loff_t max_size,enum kernel_read_file_id id)+{
- return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns); -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
enum kernel_read_file_id id)+int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
loff_t max_size, loff_t pos,enum kernel_read_file_id id){ struct fd f = fdget(fd); int ret = -EBADF; @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, if (!f.file) goto out;
- ret = kernel_read_file(f.file, buf, size, max_size, id);
- ret = kernel_pread_file(f.file, buf, size, max_size, pos, id);
out: fdput(f); return ret; }
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
enum kernel_read_file_id id)+{
- return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
For each of these execution path, the mapping to LSM hooks is:
- all path must call security_kernel_read_file(file, id) before reading (this appears to be fine as-is in your series).
- anything doing a "full" read needs to call security_kernel_post_read_file() with the file and full buffer, size, etc (so all the kernel_read_file*() paths). I imagine instead of adding 3 copy/pasted versions of this, it may be possible to refactor the helpers into a single core "full" caller that takes struct file, or doing some logic in kernel_pread_file() that notices it has the entire file in the buffer and doing the call then. As an example of what I mean about doing the call, here's how I might imagine it for one of the paths if it took struct file:
int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id) { int ret;
ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id); if (ret) return ret; return security_kernel_post_read_file(file, buf, *size, id); }
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h index 53f5ca41519a..f061ccb8d0b4 100644 --- a/include/linux/kernel_read_file.h +++ b/include/linux/kernel_read_file.h @@ -8,6 +8,7 @@ #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown) \ id(FIRMWARE, firmware) \
- id(FIRMWARE_PARTIAL_READ, firmware) \ id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(FIRMWARE_EFI_EMBEDDED, firmware) \
And again, sorry that this was in here as a misleading example.
id(MODULE, kernel-module) \ @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) return kernel_read_file_str[id]; } +int kernel_pread_file(struct file *file,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);int kernel_read_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path(const char *path,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path_initns(const char *path,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);int kernel_read_file_from_path_initns(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_fd(int fd,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id);
I remain concerned that adding these helpers will lead a poor interaction with LSMs, but I guess I get to hold my tongue. :)
On Tue, 2020-07-07 at 16:56 -0700, Kees Cook wrote:
@@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, ret = -EINVAL; goto out; } - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ /* Default read to end of file */ + read_end = i_size;
+ /* Allow reading partial portion of file */ + if ((id == READING_FIRMWARE_PARTIAL_READ) && + (i_size > (pos + max_size))) + read_end = pos + max_size;
There's no need to involve "id" here. There are other signals about what's happening (i.e. pos != 0, max_size != i_size, etc).
Both the pre and post security kernel_read_file hooks are called here, but there isn't enough information being passed to the LSM/IMA to be able to different which hook is applicable. One method of providing that additional information is by enumeration. The other option would be to pass some additional information.
For example, on the post kernel_read_file hook, the file is read once into memory. IMA calculates the firmware file hash based on the buffer contents. On the pre kernel_read_file hook, IMA would need to read the entire file, calculating the file hash. Both methods of calculating the file hash work, but the post hook is more efficient.
Mimi
On 2020-07-07 4:56 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
Add kernel_pread_file* support to kernel to allow for partial read of files with an offset into the file.
Signed-off-by: Scott Branden scott.branden@broadcom.com
fs/exec.c | 93 ++++++++++++++++++++++++-------- include/linux/kernel_read_file.h | 17 ++++++ 2 files changed, 87 insertions(+), 23 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c index 4ea87db5e4d5..e6a8a65f7478 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) } EXPORT_SYMBOL(open_exec); -int kernel_read_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)-{
- loff_t i_size, pos;
+int kernel_pread_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, loff_t pos,enum kernel_read_file_id id)+{
- loff_t alloc_size;
- loff_t buf_pos;
- loff_t read_end;
- loff_t i_size; ssize_t bytes = 0; int ret;
@@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, ret = -EINVAL; goto out; }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- /* Default read to end of file */
- read_end = i_size;
- /* Allow reading partial portion of file */
- if ((id == READING_FIRMWARE_PARTIAL_READ) &&
(i_size > (pos + max_size)))read_end = pos + max_size;There's no need to involve "id" here. There are other signals about what's happening (i.e. pos != 0, max_size != i_size, etc).
There are other signals other than the fact that kernel_read_file requires the entire file to be read while kernel_pread_file allows partial files to be read. So if you do a pread at pos = 0 you need another key to indicate it is "ok" if max_size < i_size. If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to share 99% of the code between kernel_read_file and kernel_pread_file then I need to add another parameter to a common function called between these functions. And adding another parameter was rejected previously in the review as a "swiss army knife approach" by another reviewer. I am happy to add it back in because it is necessary to share code and differentiate whether we are performing a partial read or not.
- alloc_size = read_end - pos;
- if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { ret = -EFBIG; goto out; }
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
*buf = vmalloc(i_size);
- if ((id != READING_FIRMWARE_PARTIAL_READ) &&
(id != READING_FIRMWARE_PREALLOC_BUFFER))*buf = vmalloc(alloc_size);The id usage here was a mistake in upstream, and the series I sent is trying to clean that up.
I see that cleanup and it works fine with the pread. Other than I need some sort of key to share code and indicate whether it is "ok" to do a partial read of the file or not.
Greg, it seems this series is going to end up in your tree due to it being drivers/misc? I guess I need to direct my series to Greg then, but get LSM folks Acks.
- pos = 0;
- while (pos < i_size) {
bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
- buf_pos = 0;
- while (pos < read_end) {
bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos);@@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, if (bytes == 0) break;
buf_pos += bytes;
- if (pos != i_size) {
- if (pos != read_end) { ret = -EIO; goto out_free; }
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- ret = security_kernel_post_read_file(file, *buf, alloc_size, id); if (!ret) *size = pos;
This call cannot be inside kernel_pread_file(): any future LSMs will see a moving window of contents, etc. It'll need to be in kernel_read_file() proper.
If IMA still passes (after testing my next patch series with your changes and my modifications) I will need some more help here.
out_free: if (ret < 0) {
if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
if ((id != READING_FIRMWARE_PARTIAL_READ) &&(id != READING_FIRMWARE_PREALLOC_BUFFER)) { vfree(*buf); *buf = NULL;@@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, allow_write_access(file); return ret; }
+int kernel_read_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)+{
- return kernel_pread_file(file, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file); -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)+int kernel_pread_file_from_path(const char *path, void **buf,
loff_t *size,loff_t max_size, loff_t pos,enum kernel_read_file_id id)@@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, if (IS_ERR(file)) return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
- ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; }
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)+{
- return kernel_pread_file_from_path(path, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file_from_path); -int kernel_read_file_from_path_initns(const char *path, void **buf,
loff_t *size, loff_t max_size,enum kernel_read_file_id id)+int kernel_pread_file_from_path_initns(const char *path, void **buf,
loff_t *size,loff_t max_size, loff_t pos,enum kernel_read_file_id id)@@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, if (IS_ERR(file)) return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
- ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; }
+int kernel_read_file_from_path_initns(const char *path, void **buf,
loff_t *size, loff_t max_size,enum kernel_read_file_id id)+{
- return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns); -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
enum kernel_read_file_id id)+int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
loff_t max_size, loff_t pos,enum kernel_read_file_id id)@@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, if (!f.file) goto out;
- ret = kernel_read_file(f.file, buf, size, max_size, id);
- ret = kernel_pread_file(f.file, buf, size, max_size, pos, id); out: fdput(f); return ret; }
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
enum kernel_read_file_id id)+{
- return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id);
+} EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
For each of these execution path, the mapping to LSM hooks is:
all path must call security_kernel_read_file(file, id) before reading (this appears to be fine as-is in your series).
anything doing a "full" read needs to call security_kernel_post_read_file() with the file and full buffer, size, etc (so all the kernel_read_file*() paths). I imagine instead of adding 3 copy/pasted versions of this, it may be possible to refactor the helpers into a single core "full" caller that takes struct file, or doing some logic in kernel_pread_file() that notices it has the entire file in the buffer and doing the call then. As an example of what I mean about doing the call, here's how I might imagine it for one of the paths if it took struct file:
int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id) { int ret;
ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id); if (ret) return ret; return security_kernel_post_read_file(file, buf, *size, id); }
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h index 53f5ca41519a..f061ccb8d0b4 100644 --- a/include/linux/kernel_read_file.h +++ b/include/linux/kernel_read_file.h @@ -8,6 +8,7 @@ #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown) \ id(FIRMWARE, firmware) \
- id(FIRMWARE_PARTIAL_READ, firmware) \ id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(FIRMWARE_EFI_EMBEDDED, firmware) \
And again, sorry that this was in here as a misleading example.
id(MODULE, kernel-module) \ @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) return kernel_read_file_str[id]; } +int kernel_pread_file(struct file *file,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);+int kernel_pread_file_from_path(const char *path,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);+int kernel_pread_file_from_path_initns(const char *path,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);+int kernel_pread_file_from_fd(int fd,
void **buf, loff_t *size, loff_t pos,loff_t max_size,enum kernel_read_file_id id);I remain concerned that adding these helpers will lead a poor interaction with LSMs, but I guess I get to hold my tongue. :)
We could add pread functions that are "unsafe" in nature instead then? As I certainly do not need any integrity checks on the file for my driver. The real check is done by the card the data is loaded to whether is passes the linux security checks or not. And then, if someone does want to do something "safe" with preads another kernel_read_file_securelock/unlock could be added for those that need security for their partial reads?
Hi Kees,
one more comment below.
On 2020-07-07 9:01 p.m., Scott Branden wrote:
On 2020-07-07 4:56 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
Add kernel_pread_file* support to kernel to allow for partial read of files with an offset into the file.
Signed-off-by: Scott Branden scott.branden@broadcom.com
fs/exec.c | 93 ++++++++++++++++++++++++-------- include/linux/kernel_read_file.h | 17 ++++++ 2 files changed, 87 insertions(+), 23 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c index 4ea87db5e4d5..e6a8a65f7478 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) } EXPORT_SYMBOL(open_exec); -int kernel_read_file(struct file *file, void **buf, loff_t *size, - loff_t max_size, enum kernel_read_file_id id) -{ - loff_t i_size, pos; +int kernel_pread_file(struct file *file, void **buf, loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) +{ + loff_t alloc_size; + loff_t buf_pos; + loff_t read_end; + loff_t i_size; ssize_t bytes = 0; int ret; @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, ret = -EINVAL; goto out; } - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ /* Default read to end of file */ + read_end = i_size;
+ /* Allow reading partial portion of file */ + if ((id == READING_FIRMWARE_PARTIAL_READ) && + (i_size > (pos + max_size))) + read_end = pos + max_size;
There's no need to involve "id" here. There are other signals about what's happening (i.e. pos != 0, max_size != i_size, etc).
There are other signals other than the fact that kernel_read_file requires the entire file to be read while kernel_pread_file allows partial files to be read. So if you do a pread at pos = 0 you need another key to indicate it is "ok" if max_size < i_size. If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to share 99% of the code between kernel_read_file and kernel_pread_file then I need to add another parameter to a common function called between these functions. And adding another parameter was rejected previously in the review as a "swiss army knife approach" by another reviewer. I am happy to add it back in because it is necessary to share code and differentiate whether we are performing a partial read or not.
+ alloc_size = read_end - pos; + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { ret = -EFBIG; goto out; } - if (id != READING_FIRMWARE_PREALLOC_BUFFER) - *buf = vmalloc(i_size); + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) + *buf = vmalloc(alloc_size); if (!*buf) { ret = -ENOMEM; goto out; }
The id usage here was a mistake in upstream, and the series I sent is trying to clean that up.
I see that cleanup and it works fine with the pread. Other than I need some sort of key to share code and indicate whether it is "ok" to do a partial read of the file or not.
Greg, it seems this series is going to end up in your tree due to it being drivers/misc? I guess I need to direct my series to Greg then, but get LSM folks Acks.
- pos = 0; - while (pos < i_size) { - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos); + buf_pos = 0; + while (pos < read_end) { + bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos); if (bytes < 0) { ret = bytes; goto out_free; @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, if (bytes == 0) break;
+ buf_pos += bytes; } - if (pos != i_size) { + if (pos != read_end) { ret = -EIO; goto out_free; } - ret = security_kernel_post_read_file(file, *buf, i_size, id); + ret = security_kernel_post_read_file(file, *buf, alloc_size, id); if (!ret) *size = pos;
This call cannot be inside kernel_pread_file(): any future LSMs will see a moving window of contents, etc. It'll need to be in kernel_read_file() proper.
If IMA still passes (after testing my next patch series with your changes and my modifications) I will need some more help here.
out_free: if (ret < 0) { - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) { vfree(*buf); *buf = NULL; } @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, allow_write_access(file); return ret; }
+int kernel_read_file(struct file *file, void **buf, loff_t *size, + loff_t max_size, enum kernel_read_file_id id) +{ + return kernel_pread_file(file, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file); -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, - loff_t max_size, enum kernel_read_file_id id) +int kernel_pread_file_from_path(const char *path, void **buf, + loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct file *file; int ret; @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, if (IS_ERR(file)) return PTR_ERR(file); - ret = kernel_read_file(file, buf, size, max_size, id); + ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; }
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, + loff_t max_size, enum kernel_read_file_id id) +{ + return kernel_pread_file_from_path(path, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_path); -int kernel_read_file_from_path_initns(const char *path, void **buf, - loff_t *size, loff_t max_size, - enum kernel_read_file_id id) +int kernel_pread_file_from_path_initns(const char *path, void **buf, + loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct file *file; struct path root; @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, if (IS_ERR(file)) return PTR_ERR(file); - ret = kernel_read_file(file, buf, size, max_size, id); + ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; }
+int kernel_read_file_from_path_initns(const char *path, void **buf, + loff_t *size, loff_t max_size, + enum kernel_read_file_id id) +{ + return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns); -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, - enum kernel_read_file_id id) +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct fd f = fdget(fd); int ret = -EBADF; @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, if (!f.file) goto out; - ret = kernel_read_file(f.file, buf, size, max_size, id); + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id); out: fdput(f); return ret; }
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, + enum kernel_read_file_id id) +{ + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
For each of these execution path, the mapping to LSM hooks is:
- all path must call security_kernel_read_file(file, id) before reading
(this appears to be fine as-is in your series).
- anything doing a "full" read needs to call
security_kernel_post_read_file() with the file and full buffer, size, etc (so all the kernel_read_file*() paths). I imagine instead of adding 3 copy/pasted versions of this, it may be possible to refactor the helpers into a single core "full" caller that takes struct file, or doing some logic in kernel_pread_file() that notices it has the entire file in the buffer and doing the call then. As an example of what I mean about doing the call, here's how I might imagine it for one of the paths if it took struct file:
int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id) { int ret;
ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id); if (ret) return ret; return security_kernel_post_read_file(file, buf, *size, id); }
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h index 53f5ca41519a..f061ccb8d0b4 100644 --- a/include/linux/kernel_read_file.h +++ b/include/linux/kernel_read_file.h @@ -8,6 +8,7 @@ #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown) \ id(FIRMWARE, firmware) \ + id(FIRMWARE_PARTIAL_READ, firmware) \ id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(FIRMWARE_EFI_EMBEDDED, firmware) \
And again, sorry that this was in here as a misleading example.
id(MODULE, kernel-module) \ @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) return kernel_read_file_str[id]; } +int kernel_pread_file(struct file *file, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path(const char *path, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path_initns(const char *path, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_path_initns(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_fd(int fd, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id);
I remain concerned that adding these helpers will lead a poor interaction with LSMs, but I guess I get to hold my tongue. :)
I only need kernel_pread_file and kernel_pread_file_from_path_initns. kernel_pread_file_from_fd and kernel_pread_file_from_path were only added for completeness. And are really only helper functions called by their kernel_read_file* counterparts at this time. So they can be removed from this patch if that helps?
We could add pread functions that are "unsafe" in nature instead then? As I certainly do not need any integrity checks on the file for my driver. The real check is done by the card the data is loaded to whether is passes the linux security checks or not. And then, if someone does want to do something "safe" with preads another kernel_read_file_securelock/unlock could be added for those that need security for their partial reads?
Add request_partial_firmware_into_buf to allow for portions of firmware file to be read into a buffer. Necessary where firmware needs to be loaded in portions from file in memory constrained systems.
Signed-off-by: Scott Branden scott.branden@broadcom.com --- drivers/base/firmware_loader/firmware.h | 5 ++ drivers/base/firmware_loader/main.c | 79 +++++++++++++++++++------ include/linux/firmware.h | 12 ++++ 3 files changed, 79 insertions(+), 17 deletions(-)
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h index 933e2192fbe8..b5487f66dc45 100644 --- a/drivers/base/firmware_loader/firmware.h +++ b/drivers/base/firmware_loader/firmware.h @@ -32,6 +32,8 @@ * @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in * the platform's main firmware. If both this fallback and the sysfs * fallback are enabled, then this fallback will be tried first. + * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read + * entire file. */ enum fw_opt { FW_OPT_UEVENT = BIT(0), @@ -41,6 +43,7 @@ enum fw_opt { FW_OPT_NOCACHE = BIT(4), FW_OPT_NOFALLBACK_SYSFS = BIT(5), FW_OPT_FALLBACK_PLATFORM = BIT(6), + FW_OPT_PARTIAL = BIT(7), };
enum fw_status { @@ -68,6 +71,8 @@ struct fw_priv { void *data; size_t size; size_t allocated_size; + size_t offset; + u32 opt_flags; #ifdef CONFIG_FW_LOADER_PAGED_BUF bool is_paged_buf; struct page **pages; diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 7ca229760dfc..7a8d1877265c 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -168,7 +168,10 @@ static int fw_cache_piggyback_on_request(const char *name);
static struct fw_priv *__allocate_fw_priv(const char *fw_name, struct firmware_cache *fwc, - void *dbuf, size_t size) + void *dbuf, + size_t size, + size_t offset, + u32 opt_flags) { struct fw_priv *fw_priv;
@@ -186,6 +189,8 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name, fw_priv->fwc = fwc; fw_priv->data = dbuf; fw_priv->allocated_size = size; + fw_priv->offset = offset; + fw_priv->opt_flags = opt_flags; fw_state_init(fw_priv); #ifdef CONFIG_FW_LOADER_USER_HELPER INIT_LIST_HEAD(&fw_priv->pending_list); @@ -210,8 +215,11 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name) /* Returns 1 for batching firmware requests with the same name */ static int alloc_lookup_fw_priv(const char *fw_name, struct firmware_cache *fwc, - struct fw_priv **fw_priv, void *dbuf, - size_t size, u32 opt_flags) + struct fw_priv **fw_priv, + void *dbuf, + size_t size, + size_t offset, + u32 opt_flags) { struct fw_priv *tmp;
@@ -227,7 +235,7 @@ static int alloc_lookup_fw_priv(const char *fw_name, } }
- tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size); + tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, opt_flags); if (tmp) { INIT_LIST_HEAD(&tmp->list); if (!(opt_flags & FW_OPT_NOCACHE)) @@ -473,7 +481,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, /* 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; + if (fw_priv->opt_flags & FW_OPT_PARTIAL) + id = READING_FIRMWARE_PARTIAL_READ; + else + id = READING_FIRMWARE_PREALLOC_BUFFER; + msize = fw_priv->allocated_size; }
@@ -496,8 +508,10 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, fw_priv->size = 0;
/* load firmware files from the mount namespace of init */ - rc = kernel_read_file_from_path_initns(path, &buffer, - &size, msize, id); + rc = kernel_pread_file_from_path_initns(path, &buffer, + &size, msize, + fw_priv->offset, + id); if (rc) { if (rc != -ENOENT) dev_warn(device, "loading %s failed with error %d\n", @@ -684,7 +698,7 @@ int assign_fw(struct firmware *fw, struct device *device, u32 opt_flags) static int _request_firmware_prepare(struct firmware **firmware_p, const char *name, struct device *device, void *dbuf, size_t size, - u32 opt_flags) + size_t offset, u32 opt_flags) { struct firmware *firmware; struct fw_priv *fw_priv; @@ -703,7 +717,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name, }
ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size, - opt_flags); + offset, opt_flags);
/* * bind with 'priv' now to avoid warning in failure path @@ -750,7 +764,7 @@ static void fw_abort_batch_reqs(struct firmware *fw) static int _request_firmware(const struct firmware **firmware_p, const char *name, struct device *device, void *buf, size_t size, - u32 opt_flags) + size_t offset, u32 opt_flags) { struct firmware *fw = NULL; int ret; @@ -764,7 +778,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name, }
ret = _request_firmware_prepare(&fw, name, device, buf, size, - opt_flags); + offset, opt_flags); if (ret <= 0) /* error or already assigned */ goto out;
@@ -826,7 +840,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
/* Need to pin this module until return */ __module_get(THIS_MODULE); - ret = _request_firmware(firmware_p, name, device, NULL, 0, + ret = _request_firmware(firmware_p, name, device, NULL, 0, 0, FW_OPT_UEVENT); module_put(THIS_MODULE); return ret; @@ -853,7 +867,7 @@ int firmware_request_nowarn(const struct firmware **firmware, const char *name,
/* Need to pin this module until return */ __module_get(THIS_MODULE); - ret = _request_firmware(firmware, name, device, NULL, 0, + ret = _request_firmware(firmware, name, device, NULL, 0, 0, FW_OPT_UEVENT | FW_OPT_NO_WARN); module_put(THIS_MODULE); return ret; @@ -877,7 +891,7 @@ int request_firmware_direct(const struct firmware **firmware_p, int ret;
__module_get(THIS_MODULE); - ret = _request_firmware(firmware_p, name, device, NULL, 0, + ret = _request_firmware(firmware_p, name, device, NULL, 0, 0, FW_OPT_UEVENT | FW_OPT_NO_WARN | FW_OPT_NOFALLBACK_SYSFS); module_put(THIS_MODULE); @@ -902,7 +916,7 @@ int firmware_request_platform(const struct firmware **firmware,
/* Need to pin this module until return */ __module_get(THIS_MODULE); - ret = _request_firmware(firmware, name, device, NULL, 0, + ret = _request_firmware(firmware, name, device, NULL, 0, 0, FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM); module_put(THIS_MODULE); return ret; @@ -958,13 +972,44 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name, return -EOPNOTSUPP;
__module_get(THIS_MODULE); - ret = _request_firmware(firmware_p, name, device, buf, size, + ret = _request_firmware(firmware_p, name, device, buf, size, 0, FW_OPT_UEVENT | FW_OPT_NOCACHE); module_put(THIS_MODULE); return ret; } EXPORT_SYMBOL(request_firmware_into_buf);
+/** + * request_partial_firmware_into_buf() - load partial firmware into a previously allocated buffer + * @firmware_p: pointer to firmware image + * @name: name of firmware file + * @device: device for which firmware is being loaded and DMA region allocated + * @buf: address of buffer to load firmware into + * @size: size of buffer + * @offset: offset into file to read + * + * This function works pretty much like request_firmware_into_buf except + * it allows a partial read of the file. + */ +int +request_partial_firmware_into_buf(const struct firmware **firmware_p, + const char *name, struct device *device, + void *buf, size_t size, size_t offset) +{ + int ret; + + if (fw_cache_is_setup(device, name)) + return -EOPNOTSUPP; + + __module_get(THIS_MODULE); + ret = _request_firmware(firmware_p, name, device, buf, size, offset, + FW_OPT_UEVENT | FW_OPT_NOCACHE | + FW_OPT_PARTIAL); + module_put(THIS_MODULE); + return ret; +} +EXPORT_SYMBOL(request_partial_firmware_into_buf); + /** * release_firmware() - release the resource associated with a firmware image * @fw: firmware resource to release @@ -997,7 +1042,7 @@ static void request_firmware_work_func(struct work_struct *work)
fw_work = container_of(work, struct firmware_work, work);
- _request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0, + _request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0, 0, fw_work->opt_flags); fw_work->cont(fw, fw_work->context); put_device(fw_work->device); /* taken in request_firmware_nowait() */ diff --git a/include/linux/firmware.h b/include/linux/firmware.h index cb3e2c06ed8a..c15acadc6cf4 100644 --- a/include/linux/firmware.h +++ b/include/linux/firmware.h @@ -53,6 +53,9 @@ int request_firmware_direct(const struct firmware **fw, const char *name, struct device *device); int request_firmware_into_buf(const struct firmware **firmware_p, const char *name, struct device *device, void *buf, size_t size); +int request_partial_firmware_into_buf(const struct firmware **firmware_p, + const char *name, struct device *device, + void *buf, size_t size, size_t offset);
void release_firmware(const struct firmware *fw); #else @@ -102,6 +105,15 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p, return -EINVAL; }
+static inline int request_partial_firmware_into_buf + (const struct firmware **firmware_p, + const char *name, + struct device *device, + void *buf, size_t size, size_t offset) +{ + return -EINVAL; +} + #endif
int firmware_request_cache(struct device *device, const char *name);
On Mon, Jul 06, 2020 at 04:23:03PM -0700, Scott Branden wrote:
Add request_partial_firmware_into_buf to allow for portions of firmware file to be read into a buffer. Necessary where firmware needs to be loaded in portions from file in memory constrained systems.
Just tear out the differing "id" and just use FW_OPT_PARTIAL and I think if Luis is happy, you're all set.
On 2020-07-07 4:58 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:03PM -0700, Scott Branden wrote:
Add request_partial_firmware_into_buf to allow for portions of firmware file to be read into a buffer. Necessary where firmware needs to be loaded in portions from file in memory constrained systems.
Just tear out the differing "id" and just use FW_OPT_PARTIAL and I think if Luis is happy, you're all set.
I hope so. Also, I will need to call kernel_pread_file_from_path_initns() if FW_OPT_PARTIAL is set and kernel_read_file_from_path_initns() otherwise to avoid a swiss army-knife approach of calling a common function with multiple parameters.
Add additional hooks to test_firmware to pass in support for partial file read using request_firmware_into_buf. buf_size: size of buffer to request firmware into partial: indicates that a partial file request is being made file_offset: to indicate offset into file to request
Signed-off-by: Scott Branden scott.branden@broadcom.com --- lib/test_firmware.c | 154 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 142 insertions(+), 12 deletions(-)
diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 9fee2b93a8d1..48d8a3d5bea9 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -50,6 +50,9 @@ struct test_batched_req { * @name: the name of the firmware file to look for * @into_buf: when the into_buf is used if this is true * request_firmware_into_buf() will be used instead. + * @buf_size: size of buf to allocate when into_buf is true + * @file_offset: file offset to request when calling request_firmware_into_buf + * @partial: partial read opt when calling request_firmware_into_buf * @sync_direct: when the sync trigger is used if this is true * request_firmware_direct() will be used instead. * @send_uevent: whether or not to send a uevent for async requests @@ -89,6 +92,9 @@ struct test_batched_req { struct test_config { char *name; bool into_buf; + size_t buf_size; + size_t file_offset; + bool partial; bool sync_direct; bool send_uevent; u8 num_requests; @@ -183,6 +189,9 @@ static int __test_firmware_config_init(void) test_fw_config->num_requests = TEST_FIRMWARE_NUM_REQS; test_fw_config->send_uevent = true; test_fw_config->into_buf = false; + test_fw_config->buf_size = TEST_FIRMWARE_BUF_SIZE; + test_fw_config->file_offset = 0; + test_fw_config->partial = false; test_fw_config->sync_direct = false; test_fw_config->req_firmware = request_firmware; test_fw_config->test_result = 0; @@ -236,28 +245,35 @@ static ssize_t config_show(struct device *dev, dev_name(dev));
if (test_fw_config->name) - len += scnprintf(buf+len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "name:\t%s\n", test_fw_config->name); else - len += scnprintf(buf+len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "name:\tEMTPY\n");
- len += scnprintf(buf+len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "num_requests:\t%u\n", test_fw_config->num_requests);
- len += scnprintf(buf+len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "send_uevent:\t\t%s\n", test_fw_config->send_uevent ? "FW_ACTION_HOTPLUG" : "FW_ACTION_NOHOTPLUG"); - len += scnprintf(buf+len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "into_buf:\t\t%s\n", test_fw_config->into_buf ? "true" : "false"); - len += scnprintf(buf+len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, + "buf_size:\t%zu\n", test_fw_config->buf_size); + len += scnprintf(buf + len, PAGE_SIZE - len, + "file_offset:\t%zu\n", test_fw_config->file_offset); + len += scnprintf(buf + len, PAGE_SIZE - len, + "partial:\t\t%s\n", + test_fw_config->partial ? "true" : "false"); + len += scnprintf(buf + len, PAGE_SIZE - len, "sync_direct:\t\t%s\n", test_fw_config->sync_direct ? "true" : "false"); - len += scnprintf(buf+len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "read_fw_idx:\t%u\n", test_fw_config->read_fw_idx);
mutex_unlock(&test_fw_mutex); @@ -315,6 +331,30 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val) return snprintf(buf, PAGE_SIZE, "%d\n", val); }
+static int test_dev_config_update_size_t(const char *buf, + size_t size, + size_t *cfg) +{ + int ret; + long new; + + ret = kstrtol(buf, 10, &new); + if (ret) + return ret; + + mutex_lock(&test_fw_mutex); + *(size_t *)cfg = new; + mutex_unlock(&test_fw_mutex); + + /* Always return full write size even if we didn't consume all */ + return size; +} + +static ssize_t test_dev_config_show_size_t(char *buf, size_t val) +{ + return snprintf(buf, PAGE_SIZE, "%zu\n", val); +} + static ssize_t test_dev_config_show_int(char *buf, int val) { return snprintf(buf, PAGE_SIZE, "%d\n", val); @@ -400,6 +440,83 @@ static ssize_t config_into_buf_show(struct device *dev, } static DEVICE_ATTR_RW(config_into_buf);
+static ssize_t config_buf_size_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int rc; + + mutex_lock(&test_fw_mutex); + if (test_fw_config->reqs) { + pr_err("Must call release_all_firmware prior to changing config\n"); + rc = -EINVAL; + mutex_unlock(&test_fw_mutex); + goto out; + } + mutex_unlock(&test_fw_mutex); + + rc = test_dev_config_update_size_t(buf, count, + &test_fw_config->buf_size); + +out: + return rc; +} + +static ssize_t config_buf_size_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return test_dev_config_show_size_t(buf, test_fw_config->buf_size); +} +static DEVICE_ATTR_RW(config_buf_size); + +static ssize_t config_file_offset_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int rc; + + mutex_lock(&test_fw_mutex); + if (test_fw_config->reqs) { + pr_err("Must call release_all_firmware prior to changing config\n"); + rc = -EINVAL; + mutex_unlock(&test_fw_mutex); + goto out; + } + mutex_unlock(&test_fw_mutex); + + rc = test_dev_config_update_size_t(buf, count, + &test_fw_config->file_offset); + +out: + return rc; +} + +static ssize_t config_file_offset_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return test_dev_config_show_size_t(buf, test_fw_config->file_offset); +} +static DEVICE_ATTR_RW(config_file_offset); + +static ssize_t config_partial_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + return test_dev_config_update_bool(buf, + count, + &test_fw_config->partial); +} + +static ssize_t config_partial_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return test_dev_config_show_bool(buf, test_fw_config->partial); +} +static DEVICE_ATTR_RW(config_partial); + static ssize_t config_sync_direct_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) @@ -650,11 +767,21 @@ static int test_fw_run_batch_request(void *data) if (!test_buf) return -ENOSPC;
- req->rc = request_firmware_into_buf(&req->fw, - req->name, - req->dev, - test_buf, - TEST_FIRMWARE_BUF_SIZE); + if (test_fw_config->partial) + req->rc = request_partial_firmware_into_buf + (&req->fw, + req->name, + req->dev, + test_buf, + test_fw_config->buf_size, + test_fw_config->file_offset); + else + req->rc = request_firmware_into_buf + (&req->fw, + req->name, + req->dev, + test_buf, + test_fw_config->buf_size); if (!req->fw) kfree(test_buf); } else { @@ -927,6 +1054,9 @@ static struct attribute *test_dev_attrs[] = { TEST_FW_DEV_ATTR(config_name), TEST_FW_DEV_ATTR(config_num_requests), TEST_FW_DEV_ATTR(config_into_buf), + TEST_FW_DEV_ATTR(config_buf_size), + TEST_FW_DEV_ATTR(config_file_offset), + TEST_FW_DEV_ATTR(config_partial), TEST_FW_DEV_ATTR(config_sync_direct), TEST_FW_DEV_ATTR(config_send_uevent), TEST_FW_DEV_ATTR(config_read_fw_idx),
On Mon, Jul 06, 2020 at 04:23:04PM -0700, Scott Branden wrote:
Add additional hooks to test_firmware to pass in support for partial file read using request_firmware_into_buf. buf_size: size of buffer to request firmware into partial: indicates that a partial file request is being made file_offset: to indicate offset into file to request
Signed-off-by: Scott Branden scott.branden@broadcom.com
I am a fan of tests. :) If Luis gives an Ack here, you're good.
On 2020-07-07 4:59 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:04PM -0700, Scott Branden wrote:
Add additional hooks to test_firmware to pass in support for partial file read using request_firmware_into_buf. buf_size: size of buffer to request firmware into partial: indicates that a partial file request is being made file_offset: to indicate offset into file to request
Signed-off-by: Scott Branden scott.branden@broadcom.com
I am a fan of tests. :) If Luis gives an Ack here, you're good.
There were not even any tests for request_firmware_into_buf before I started this partial read support. Fortunately those base changes have already been accepted so I think this change is a simple addition to those accepted patches.
Add firmware tests for partial file reads of request_partial_firmware_into_buf.
Signed-off-by: Scott Branden scott.branden@broadcom.com --- .../selftests/firmware/fw_filesystem.sh | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+)
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh index fcc281373b4d..afc2e469ac06 100755 --- a/tools/testing/selftests/firmware/fw_filesystem.sh +++ b/tools/testing/selftests/firmware/fw_filesystem.sh @@ -149,6 +149,26 @@ config_unset_into_buf() echo 0 > $DIR/config_into_buf }
+config_set_buf_size() +{ + echo $1 > $DIR/config_buf_size +} + +config_set_file_offset() +{ + echo $1 > $DIR/config_file_offset +} + +config_set_partial() +{ + echo 1 > $DIR/config_partial +} + +config_unset_partial() +{ + echo 0 > $DIR/config_partial +} + config_set_sync_direct() { echo 1 > $DIR/config_sync_direct @@ -207,6 +227,35 @@ read_firmwares() done }
+read_partial_firmwares() +{ + if [ "$(cat $DIR/config_into_buf)" == "1" ]; then + fwfile="${FW_INTO_BUF}" + else + fwfile="${FW}" + fi + + if [ "$1" = "xzonly" ]; then + fwfile="${fwfile}-orig" + fi + + # Strip fwfile down to match partial offset and length + partial_data="$(cat $fwfile)" + partial_data="${partial_data:$2:$3}" + + for i in $(seq 0 3); do + config_set_read_fw_idx $i + + read_firmware="$(cat $DIR/read_firmware)" + + # Verify the contents are what we expect. + if [ $read_firmware != $partial_data ]; then + echo "request #$i: partial firmware was not loaded" >&2 + exit 1 + fi + done +} + read_firmwares_expect_nofile() { for i in $(seq 0 3); do @@ -319,6 +368,21 @@ test_batched_request_firmware_into_buf() echo "OK" }
+test_batched_request_partial_firmware_into_buf() +{ + echo -n "Batched request_partial_firmware_into_buf() $2 off=$3 size=$4 try #$1: " + config_reset + config_set_name $TEST_FIRMWARE_INTO_BUF_FILENAME + config_set_into_buf + config_set_partial + config_set_buf_size $4 + config_set_file_offset $3 + config_trigger_sync + read_partial_firmwares $2 $3 $4 + release_all_firmware + echo "OK" +} + test_batched_request_firmware_direct() { echo -n "Batched request_firmware_direct() $2 try #$1: " @@ -371,6 +435,22 @@ for i in $(seq 1 5); do test_batched_request_firmware_into_buf $i normal done
+for i in $(seq 1 5); do + test_batched_request_partial_firmware_into_buf $i normal 0 10 +done + +for i in $(seq 1 5); do + test_batched_request_partial_firmware_into_buf $i normal 0 5 +done + +for i in $(seq 1 5); do + test_batched_request_partial_firmware_into_buf $i normal 1 6 +done + +for i in $(seq 1 5); do + test_batched_request_partial_firmware_into_buf $i normal 2 10 +done + for i in $(seq 1 5); do test_batched_request_firmware_direct $i normal done
Add user space api for bcm-vk driver.
Signed-off-by: Scott Branden scott.branden@broadcom.com --- include/uapi/linux/misc/bcm_vk.h | 99 ++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 include/uapi/linux/misc/bcm_vk.h
diff --git a/include/uapi/linux/misc/bcm_vk.h b/include/uapi/linux/misc/bcm_vk.h new file mode 100644 index 000000000000..783087b7c31f --- /dev/null +++ b/include/uapi/linux/misc/bcm_vk.h @@ -0,0 +1,99 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */ +/* + * Copyright 2018-2020 Broadcom. + */ + +#ifndef __UAPI_LINUX_MISC_BCM_VK_H +#define __UAPI_LINUX_MISC_BCM_VK_H + +#include <linux/ioctl.h> +#include <linux/types.h> + +#define BCM_VK_MAX_FILENAME 64 + +struct vk_image { + __u32 type; /* Type of image */ +#define VK_IMAGE_TYPE_BOOT1 1 /* 1st stage (load to SRAM) */ +#define VK_IMAGE_TYPE_BOOT2 2 /* 2nd stage (load to DDR) */ + char filename[BCM_VK_MAX_FILENAME]; /* Filename of image */ +}; + +struct vk_reset { + __u32 arg1; + __u32 arg2; +}; + +#define VK_MAGIC 0x5e + +/* Load image to Valkyrie */ +#define VK_IOCTL_LOAD_IMAGE _IOW(VK_MAGIC, 0x2, struct vk_image) + +/* Send Reset to Valkyrie */ +#define VK_IOCTL_RESET _IOW(VK_MAGIC, 0x4, struct vk_reset) + +/* + * message block - basic unit in the message where a message's size is always + * N x sizeof(basic_block) + */ +struct vk_msg_blk { + __u8 function_id; +#define VK_FID_TRANS_BUF 5 +#define VK_FID_SHUTDOWN 8 + __u8 size; + __u16 trans_id; /* transport id, queue & msg_id */ + __u32 context_id; + __u32 args[2]; +#define VK_CMD_PLANES_MASK 0x000f /* number of planes to up/download */ +#define VK_CMD_UPLOAD 0x0400 /* memory transfer to vk */ +#define VK_CMD_DOWNLOAD 0x0500 /* memory transfer from vk */ +#define VK_CMD_MASK 0x0f00 /* command mask */ +}; + +#define VK_BAR_FWSTS 0x41c +#define VK_BAR_COP_FWSTS 0x428 +/* VK_FWSTS definitions */ +#define VK_FWSTS_RELOCATION_ENTRY BIT(0) +#define VK_FWSTS_RELOCATION_EXIT BIT(1) +#define VK_FWSTS_INIT_START BIT(2) +#define VK_FWSTS_ARCH_INIT_DONE BIT(3) +#define VK_FWSTS_PRE_KNL1_INIT_DONE BIT(4) +#define VK_FWSTS_PRE_KNL2_INIT_DONE BIT(5) +#define VK_FWSTS_POST_KNL_INIT_DONE BIT(6) +#define VK_FWSTS_INIT_DONE BIT(7) +#define VK_FWSTS_APP_INIT_START BIT(8) +#define VK_FWSTS_APP_INIT_DONE BIT(9) +#define VK_FWSTS_MASK 0xffffffff +#define VK_FWSTS_READY (VK_FWSTS_INIT_START | \ + VK_FWSTS_ARCH_INIT_DONE | \ + VK_FWSTS_PRE_KNL1_INIT_DONE | \ + VK_FWSTS_PRE_KNL2_INIT_DONE | \ + VK_FWSTS_POST_KNL_INIT_DONE | \ + VK_FWSTS_INIT_DONE | \ + VK_FWSTS_APP_INIT_START | \ + VK_FWSTS_APP_INIT_DONE) +/* Deinit */ +#define VK_FWSTS_APP_DEINIT_START BIT(23) +#define VK_FWSTS_APP_DEINIT_DONE BIT(24) +#define VK_FWSTS_DRV_DEINIT_START BIT(25) +#define VK_FWSTS_DRV_DEINIT_DONE BIT(26) +#define VK_FWSTS_RESET_DONE BIT(27) +#define VK_FWSTS_DEINIT_TRIGGERED (VK_FWSTS_APP_DEINIT_START | \ + VK_FWSTS_APP_DEINIT_DONE | \ + VK_FWSTS_DRV_DEINIT_START | \ + VK_FWSTS_DRV_DEINIT_DONE) +/* Last nibble for reboot reason */ +#define VK_FWSTS_RESET_REASON_SHIFT 28 +#define VK_FWSTS_RESET_REASON_MASK (0xf << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_SYS_PWRUP (0x0 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_MBOX_DB (0x1 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_M7_WDOG (0x2 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_TEMP (0x3 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_FLR (0x4 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_HOT (0x5 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_WARM (0x6 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_COLD (0x7 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_L1 (0x8 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_L0 (0x9 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_UNKNOWN (0xf << VK_FWSTS_RESET_REASON_SHIFT) + +#endif /* __UAPI_LINUX_MISC_BCM_VK_H */
Add maintainer entry for new Broadcom VK Driver
Signed-off-by: Scott Branden scott.branden@broadcom.com --- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS index fb5fa302d05b..996e06f78f27 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3662,6 +3662,13 @@ L: netdev@vger.kernel.org S: Supported F: drivers/net/ethernet/broadcom/tg3.*
+BROADCOM VK DRIVER +M: Scott Branden scott.branden@broadcom.com +L: bcm-kernel-feedback-list@broadcom.com +S: Supported +F: drivers/misc/bcm-vk/ +F: include/uapi/linux/misc/bcm_vk.h + BROCADE BFA FC SCSI DRIVER M: Anil Gurumurthy anil.gurumurthy@qlogic.com M: Sudarsana Kalluru sudarsana.kalluru@qlogic.com
Add FIRMWARE_PARTIAL_READ support for integrity measurement on partial reads of firmware files.
Signed-off-by: Scott Branden scott.branden@broadcom.com --- security/integrity/ima/ima_main.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 15f29fed6d9f..04a431924265 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -611,6 +611,9 @@ void ima_post_path_mknod(struct dentry *dentry) */ int ima_read_file(struct file *file, enum kernel_read_file_id read_id) { + enum ima_hooks func; + u32 secid; + /* * READING_FIRMWARE_PREALLOC_BUFFER * @@ -619,11 +622,27 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id) * of IMA's signature verification any more than when using two * buffers? */ - return 0; + if (read_id != READING_FIRMWARE_PARTIAL_READ) + return 0; + + if (!file) { + if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && + (ima_appraise & IMA_APPRAISE_ENFORCE)) { + pr_err("Prevent firmware loading_store.\n"); + return -EACCES; /* INTEGRITY_UNKNOWN */ + } + return 0; + } + + func = read_idmap[read_id] ?: FILE_CHECK; + security_task_getsecid(current, &secid); + return process_measurement(file, current_cred(), secid, NULL, + 0, MAY_READ, func); }
const int read_idmap[READING_MAX_ID] = { [READING_FIRMWARE] = FIRMWARE_CHECK, + [READING_FIRMWARE_PARTIAL_READ] = FIRMWARE_CHECK, [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK, [READING_MODULE] = MODULE_CHECK, [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, @@ -650,6 +669,9 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, enum ima_hooks func; u32 secid;
+ if (read_id == READING_FIRMWARE_PARTIAL_READ) + return 0; + if (!file && read_id == READING_FIRMWARE) { if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
Add FIRMWARE_PARTIAL_READ support for integrity measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle the "full read" semantics so that individual LSMs don't need to each duplicate the same efforts. As it happens, only IMA is impacted (SELinux ignores everything except modules, and LoadPin only cares about origin not contents).
Next is the problem that enum kernel_read_file_id is an object TYPE enum, not a HOW enum. (And it seems I missed the addition of READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.) That it's a partial read doesn't change _what_ you're reading: that's an internal API detail. What happens when I attempt to do a partial read of a kexec image? I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE, but the LSMs will have no idea it's a partial read.
Finally, what keeps the contents of the file from changing between the first call (which IMA will read the entire file for) and the next reads which will bypass IMA? I'd suggested that the open file must have writes disabled on it (as execve() does).
So, please redesign this: - do not add an enum - make the file unwritable for the life of having the handle open - make the "full read" happen as part of the first partial read so the LSMs don't have to reimplement everything
-Kees
Hi Kees,
You and others are certainly more experts in the filesystem and security infrastructure of the kernel. What I am trying to accomplish is a simple operation: request part of a file into a buffer rather than the whole file. If someone could add such support I would be more than happy to use it.
This has now bubbled into many other designs issues in the existing codebase. I will need more details on your comments - see below.
On 2020-07-06 8:08 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
Add FIRMWARE_PARTIAL_READ support for integrity measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle the "full read" semantics so that individual LSMs don't need to each duplicate the same efforts. As it happens, only IMA is impacted (SELinux ignores everything except modules, and LoadPin only cares about origin not contents).
Does your patch series "Fix misused kernel_read_file() enums" handle this because this suggestion is outside the scope of my change?
Next is the problem that enum kernel_read_file_id is an object TYPE enum, not a HOW enum. (And it seems I missed the addition of READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.) That it's a partial read doesn't change _what_ you're reading: that's an internal API detail. What happens when I attempt to do a partial read of a kexec image?
It does not appear there is any user of partial reads of kexec images? I have been informed by Greg K-H to not add apis that are not used so such support doesn't make sense to add at this time.
I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE, but the LSMs will have no idea it's a partial read.
The addition I am adding is for request_partial_firmware_into_buf. In order to do so it adds internal support for partial reads of firmware files, not kexec image.
The above seems outside the scope of my patch?
Finally, what keeps the contents of the file from changing between the first call (which IMA will read the entire file for) and the next reads which will bypass IMA?
The request is for a partial read. IMA ensures the whole file integrity even though I only do a partial read. The next partial read will re-read and check integrity of file.
I'd suggested that the open file must have writes disabled on it (as execve() does).
The file will be reopened and integrity checked on the next partial read (if there is one). So I don't think there is any change to be made here. If writes aren't already disabled for a whole file read then that is something that needs to be fixed in the existing code.
So, please redesign this:
- do not add an enum
I used existing infrastructure provided by Mimi but now looks like it will have to fit with your patches from yesterday.
- make the file unwritable for the life of having the handle open
It's no different than a full file read so no change to be made here.
- make the "full read" happen as part of the first partial read so the LSMs don't have to reimplement everything
Each partial read is an individual operation so I think a "full read" is performed every time if your security IMA is enabled. If someone wants to add a file lock and then partial reads in the kernel then that would be different than what is needed by the kernel driver.
-Kees
Regards, Scott
On Tue, Jul 07, 2020 at 10:13:42AM -0700, Scott Branden wrote:
You and others are certainly more experts in the filesystem and security infrastructure of the kernel. What I am trying to accomplish is a simple operation: request part of a file into a buffer rather than the whole file. If someone could add such support I would be more than happy to use it.
Sure, and I totally understand that, but as it happens, no one has stepped up with spare time to do that work. Since you're the person with the need for the API, it falls to you to do it. And I understand what feature creep feels like (I needed to fix one design problem[1] with timers, and I spent months sending hundreds of patches). Some times you get lucky and it's easy, and sometimes you end up touching something that needs a LOT of work to refactor before you can make your desired change work well with the rest of the kernel and be maintainable by other people into the future.
Quick tangent: I can't find in the many many threads where you explain how large these firmware images are and why existing kernel memory allocations are insufficient to load them. How large are these[2] files?
/lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin /lib/firmware/vk-boot2-bcm958401m2_a72.ecdsa.bin
For me, the requirements for partial read support are these things, which are the characteristics of the existing API:
- the LSM must be able to validate the entire file contents before any data is available to any reader. (Which was pointed out back in August 2019[3].) And "any" reader includes having a DMA window open on the memory range used for reading the contents (which was pointed out at by Mimi[4] but went unanswered and remains broken still in this v10, but I will comment separately on that.)
- the integrity of the file contents must be maintained between validation and delivery (currently this is handled internally via disallow_writes()).
This has now bubbled into many other designs issues in the existing codebase.
Correct -- this is one of the many difficulties of contributing to a large and complex code base with many maintainers. There can be a lot of requirements for the code that have nothing to do with seemingly more narrow areas of endeavour.
I will need more details on your comments - see below.
On 2020-07-06 8:08 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
Add FIRMWARE_PARTIAL_READ support for integrity measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle the "full read" semantics so that individual LSMs don't need to each duplicate the same efforts. As it happens, only IMA is impacted (SELinux ignores everything except modules, and LoadPin only cares about origin not contents).
Does your patch series "Fix misused kernel_read_file() enums" handle this because this suggestion is outside the scope of my change?
My proposed patch series cleans up a number of mistakes that were made to the kernel_read_file() API, and helps clarify my point about the enums being used for *what*, and not *how* or *where*, which needs to be fixed in this series and shouldn't be a big deal (I will reply to individual patches).
Next is the problem that enum kernel_read_file_id is an object TYPE enum, not a HOW enum. (And it seems I missed the addition of READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.) That it's a partial read doesn't change _what_ you're reading: that's an internal API detail. What happens when I attempt to do a partial read of a kexec image?
It does not appear there is any user of partial reads of kexec images? I have been informed by Greg K-H to not add apis that are not used so such support doesn't make sense to add at this time.
But you are proposing a generic API enhancement that any other user in the kernel may end up using. Just because the bcm-vk driver is the only user now, and IMA is the only LSM performing content analysis, it doesn't mean that there won't be another driver added later, nor another LSM. In fact, the new BPF LSM means that anything exposed by LSM hooks is now available for analysis.
I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE, but the LSMs will have no idea it's a partial read.
The addition I am adding is for request_partial_firmware_into_buf. In order to do so it adds internal support for partial reads of firmware files, not kexec image.
Yes, but you're changing kernel_read_file() APIs to do it. There are plenty of users of that API. Maybe they would like to also use partial reads?
$ git grep kernel_read_file | wc -l 77
The above seems outside the scope of my patch?
Unfortunately, it is not. Part of your responsibility as a kernel developer making API changes/additions is that those changes need to interact correctly with the rest of the kernel (and be maintainable).
Finally, what keeps the contents of the file from changing between the first call (which IMA will read the entire file for) and the next reads which will bypass IMA?
The request is for a partial read. IMA ensures the whole file integrity even though I only do a partial read. The next partial read will re-read and check integrity of file.
So, while terribly inefficient, I guess this approach is tenable. It means that each partial read will trigger a full read for LSMs that care about the hook.
So, to that end, I wonder why IMA doesn't do this for all file types?
It also means that we won't have a strict pairing of security_kernel_read_file() to security_kernel_post_read_file() in the LSMs, but it seems that only IMA currently explicitly cares about this (or maybe not at all).
I'm not entirely happy about using this design, but it does appear sufficient.
I'd suggested that the open file must have writes disabled on it (as execve() does).
The file will be reopened and integrity checked on the next partial read (if there is one). So I don't think there is any change to be made here. If writes aren't already disabled for a whole file read then that is something that needs to be fixed in the existing code.
My suggestion quoted here was operating on the idea that whole-file verification wasn't happening on every partial read, so this isn't a problem in that case.
So, please redesign this:
- do not add an enum
I used existing infrastructure provided by Mimi but now looks like it will have to fit with your patches from yesterday.
Right, this won't be hard. I will send a v2 of my patches to clarify the purpose of the 3 file content hooks (load_data, read_file, post_read_file), which might need renaming...
- make the file unwritable for the life of having the handle open
It's no different than a full file read so no change to be made here.
Correct.
- make the "full read" happen as part of the first partial read so the LSMs don't have to reimplement everything
Each partial read is an individual operation so I think a "full read" is performed every time if your security IMA is enabled. If someone wants to add a file lock and then partial reads in the kernel then that would be different than what is needed by the kernel driver.
So, given that Mimi is (I think?) satisfied with your approach here, I can't realistically complain. I still don't like the idea of each LSM needing to perform it's full read loop for the contents, but so be it: IMA will have the code, SELinux doesn't care (yet), and LoadPin doesn't care about contents.
-Kees
[1] https://lore.kernel.org/lkml/20170808003345.GA107289@beast/ git log --oneline --grep 'Convert timer' --author "Kees Cook" | wc -l 271 [2] https://lore.kernel.org/lkml/824407ae-8ab8-0fe3-bd72-270fce960ac5@broadcom.c... [3] https://lore.kernel.org/lkml/s5hsgpsqd49.wl-tiwai@suse.de/ [4] https://lore.kernel.org/lkml/1591622862.4638.74.camel@linux.ibm.com/
On 2020-07-07 4:36 p.m., Kees Cook wrote:
On Tue, Jul 07, 2020 at 10:13:42AM -0700, Scott Branden wrote:
You and others are certainly more experts in the filesystem and security infrastructure of the kernel. What I am trying to accomplish is a simple operation: request part of a file into a buffer rather than the whole file. If someone could add such support I would be more than happy to use it.
Sure, and I totally understand that, but as it happens, no one has stepped up with spare time to do that work. Since you're the person with the need for the API, it falls to you to do it. And I understand what feature creep feels like (I needed to fix one design problem[1] with timers, and I spent months sending hundreds of patches). Some times you get lucky and it's easy, and sometimes you end up touching something that needs a LOT of work to refactor before you can make your desired change work well with the rest of the kernel and be maintainable by other people into the future.
Quick tangent: I can't find in the many many threads where you explain how large these firmware images are and why existing kernel memory allocations are insufficient to load them. How large are these[2] files?
/lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin
This is on the order of a few MB at most.
/lib/firmware/vk-boot2-bcm958401m2_a72.ecdsa.bin
Some of these images are current 250MB. At we anticipate them growing to 512MB. And, we do have systems with the driver loading 16 cards in parallel with no requirement that they are the same images (although loading 16 different images to 16 different cards would be strange but possible).
For me, the requirements for partial read support are these things, which are the characteristics of the existing API:
- the LSM must be able to validate the entire file contents before any data is available to any reader. (Which was pointed out back in August 2019[3].)
I thought this was addressed in patch v6 "ima: aad FIRMWARE_PARTIAL_READ support" https://lkml.org/lkml/2020/6/5/1126 (although implementation not to your liking in current review)?
And "any" reader includes having a DMA window open on the memory range used for reading the contents (which was pointed out at by Mimi[4] but went unanswered and remains broken still in this v10, but I will comment separately on that.)
- the integrity of the file contents must be maintained between validation and delivery (currently this is handled internally via disallow_writes()).
I don't understand what you are staying here: I am request a partial firmware read into a buf. At the time the partial firmware is read into a buf it is validated by the security module if such integrity checks are enabled. If, at another time I wish to request another partial firmware into a buffer (could be the same part of the file or a different part of a file or from another file), the integrity check is performed again and the portion of the file I request should be put into the buffer. If a lock on a file is needed by somebody between these partial reads that is a different API and out of the scope of my patch series.
This has now bubbled into many other designs issues in the existing codebase.
Correct -- this is one of the many difficulties of contributing to a large and complex code base with many maintainers. There can be a lot of requirements for the code that have nothing to do with seemingly more narrow areas of endeavour.
Thanks at least for helping with guidance, I see your review is thought out and hopefully we can come to a conclusion as I feel we are fairly close with your changes.
I will need more details on your comments - see below.
On 2020-07-06 8:08 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
Add FIRMWARE_PARTIAL_READ support for integrity measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle the "full read" semantics so that individual LSMs don't need to each duplicate the same efforts. As it happens, only IMA is impacted (SELinux ignores everything except modules, and LoadPin only cares about origin not contents).
Does your patch series "Fix misused kernel_read_file() enums" handle this because this suggestion is outside the scope of my change?
My proposed patch series cleans up a number of mistakes that were made to the kernel_read_file() API, and helps clarify my point about the enums being used for *what*, and not *how* or *where*, which needs to be fixed in this series and shouldn't be a big deal (I will reply to individual patches).
Next is the problem that enum kernel_read_file_id is an object TYPE enum, not a HOW enum. (And it seems I missed the addition of READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.) That it's a partial read doesn't change _what_ you're reading: that's an internal API detail. What happens when I attempt to do a partial read of a kexec image?
It does not appear there is any user of partial reads of kexec images? I have been informed by Greg K-H to not add apis that are not used so such support doesn't make sense to add at this time.
But you are proposing a generic API enhancement that any other user in the kernel may end up using. Just because the bcm-vk driver is the only user now, and IMA is the only LSM performing content analysis, it doesn't mean that there won't be another driver added later, nor another LSM. In fact, the new BPF LSM means that anything exposed by LSM hooks is now available for analysis.
I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE, but the LSMs will have no idea it's a partial read.
The addition I am adding is for request_partial_firmware_into_buf. In order to do so it adds internal support for partial reads of firmware files, not kexec image.
Yes, but you're changing kernel_read_file() APIs to do it. There are plenty of users of that API. Maybe they would like to also use partial reads?
$ git grep kernel_read_file | wc -l 77
The above seems outside the scope of my patch?
Unfortunately, it is not. Part of your responsibility as a kernel developer making API changes/additions is that those changes need to interact correctly with the rest of the kernel (and be maintainable).
Finally, what keeps the contents of the file from changing between the first call (which IMA will read the entire file for) and the next reads which will bypass IMA?
The request is for a partial read. IMA ensures the whole file integrity even though I only do a partial read. The next partial read will re-read and check integrity of file.
So, while terribly inefficient, I guess this approach is tenable. It means that each partial read will trigger a full read for LSMs that care about the hook.
So, to that end, I wonder why IMA doesn't do this for all file types?
It also means that we won't have a strict pairing of security_kernel_read_file() to security_kernel_post_read_file() in the LSMs, but it seems that only IMA currently explicitly cares about this (or maybe not at all).
I'm not entirely happy about using this design, but it does appear sufficient.
Yes, terribly inefficient, but if somebody wants to do some optimization they are welcome to it. In fact, I want to call a partial read of a file with NO security. Can I add such a call instead? If we did that now then the inefficient read of the file multiple times to authenticate it each time wouldn't be introduced. In my use case the linux kernel security on the file is quite meaningless and a waste of time to even perform. Whether the file has been compromised, corrupt or otherwise the image is validated by the hardware after the image is loaded to the card to ensure it passes authentication.
I'd suggested that the open file must have writes disabled on it (as execve() does).
The file will be reopened and integrity checked on the next partial read (if there is one). So I don't think there is any change to be made here. If writes aren't already disabled for a whole file read then that is something that needs to be fixed in the existing code.
My suggestion quoted here was operating on the idea that whole-file verification wasn't happening on every partial read, so this isn't a problem in that case.
So, please redesign this:
- do not add an enum
I used existing infrastructure provided by Mimi but now looks like it will have to fit with your patches from yesterday.
Right, this won't be hard. I will send a v2 of my patches to clarify the purpose of the 3 file content hooks (load_data, read_file, post_read_file), which might need renaming...
I see your cleanup and merged with it. Will need to test everything again.
- make the file unwritable for the life of having the handle open
It's no different than a full file read so no change to be made here.
Correct.
- make the "full read" happen as part of the first partial read so the LSMs don't have to reimplement everything
Each partial read is an individual operation so I think a "full read" is performed every time if your security IMA is enabled. If someone wants to add a file lock and then partial reads in the kernel then that would be different than what is needed by the kernel driver.
So, given that Mimi is (I think?) satisfied with your approach here, I can't realistically complain. I still don't like the idea of each LSM needing to perform it's full read loop for the contents, but so be it: IMA will have the code, SELinux doesn't care (yet), and LoadPin doesn't care about contents.
I obviously am not aware of all of the security hooks and architecture in place but I do see your cleanups as a simplification over what was there.
-Kees
[1] https://lore.kernel.org/lkml/20170808003345.GA107289@beast/ git log --oneline --grep 'Convert timer' --author "Kees Cook" | wc -l 271 [2] https://lore.kernel.org/lkml/824407ae-8ab8-0fe3-bd72-270fce960ac5@broadcom.c... [3] https://lore.kernel.org/lkml/s5hsgpsqd49.wl-tiwai@suse.de/ [4] https://lore.kernel.org/lkml/1591622862.4638.74.camel@linux.ibm.com/
On 7/6/2020 4:23 PM, Scott Branden wrote:
This patch series adds partial read support via a new call request_partial_firmware_into_buf. Such support is needed when the whole file is not needed and/or only a smaller portion of the file will fit into allocated memory at any one time. In order to accept the enhanced API it has been requested that kernel selftests and upstreamed driver utilize the API enhancement and so are included in this patch series.
Also in this patch series is the addition of a new Broadcom VK driver utilizing the new request_firmware_into_buf enhanced API.
Further comment followed to add IMA support of the partial reads originating from request_firmware_into_buf calls. And another request to move existing kernel_read_file* functions to its own include file.
Do you have any way to separate the VK drivers submission from the request_partial_firmware_into_buf() work that you are doing? It looks like it is going to require quite a few iterations of this patch set for the firmware/fs/IMA part to be ironed out, so if you could get your driver separated out, it might help you achieve partial success here.
Hi Florian,
On 2020-07-07 9:38 p.m., Florian Fainelli wrote:
On 7/6/2020 4:23 PM, Scott Branden wrote:
This patch series adds partial read support via a new call request_partial_firmware_into_buf. Such support is needed when the whole file is not needed and/or only a smaller portion of the file will fit into allocated memory at any one time. In order to accept the enhanced API it has been requested that kernel selftests and upstreamed driver utilize the API enhancement and so are included in this patch series.
Also in this patch series is the addition of a new Broadcom VK driver utilizing the new request_firmware_into_buf enhanced API.
Further comment followed to add IMA support of the partial reads originating from request_firmware_into_buf calls. And another request to move existing kernel_read_file* functions to its own include file.
Do you have any way to separate the VK drivers submission from the request_partial_firmware_into_buf() work that you are doing? It looks like it is going to require quite a few iterations of this patch set for the firmware/fs/IMA part to be ironed out, so if you could get your driver separated out, it might help you achieve partial success here.
Originally I did not submit the driver. But Greg K-H rejected the pread support unless there was an actual user in the kernel. Hence the need to submit this all in the patch series.
linux-kselftest-mirror@lists.linaro.org
-
Florian Fainelli -
Kees Cook -
Mimi Zohar -
Scott Branden