[PATCH v10 0/9] firmware: add request_partial_firmware_into_buf

[PATCH v10 0/9] firmware: add request_partial_firmware_into_buf

[Scott Branden]

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

-- 
2.17.1

[PATCH v10 1/9] fs: move kernel_read_file* to its own include file

[Scott Branden]

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>
-- 
2.17.1

[PATCH v10 2/9] fs: introduce kernel_pread_file* support

[Scott Branden]

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);
-- 
2.17.1

[PATCH v10 3/9] firmware: add request_partial_firmware_into_buf

[Scott Branden]

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);
-- 
2.17.1

[PATCH v10 4/9] test_firmware: add partial read support for request_firmware_into_buf

[Scott Branden]

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),
-- 
2.17.1

[PATCH v10 5/9] firmware: test partial file reads of request_partial_firmware_into_buf

[Scott Branden]

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
-- 
2.17.1

[PATCH v10 6/9] bcm-vk: add bcm_vk UAPI

[Scott Branden]

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 */
-- 
2.17.1

[PATCH v10 8/9] MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver

[Scott Branden]

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>
-- 
2.17.1

[PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support

[Scott Branden]

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)) {
-- 
2.17.1

Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support

[Scott Branden]

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

Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support

[Kees Cook]

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.com/
[3] https://lore.kernel.org/lkml/s5hsgpsqd49.wl-tiwai@suse.de/
[4] https://lore.kernel.org/lkml/1591622862.4638.74.camel@linux.ibm.com/

-- 
Kees Cook

Re: [PATCH v10 1/9] fs: move kernel_read_file* to its own include file

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support

[Kees Cook]

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);
>  	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.

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.

>  
>  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. :)

-- 
Kees Cook

Re: [PATCH v10 3/9] firmware: add request_partial_firmware_into_buf

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH v10 4/9] test_firmware: add partial read support for request_firmware_into_buf

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH v10 7/9] misc: bcm-vk: add Broadcom VK driver

[Kees Cook]

On Mon, Jul 06, 2020 at 04:23:07PM -0700, Scott Branden wrote:
> Add Broadcom VK driver offload engine.
> This driver interfaces to the VK PCIe offload engine to perform
> should offload functions as video transcoding on multiple streams
> in parallel.  VK device is booted from files loaded using
> request_firmware_into_buf mechanism.  After booted card status is updated
> and messages can then be sent to the card.
> Such messages contain scatter gather list of addresses
> to pull data from the host to perform operations on.
> 
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>

nit: your S-o-b chain doesn't make sense (I would expect you at the end
since you're sending it and showing as the Author). Is it Co-developed-by?
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

> [...]
> +
> +		max_buf = SZ_4M;
> +		bufp = dma_alloc_coherent(dev,
> +					  max_buf,
> +					  &boot_dma_addr, GFP_KERNEL);
> +		if (!bufp) {
> +			dev_err(dev, "Error allocating 0x%zx\n", max_buf);
> +			ret = -ENOMEM;
> +			goto err_buf_out;
> +		}
> +
> +		bcm_vk_buf_notify(vk, bufp, boot_dma_addr, max_buf);
> +	} else {
> +		dev_err(dev, "Error invalid image type 0x%x\n", load_type);
> +		ret = -EINVAL;
> +		goto err_buf_out;
> +	}
> +
> +	ret = request_partial_firmware_into_buf(&fw, filename, dev,
> +						bufp, max_buf, 0);

Unless I don't understand what's happening here, this needs to be
reordered if you're going to keep Mimi happy and disallow the device
being able to see the firmware before it has been verified. (i.e. please
load the firmware before mapping DMA across the buffer.)

-- 
Kees Cook

Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support

[Mimi Zohar]

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

Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support

[Scott Branden]

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.com/
> [3] https://lore.kernel.org/lkml/s5hsgpsqd49.wl-tiwai@suse.de/
> [4] https://lore.kernel.org/lkml/1591622862.4638.74.camel@linux.ibm.com/
>

Re: [PATCH v10 1/9] fs: move kernel_read_file* to its own include file

[Scott Branden]

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.
>

Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support

[Scott Branden]

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. :)
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?
>

Re: [PATCH v10 3/9] firmware: add request_partial_firmware_into_buf

[Scott Branden]

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.

Re: [PATCH v10 4/9] test_firmware: add partial read support for request_firmware_into_buf

[Scott Branden]

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.
>

Re: [PATCH v10 7/9] misc: bcm-vk: add Broadcom VK driver

[Scott Branden]

On 2020-07-07 5:03 p.m., Kees Cook wrote:
> On Mon, Jul 06, 2020 at 04:23:07PM -0700, Scott Branden wrote:
>> Add Broadcom VK driver offload engine.
>> This driver interfaces to the VK PCIe offload engine to perform
>> should offload functions as video transcoding on multiple streams
>> in parallel.  VK device is booted from files loaded using
>> request_firmware_into_buf mechanism.  After booted card status is updated
>> and messages can then be sent to the card.
>> Such messages contain scatter gather list of addresses
>> to pull data from the host to perform operations on.
>>
>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>
> nit: your S-o-b chain doesn't make sense (I would expect you at the end
> since you're sending it and showing as the Author). Is it Co-developed-by?
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
Yes, Co-developed-by.  Will adjust.
>
>> [...]
>> +
>> +		max_buf = SZ_4M;
>> +		bufp = dma_alloc_coherent(dev,
>> +					  max_buf,
>> +					  &boot_dma_addr, GFP_KERNEL);
>> +		if (!bufp) {
>> +			dev_err(dev, "Error allocating 0x%zx\n", max_buf);
>> +			ret = -ENOMEM;
>> +			goto err_buf_out;
>> +		}
>> +
>> +		bcm_vk_buf_notify(vk, bufp, boot_dma_addr, max_buf);
>> +	} else {
>> +		dev_err(dev, "Error invalid image type 0x%x\n", load_type);
>> +		ret = -EINVAL;
>> +		goto err_buf_out;
>> +	}
>> +
>> +	ret = request_partial_firmware_into_buf(&fw, filename, dev,
>> +						bufp, max_buf, 0);
> Unless I don't understand what's happening here, this needs to be
> reordered if you're going to keep Mimi happy and disallow the device
> being able to see the firmware before it has been verified. (i.e. please
> load the firmware before mapping DMA across the buffer.)
I don't understand your concern here.  We request partial firmware into 
a buffer that we allocated.
After loading it we signal the card the firmware has been loaded into 
that memory region.
The card then pulls the data into its internal memory.  And, 
authenticates it.

Even if the card randomly read and writes to that buffer it shouldn't 
matter to the linux kernel security subsystem.
It passed the security check already when placed in the buffer.
If there is a concern could we add an "nosecurity" 
request_partial_firmware_into_buf instead as there is no need for any 
security on this particular request?

Re: [PATCH v10 0/9] firmware: add request_partial_firmware_into_buf

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support

[Scott Branden]

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?
>>
>

Re: [PATCH v10 0/9] firmware: add request_partial_firmware_into_buf

[Scott Branden]

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.