[PATCH v7 0/8] firmware: add partial read support in request_firmware_into_buf

[PATCH v7 0/8] firmware: add partial read support in request_firmware_into_buf

[Scott Branden]

This patch series adds partial read support in request_firmware_into_buf.
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.

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 (8):
  fs: introduce kernel_pread_file* support
  firmware: add offset to request_firmware_into_buf
  test_firmware: add partial read support for request_firmware_into_buf
  firmware: test partial file reads of request_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           |   59 +-
 drivers/i2c/i2c-slave-eeprom.c                |    4 +-
 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                  |  408 +++++
 drivers/misc/bcm-vk/bcm_vk_dev.c              | 1312 +++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.c              | 1438 +++++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.h              |  201 +++
 drivers/misc/bcm-vk/bcm_vk_sg.c               |  271 ++++
 drivers/misc/bcm-vk/bcm_vk_sg.h               |   60 +
 drivers/misc/bcm-vk/bcm_vk_tty.c              |  352 ++++
 drivers/soc/qcom/mdt_loader.c                 |    7 +-
 fs/exec.c                                     |  101 +-
 include/linux/firmware.h                      |    8 +-
 include/linux/fs.h                            |   30 +
 include/uapi/linux/misc/bcm_vk.h              |   99 ++
 lib/test_firmware.c                           |  144 +-
 security/integrity/ima/ima_main.c             |   24 +-
 .../selftests/firmware/fw_filesystem.sh       |   80 +
 23 files changed, 4598 insertions(+), 54 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/uapi/linux/misc/bcm_vk.h

-- 
2.17.1

[PATCH v7 1/8] 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.  Existing kernel_read_file
functions call new kernel_pread_file functions with offset=0 and
opt=KERNEL_PREAD_WHOLE.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 fs/exec.c          | 95 ++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h | 29 ++++++++++++++
 2 files changed, 103 insertions(+), 21 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 306fc566171e..751f5ddc7538 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,10 +927,15 @@ 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 pos, loff_t max_size,
+		      enum kernel_pread_opt opt,
+		      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;
 
@@ -950,21 +955,31 @@ 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 ((opt == KERNEL_PREAD_PART) &&
+	    (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);
+		*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;
@@ -972,14 +987,16 @@ 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;
 
@@ -995,10 +1012,20 @@ 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, 0, max_size,
+				 KERNEL_PREAD_WHOLE, 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 pos,
+				loff_t max_size,
+				enum kernel_pread_opt opt,
+				enum kernel_read_file_id id)
 {
 	struct file *file;
 	int ret;
@@ -1010,15 +1037,24 @@ 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, pos, max_size, opt, 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, 0, max_size,
+					   KERNEL_PREAD_WHOLE, 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 pos,
+				       loff_t max_size,
+				       enum kernel_pread_opt opt,
+				       enum kernel_read_file_id id)
 {
 	struct file *file;
 	struct path root;
@@ -1036,14 +1072,24 @@ 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, pos, max_size, opt, 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, 0, max_size,
+						  KERNEL_PREAD_WHOLE, 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 pos,
+			      loff_t max_size,
+			      enum kernel_pread_opt opt,
+			      enum kernel_read_file_id id)
 {
 	struct fd f = fdget(fd);
 	int ret = -EBADF;
@@ -1051,11 +1097,18 @@ 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, pos, max_size, opt, 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, 0, max_size,
+					 KERNEL_PREAD_WHOLE, id);
+}
 EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
 
 #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
diff --git a/include/linux/fs.h b/include/linux/fs.h
index db604c7bd79c..aee7600958ef 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3048,12 +3048,41 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
 	return kernel_read_file_str[id];
 }
 
+/**
+ * enum kernel_pread_opt - options to control pread file loading behaviour
+ *
+ * @KERNEL_PREAD_WHOLE: Only Allow reading of whole file.
+ * @KERNEL_PREAD_PART: Allow reading part of file.
+ */
+enum kernel_pread_opt {
+	KERNEL_PREAD_WHOLE = 0,
+	KERNEL_PREAD_PART = BIT(0),
+};
+
+int kernel_pread_file(struct file *file, void **buf, loff_t *size,
+		      loff_t pos, loff_t max_size,
+		      enum kernel_pread_opt opt,
+		      enum kernel_read_file_id id);
 extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
 			    enum kernel_read_file_id);
+int kernel_pread_file_from_path(const char *path, void **buf,
+				loff_t *size, loff_t pos,
+				loff_t max_size,
+				enum kernel_pread_opt opt,
+				enum kernel_read_file_id id);
 extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
 				      enum kernel_read_file_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_pread_opt opt,
+				       enum kernel_read_file_id id);
 extern int kernel_read_file_from_path_initns(const char *, void **, loff_t *, loff_t,
 					     enum kernel_read_file_id);
+int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
+			      loff_t pos, loff_t max_size,
+			      enum kernel_pread_opt opt,
+			      enum kernel_read_file_id 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 *);
-- 
2.17.1

[PATCH v7 2/8] firmware: add offset to request_firmware_into_buf

[Scott Branden]

Add offset to request_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     | 53 +++++++++++++++++--------
 drivers/i2c/i2c-slave-eeprom.c          |  4 +-
 drivers/soc/qcom/mdt_loader.c           |  7 +++-
 include/linux/firmware.h                |  8 +++-
 lib/test_firmware.c                     |  4 +-
 6 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 933e2192fbe8..c9b6ba8d29d8 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;
+	enum kernel_pread_opt opt;
 #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 ca871b13524e..93e7fee42cd4 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -167,7 +167,9 @@ 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,
+					  enum kernel_pread_opt opt)
 {
 	struct fw_priv *fw_priv;
 
@@ -185,6 +187,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 = opt;
 	fw_state_init(fw_priv);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&fw_priv->pending_list);
@@ -210,9 +214,11 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_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)
+				size_t size, u32 opt_flags,
+				size_t offset)
 {
 	struct fw_priv *tmp;
+	enum kernel_pread_opt pread_opt;
 
 	spin_lock(&fwc->lock);
 	if (!(opt_flags & FW_OPT_NOCACHE)) {
@@ -226,7 +232,12 @@ static int alloc_lookup_fw_priv(const char *fw_name,
 		}
 	}
 
-	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
+	if (opt_flags & FW_OPT_PARTIAL)
+		pread_opt = KERNEL_PREAD_PART;
+	else
+		pread_opt = KERNEL_PREAD_WHOLE;
+
+	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_opt);
 	if (tmp) {
 		INIT_LIST_HEAD(&tmp->list);
 		if (!(opt_flags & FW_OPT_NOCACHE))
@@ -495,8 +506,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, fw_priv->offset,
+							msize,
+							fw_priv->opt, id);
 		if (rc) {
 			if (rc != -ENOENT)
 				dev_warn(device, "loading %s failed with error %d\n",
@@ -683,7 +696,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)
+			  u32 opt_flags, size_t offset)
 {
 	struct firmware *firmware;
 	struct fw_priv *fw_priv;
@@ -702,7 +715,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);
+				   opt_flags, offset);
 
 	/*
 	 * bind with 'priv' now to avoid warning in failure path
@@ -749,7 +762,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)
+		  u32 opt_flags, size_t offset)
 {
 	struct firmware *fw = NULL;
 	int ret;
@@ -763,7 +776,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	}
 
 	ret = _request_firmware_prepare(&fw, name, device, buf, size,
-					opt_flags);
+					opt_flags, offset);
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
@@ -826,7 +839,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,
-				FW_OPT_UEVENT);
+				FW_OPT_UEVENT, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -853,7 +866,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,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN);
+				FW_OPT_UEVENT | FW_OPT_NO_WARN, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -878,7 +891,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN |
-				FW_OPT_NOFALLBACK_SYSFS);
+				FW_OPT_NOFALLBACK_SYSFS, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -902,7 +915,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,
-				FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM);
+				FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -939,6 +952,8 @@ EXPORT_SYMBOL_GPL(firmware_request_cache);
  * @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
+ * @pread_opt: KERNEL_PREAD_PART to allow partial file read
  *
  * This function works pretty much like request_firmware(), but it doesn't
  * allocate a buffer to hold the firmware data. Instead, the firmware
@@ -949,16 +964,22 @@ EXPORT_SYMBOL_GPL(firmware_request_cache);
  */
 int
 request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
-			  struct device *device, void *buf, size_t size)
+			  struct device *device, void *buf, size_t size,
+			  size_t offset, enum kernel_pread_opt pread_opt)
 {
 	int ret;
+	u32 opt_flags;
 
 	if (fw_cache_is_setup(device, name))
 		return -EOPNOTSUPP;
 
 	__module_get(THIS_MODULE);
+	opt_flags = FW_OPT_UEVENT | FW_OPT_NOCACHE;
+	if (pread_opt == KERNEL_PREAD_PART)
+		opt_flags |= FW_OPT_PARTIAL;
+
 	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_NOCACHE);
+				opt_flags, offset);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -997,7 +1018,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,
-			  fw_work->opt_flags);
+			  fw_work->opt_flags, 0);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
diff --git a/drivers/i2c/i2c-slave-eeprom.c b/drivers/i2c/i2c-slave-eeprom.c
index 593f2fd39d17..facc5dff218f 100644
--- a/drivers/i2c/i2c-slave-eeprom.c
+++ b/drivers/i2c/i2c-slave-eeprom.c
@@ -129,7 +129,9 @@ static int i2c_slave_init_eeprom_data(struct eeprom_data *eeprom, struct i2c_cli
 
 	if (!ret) {
 		ret = request_firmware_into_buf(&fw, eeprom_data, &client->dev,
-						eeprom->buffer, size);
+						eeprom->buffer, size,
+						0,
+						KERNEL_PREAD_WHOLE);
 		if (ret)
 			return ret;
 		release_firmware(fw);
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193dec55..c9490c15da68 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -246,8 +246,11 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 		} else if (phdr->p_filesz) {
 			/* Firmware not large enough, load split-out segments */
 			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
-			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
-							ptr, phdr->p_filesz);
+			ret = request_firmware_into_buf
+						(&seg_fw, fw_name, dev,
+						 ptr, phdr->p_filesz,
+						 0,
+						 KERNEL_PREAD_WHOLE);
 			if (ret) {
 				dev_err(dev, "failed to load %s\n", fw_name);
 				break;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index cb3e2c06ed8a..a53a854294e2 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -4,6 +4,7 @@
 
 #include <linux/types.h>
 #include <linux/compiler.h>
+#include <linux/fs.h>
 #include <linux/gfp.h>
 
 #define FW_ACTION_NOHOTPLUG 0
@@ -52,7 +53,9 @@ int request_firmware_nowait(
 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);
+			      const char *name, struct device *device,
+			      void *buf, size_t size,
+			      size_t offset, enum kernel_pread_opt opt);
 
 void release_firmware(const struct firmware *fw);
 #else
@@ -97,7 +100,8 @@ static inline int request_firmware_direct(const struct firmware **fw,
 }
 
 static inline int request_firmware_into_buf(const struct firmware **firmware_p,
-	const char *name, struct device *device, void *buf, size_t size)
+	const char *name, struct device *device, void *buf, size_t size,
+	size_t offset, enum kernel_pread_opt opt)
 {
 	return -EINVAL;
 }
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 9fee2b93a8d1..af747660fe29 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -654,7 +654,9 @@ static int test_fw_run_batch_request(void *data)
 						    req->name,
 						    req->dev,
 						    test_buf,
-						    TEST_FIRMWARE_BUF_SIZE);
+						    TEST_FIRMWARE_BUF_SIZE,
+						    0,
+						    KERNEL_PREAD_WHOLE);
 		if (!req->fw)
 			kfree(test_buf);
 	} else {
-- 
2.17.1

[PATCH v7 3/8] 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 | 146 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 136 insertions(+), 10 deletions(-)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index af747660fe29..1a79611cae78 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)
@@ -645,18 +762,24 @@ static int test_fw_run_batch_request(void *data)
 
 	if (test_fw_config->into_buf) {
 		void *test_buf;
+		enum kernel_pread_opt pread_opt;
 
 		test_buf = kzalloc(TEST_FIRMWARE_BUF_SIZE, GFP_KERNEL);
 		if (!test_buf)
 			return -ENOSPC;
 
+		if (test_fw_config->partial)
+			pread_opt = KERNEL_PREAD_PART;
+		else
+			pread_opt = KERNEL_PREAD_WHOLE;
+
 		req->rc = request_firmware_into_buf(&req->fw,
 						    req->name,
 						    req->dev,
 						    test_buf,
-						    TEST_FIRMWARE_BUF_SIZE,
-						    0,
-						    KERNEL_PREAD_WHOLE);
+						    test_fw_config->buf_size,
+						    test_fw_config->file_offset,
+						    pread_opt);
 		if (!req->fw)
 			kfree(test_buf);
 	} else {
@@ -929,6 +1052,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 v7 4/8] firmware: test partial file reads of request_firmware_into_buf

[Scott Branden]

Add firmware tests for partial file reads of request_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..38e89ba1b4d3 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_firmwares_partial()
+{
+	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_firmware_into_buf_partial()
+{
+	echo -n "Batched request_firmware_into_buf_partial() $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_firmwares_partial $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_firmware_into_buf_partial $i normal 0 10
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_into_buf_partial $i normal 0 5
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_into_buf_partial $i normal 1 6
+done
+
+for i in $(seq 1 5); do
+	test_batched_request_firmware_into_buf_partial $i normal 2 10
+done
+
 for i in $(seq 1 5); do
 	test_batched_request_firmware_direct $i normal
 done
-- 
2.17.1

[PATCH v7 5/8] 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 v7 7/8] 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 411cd97bb485..ced084c526df 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3656,6 +3656,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 v7 8/8] 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>
---
 drivers/base/firmware_loader/main.c |  6 +++++-
 fs/exec.c                           |  6 ++++--
 include/linux/fs.h                  |  1 +
 security/integrity/ima/ima_main.c   | 24 +++++++++++++++++++++++-
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 93e7fee42cd4..d0c42194af17 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -483,7 +483,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 == KERNEL_PREAD_PART)
+			id = READING_FIRMWARE_PARTIAL_READ;
+		else
+			id = READING_FIRMWARE_PREALLOC_BUFFER;
+
 		msize = fw_priv->allocated_size;
 	}
 
diff --git a/fs/exec.c b/fs/exec.c
index 751f5ddc7538..06e2465d8d40 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -970,7 +970,8 @@ int kernel_pread_file(struct file *file, void **buf, loff_t *size,
 		goto out;
 	}
 
-	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
+	if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+	    (id != READING_FIRMWARE_PREALLOC_BUFFER))
 		*buf = vmalloc(alloc_size);
 	if (!*buf) {
 		ret = -ENOMEM;
@@ -1002,7 +1003,8 @@ int kernel_pread_file(struct file *file, void **buf, loff_t *size,
 
 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;
 		}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index aee7600958ef..1180091d704d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3020,6 +3020,7 @@ extern int do_pipe_flags(int *, int);
 #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)		\
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 800fb3bba418..fc5134807acf 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -609,6 +609,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
 	 *
@@ -617,11 +620,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,
@@ -648,6 +667,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 v7 1/8] fs: introduce kernel_pread_file* support

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
> -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 pos, loff_t max_size,
> +		      enum kernel_pread_opt opt,
> +		      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;
>  

Look, it's not your fault, but this is a great example of how we end
up with atrocious interfaces.  Someone comes along and implements a
simple DWIM interface that solves their problem.  Then somebody else
adds a slight variant that solves their problem, and so on and so on,
and we end up with this bonkers API where the arguments literally change
meaning depending on other arguments.

> @@ -950,21 +955,31 @@ 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 ((opt == KERNEL_PREAD_PART) &&
> +	    (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;

... like that.

I think what we actually want is:

ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
void vunmap_file_range(struct file *, void *buf);

If end > i_size, limit the allocation to i_size.  Returns the number
of bytes allocated, or a negative errno.  Writes the pointer allocated
to *bufp.  Internally, it should use the page cache to read in the pages
(taking appropriate reference counts).  Then it maps them using vmap()
instead of copying them to a private vmalloc() array.

kernel_read_file() can be converted to use this API.  The users will
need to be changed to call kernel_read_end(struct file *file, void *buf)
instead of vfree() so it can call allow_write_access() for them.

vmap_file_range() has a lot of potential uses.  I'm surprised we don't
have it already, to be honest.

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Mimi Zohar]

On Sat, 2020-06-06 at 08:52 -0700, Matthew Wilcox wrote:
> On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
> > -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 pos, loff_t max_size,
> > +		      enum kernel_pread_opt opt,
> > +		      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;
> >  
> 
> Look, it's not your fault, but this is a great example of how we end
> up with atrocious interfaces.  Someone comes along and implements a
> simple DWIM interface that solves their problem.  Then somebody else
> adds a slight variant that solves their problem, and so on and so on,
> and we end up with this bonkers API where the arguments literally change
> meaning depending on other arguments.
> 
> > @@ -950,21 +955,31 @@ 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 ((opt == KERNEL_PREAD_PART) &&
> > +	    (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;
> 
> ... like that.
> 
> I think what we actually want is:
> 
> ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
> void vunmap_file_range(struct file *, void *buf);
> 
> If end > i_size, limit the allocation to i_size.  Returns the number
> of bytes allocated, or a negative errno.  Writes the pointer allocated
> to *bufp.  Internally, it should use the page cache to read in the pages
> (taking appropriate reference counts).  Then it maps them using vmap()
> instead of copying them to a private vmalloc() array.
> 
> kernel_read_file() can be converted to use this API.  The users will
> need to be changed to call kernel_read_end(struct file *file, void *buf)
> instead of vfree() so it can call allow_write_access() for them.
> 
> vmap_file_range() has a lot of potential uses.  I'm surprised we don't
> have it already, to be honest.

Prior to kernel_read_file() the same or verify similar code existed in
multiple places in the kernel.  The kernel_read_file() API
consolidated the existing code adding the pre and post security hooks.

With this new design of not using a private vmalloc, will the file
data be accessible prior to the post security hooks?  From an IMA
perspective, the hooks are used for measuring and/or verifying the
integrity of the file.

Mimi

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Matthew Wilcox]

On Mon, Jun 08, 2020 at 09:03:21AM -0400, Mimi Zohar wrote:
> On Sat, 2020-06-06 at 08:52 -0700, Matthew Wilcox wrote:
> > On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
> > > -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 pos, loff_t max_size,
> > > +		      enum kernel_pread_opt opt,
> > > +		      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;
> > >  
> > 
> > Look, it's not your fault, but this is a great example of how we end
> > up with atrocious interfaces.  Someone comes along and implements a
> > simple DWIM interface that solves their problem.  Then somebody else
> > adds a slight variant that solves their problem, and so on and so on,
> > and we end up with this bonkers API where the arguments literally change
> > meaning depending on other arguments.
> > 
> > > @@ -950,21 +955,31 @@ 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 ((opt == KERNEL_PREAD_PART) &&
> > > +	    (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;
> > 
> > ... like that.
> > 
> > I think what we actually want is:
> > 
> > ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
> > void vunmap_file_range(struct file *, void *buf);
> > 
> > If end > i_size, limit the allocation to i_size.  Returns the number
> > of bytes allocated, or a negative errno.  Writes the pointer allocated
> > to *bufp.  Internally, it should use the page cache to read in the pages
> > (taking appropriate reference counts).  Then it maps them using vmap()
> > instead of copying them to a private vmalloc() array.
> > 
> > kernel_read_file() can be converted to use this API.  The users will
> > need to be changed to call kernel_read_end(struct file *file, void *buf)
> > instead of vfree() so it can call allow_write_access() for them.
> > 
> > vmap_file_range() has a lot of potential uses.  I'm surprised we don't
> > have it already, to be honest.
> 
> Prior to kernel_read_file() the same or verify similar code existed in
> multiple places in the kernel.  The kernel_read_file() API
> consolidated the existing code adding the pre and post security hooks.
> 
> With this new design of not using a private vmalloc, will the file
> data be accessible prior to the post security hooks?  From an IMA
> perspective, the hooks are used for measuring and/or verifying the
> integrity of the file.

File data is already accessible prior to the post security hooks.
Look how kernel_read_file works:

        ret = deny_write_access(file);
        ret = security_kernel_read_file(file, id);
                *buf = vmalloc(i_size);
                bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
        ret = security_kernel_post_read_file(file, *buf, i_size, id);

kernel_read() will read the data into the page cache and then copy it
into the vmalloc'd buffer.  There's nothing here to prevent read accesses
to the file.

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Mimi Zohar]

On Mon, 2020-06-08 at 06:16 -0700, Matthew Wilcox wrote:
> On Mon, Jun 08, 2020 at 09:03:21AM -0400, Mimi Zohar wrote:
> > On Sat, 2020-06-06 at 08:52 -0700, Matthew Wilcox wrote:
> > > On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
> > > > -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 pos, loff_t max_size,
> > > > +		      enum kernel_pread_opt opt,
> > > > +		      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;
> > > >  
> > > 
> > > Look, it's not your fault, but this is a great example of how we end
> > > up with atrocious interfaces.  Someone comes along and implements a
> > > simple DWIM interface that solves their problem.  Then somebody else
> > > adds a slight variant that solves their problem, and so on and so on,
> > > and we end up with this bonkers API where the arguments literally change
> > > meaning depending on other arguments.
> > > 
> > > > @@ -950,21 +955,31 @@ 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 ((opt == KERNEL_PREAD_PART) &&
> > > > +	    (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;
> > > 
> > > ... like that.
> > > 
> > > I think what we actually want is:
> > > 
> > > ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
> > > void vunmap_file_range(struct file *, void *buf);
> > > 
> > > If end > i_size, limit the allocation to i_size.  Returns the number
> > > of bytes allocated, or a negative errno.  Writes the pointer allocated
> > > to *bufp.  Internally, it should use the page cache to read in the pages
> > > (taking appropriate reference counts).  Then it maps them using vmap()
> > > instead of copying them to a private vmalloc() array.
> > > 
> > > kernel_read_file() can be converted to use this API.  The users will
> > > need to be changed to call kernel_read_end(struct file *file, void *buf)
> > > instead of vfree() so it can call allow_write_access() for them.
> > > 
> > > vmap_file_range() has a lot of potential uses.  I'm surprised we don't
> > > have it already, to be honest.
> > 
> > Prior to kernel_read_file() the same or verify similar code existed in
> > multiple places in the kernel.  The kernel_read_file() API
> > consolidated the existing code adding the pre and post security hooks.
> > 
> > With this new design of not using a private vmalloc, will the file
> > data be accessible prior to the post security hooks?  From an IMA
> > perspective, the hooks are used for measuring and/or verifying the
> > integrity of the file.
> 
> File data is already accessible prior to the post security hooks.
> Look how kernel_read_file works:
> 
>         ret = deny_write_access(file);
>         ret = security_kernel_read_file(file, id);
>                 *buf = vmalloc(i_size);
>                 bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
>         ret = security_kernel_post_read_file(file, *buf, i_size, id);
> 
> kernel_read() will read the data into the page cache and then copy it
> into the vmalloc'd buffer.  There's nothing here to prevent read accesses
> to the file.

The post security hook needs to access to the file data in order to
calculate the file hash.  The question is whether prior to returning
from kernel_read_file() the caller can access the file data.

Mimi

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Mimi Zohar]

On Mon, 2020-06-08 at 09:22 -0400, Mimi Zohar wrote:
> On Mon, 2020-06-08 at 06:16 -0700, Matthew Wilcox wrote:
> > On Mon, Jun 08, 2020 at 09:03:21AM -0400, Mimi Zohar wrote:
> > > On Sat, 2020-06-06 at 08:52 -0700, Matthew Wilcox wrote:
> > > > On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
> > > > > -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 pos, loff_t max_size,
> > > > > +		      enum kernel_pread_opt opt,
> > > > > +		      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;
> > > > >  
> > > > 
> > > > Look, it's not your fault, but this is a great example of how we end
> > > > up with atrocious interfaces.  Someone comes along and implements a
> > > > simple DWIM interface that solves their problem.  Then somebody else
> > > > adds a slight variant that solves their problem, and so on and so on,
> > > > and we end up with this bonkers API where the arguments literally change
> > > > meaning depending on other arguments.
> > > > 
> > > > > @@ -950,21 +955,31 @@ 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 ((opt == KERNEL_PREAD_PART) &&
> > > > > +	    (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;
> > > > 
> > > > ... like that.
> > > > 
> > > > I think what we actually want is:
> > > > 
> > > > ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
> > > > void vunmap_file_range(struct file *, void *buf);
> > > > 
> > > > If end > i_size, limit the allocation to i_size.  Returns the number
> > > > of bytes allocated, or a negative errno.  Writes the pointer allocated
> > > > to *bufp.  Internally, it should use the page cache to read in the pages
> > > > (taking appropriate reference counts).  Then it maps them using vmap()
> > > > instead of copying them to a private vmalloc() array.
> > > > 
> > > > kernel_read_file() can be converted to use this API.  The users will
> > > > need to be changed to call kernel_read_end(struct file *file, void *buf)
> > > > instead of vfree() so it can call allow_write_access() for them.
> > > > 
> > > > vmap_file_range() has a lot of potential uses.  I'm surprised we don't
> > > > have it already, to be honest.
> > > 
> > > Prior to kernel_read_file() the same or verify similar code existed in
> > > multiple places in the kernel.  The kernel_read_file() API
> > > consolidated the existing code adding the pre and post security hooks.
> > > 
> > > With this new design of not using a private vmalloc, will the file
> > > data be accessible prior to the post security hooks?  From an IMA
> > > perspective, the hooks are used for measuring and/or verifying the
> > > integrity of the file.
> > 
> > File data is already accessible prior to the post security hooks.
> > Look how kernel_read_file works:
> > 
> >         ret = deny_write_access(file);
> >         ret = security_kernel_read_file(file, id);
> >                 *buf = vmalloc(i_size);
> >                 bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
> >         ret = security_kernel_post_read_file(file, *buf, i_size, id);
> > 
> > kernel_read() will read the data into the page cache and then copy it
> > into the vmalloc'd buffer.  There's nothing here to prevent read accesses
> > to the file.
> 
> The post security hook needs to access to the file data in order to
> calculate the file hash.  The question is whether prior to returning
> from kernel_read_file() the caller can access the file data.

In the case of firmware, I'm asking if the device will be able to
access the file data before kernel_read_file() returns.

Mimi

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Matthew Wilcox]

On Mon, Jun 08, 2020 at 09:22:06AM -0400, Mimi Zohar wrote:
> On Mon, 2020-06-08 at 06:16 -0700, Matthew Wilcox wrote:
> > On Mon, Jun 08, 2020 at 09:03:21AM -0400, Mimi Zohar wrote:
> > > With this new design of not using a private vmalloc, will the file
> > > data be accessible prior to the post security hooks?  From an IMA
> > > perspective, the hooks are used for measuring and/or verifying the
> > > integrity of the file.
> > 
> > File data is already accessible prior to the post security hooks.
> > Look how kernel_read_file works:
> > 
> >         ret = deny_write_access(file);
> >         ret = security_kernel_read_file(file, id);
> >                 *buf = vmalloc(i_size);
> >                 bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
> >         ret = security_kernel_post_read_file(file, *buf, i_size, id);
> > 
> > kernel_read() will read the data into the page cache and then copy it
> > into the vmalloc'd buffer.  There's nothing here to prevent read accesses
> > to the file.
> 
> The post security hook needs to access to the file data in order to
> calculate the file hash.  The question is whether prior to returning
> from kernel_read_file() the caller can access the file data.

Whether you copy the data (as today) or map it (as I'm proposing),
the data goes into the page cache.  It's up to the security system to
block access to the page cache until it's been verified.

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Scott Branden]

Hi Matthew,

I am requesting the experts in the filesystem subsystem to come to a 
consensus here.
This is not my area of expertise at all but every time I have addressed 
all of the
outstanding concerns someone else comes along and raises another one.

Please see me comments below.

On 2020-06-06 8:52 a.m., Matthew Wilcox wrote:
> On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
>> -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;
Please note that how checkpatch generated the diff here.  The code 
modifications
below are for a new function kernel_pread_file, they do not modify the 
existing API
kernel_read_file.  kernel_read_file requests the ENTIRE file is read.  
So we need to be
able to differentiate whether it is ok to read just a portion of the 
file or not.
>> +int kernel_pread_file(struct file *file, void **buf, loff_t *size,
>> +		      loff_t pos, loff_t max_size,
>> +		      enum kernel_pread_opt opt,
>> +		      enum kernel_read_file_id id)
So, to share common code a new kernel_pread_opt needed to be added in 
order to specify whether
it was ok to read a partial file or not, and provide an offset into the 
file where to begin reading.
The meaning of parameters doesn't change in the bonkers API. max_size 
still means max size, etc.
These options are needed so common code can be shared with 
kernel_read_file api.

The partial read option is then needed further in the depths of the 
kernel read for IMA operation as IMA does
things differently for optimization of whether it is OK to do a partial 
read of the file or not.
>> +{
>> +	loff_t alloc_size;
>> +	loff_t buf_pos;
>> +	loff_t read_end;
>> +	loff_t i_size;
>>   	ssize_t bytes = 0;
>>   	int ret;
>>   
> Look, it's not your fault, but this is a great example of how we end
> up with atrocious interfaces.  Someone comes along and implements a
> simple DWIM interface that solves their problem.  Then somebody else
> adds a slight variant that solves their problem, and so on and so on,
> and we end up with this bonkers API where the arguments literally change
> meaning depending on other arguments.
I don't see what arguments are changing meaning.  Please explain what is 
changing meaning.
The diff below is for kernel_pread_file, not kernel_read_file. Perhaps 
that is where your confusion is.
>
>> @@ -950,21 +955,31 @@ 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 ((opt == KERNEL_PREAD_PART) &&
>> +	    (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;
> ... like that.
like what?  We need to determine how much of the file to read based on 
size of file, position in file, and max size we can read.
>
> I think what we actually want is:
>
> ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
> void vunmap_file_range(struct file *, void *buf);
>
> If end > i_size, limit the allocation to i_size.  Returns the number
> of bytes allocated, or a negative errno.  Writes the pointer allocated
> to *bufp.  Internally, it should use the page cache to read in the pages
> (taking appropriate reference counts).  Then it maps them using vmap()
> instead of copying them to a private vmalloc() array.
> kernel_read_file() can be converted to use this API.  The users will
> need to be changed to call kernel_read_end(struct file *file, void *buf)
> instead of vfree() so it can call allow_write_access() for them.
>
> vmap_file_range() has a lot of potential uses.  I'm surprised we don't
> have it already, to be honest.
Such a change sounds like it could be done in a later patch series.
It's an incomplete solution.  It would work for some of the needed 
operations but not others.
For kernel_read_file, I don't see how in your new API it indicates if 
the end of the file was reached or not.
Also, please note that buffers may be preallocated  and shouldn't be 
freed by the kernel in some cases and
allocated and freed by the kernel in others.

Your proposed change doesn't exist and is not simple as it sounds or 
meet all the needs of the existing kernel_read_file
function, IMA, and new partial kernel_pread_file?

Patch v7 does not break existing functions or rearchitect things in a 
dramatic way.  They fit into existing code,
will not break the existing codepaths (which some didn't even have a 
test case until I added one), and can
be improved upon as need with your vmap_file_range or others once those 
have been developed, tested, and
proven by someone.

I would like the experts here to decide on what needs to be done so we 
can move forward
and get kernel_pread_file support added soon.
Thanks,
Scott

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Matthew Wilcox]

On Mon, Jun 08, 2020 at 03:29:22PM -0700, Scott Branden wrote:
> Hi Matthew,
> 
> I am requesting the experts in the filesystem subsystem to come to a
> consensus here.
> This is not my area of expertise at all but every time I have addressed all
> of the
> outstanding concerns someone else comes along and raises another one.

I appreciate it's frustrating for you, but this is the nature of
patch review.  I haven't even read the first five or so submissions.
I can see them in my inbox and they look like long threads.  I'm not
particularly inclined to read them.  I happened to read v6, and reacted
to the API being ugly.

> Please see me comments below.
> 
> On 2020-06-06 8:52 a.m., Matthew Wilcox wrote:
> > On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
> > > -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;
> Please note that how checkpatch generated the diff here.  The code
> modifications
> below are for a new function kernel_pread_file, they do not modify the
> existing API
> kernel_read_file.  kernel_read_file requests the ENTIRE file is read.  So we
> need to be
> able to differentiate whether it is ok to read just a portion of the file or
> not.

You've gone about this in entirely the wrong way though.  This enum to
read the entire file or a partial is just bad design.

> > > +int kernel_pread_file(struct file *file, void **buf, loff_t *size,
> > > +		      loff_t pos, loff_t max_size,
> > > +		      enum kernel_pread_opt opt,
> > > +		      enum kernel_read_file_id id)
> So, to share common code a new kernel_pread_opt needed to be added in order
> to specify whether
> it was ok to read a partial file or not, and provide an offset into the file
> where to begin reading.
> The meaning of parameters doesn't change in the bonkers API. max_size still
> means max size, etc.
> These options are needed so common code can be shared with kernel_read_file
> api.

Does pread() in userspace take seven parameters?  No.  It takes four.
What you're doing is taking all the complexity of all of the interfaces
and stuffing it all down into the bottom function instead of handling
some of the complexity in the wrapper functions.  For example, you
could support the functionality of 'max_size' in kernel_read_file()
and leave it out of the kernel_pread_file() interface.

> > I think what we actually want is:
> > 
> > ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
> > void vunmap_file_range(struct file *, void *buf);
> > 
> > If end > i_size, limit the allocation to i_size.  Returns the number
> > of bytes allocated, or a negative errno.  Writes the pointer allocated
> > to *bufp.  Internally, it should use the page cache to read in the pages
> > (taking appropriate reference counts).  Then it maps them using vmap()
> > instead of copying them to a private vmalloc() array.
> > kernel_read_file() can be converted to use this API.  The users will
> > need to be changed to call kernel_read_end(struct file *file, void *buf)
> > instead of vfree() so it can call allow_write_access() for them.
> > 
> > vmap_file_range() has a lot of potential uses.  I'm surprised we don't
> > have it already, to be honest.
> Such a change sounds like it could be done in a later patch series.
> It's an incomplete solution.  It would work for some of the needed
> operations but not others.
> For kernel_read_file, I don't see how in your new API it indicates if the
> end of the file was reached or not.

That's the point.  It doesn't.  If a caller needs that, then they can
figure that out themselves.

> Also, please note that buffers may be preallocated  and shouldn't be freed
> by the kernel in some cases and
> allocated and freed by the kernel in others.

You're trying to build the swiss army knife of functions.  Swiss army
knives are useful, but they're no good for carving a steak.

> I would like the experts here to decide on what needs to be done so we can
> move forward
> and get kernel_pread_file support added soon.

You know, you haven't even said _why_ you want this.  The cover letter
just says "I want this", and doesn't say why it's needed.

Re: [PATCH v7 2/8] firmware: add offset to request_firmware_into_buf

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 10:04:52PM -0700, Scott Branden wrote:
>  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,
> +					  enum kernel_pread_opt opt)
>  {

Your types are screwed up.  size_t is the size of something in memory.
loff_t is an offset in a file.  This should be an loff_t.  One of the
other patches has the opposite problem.

(this is kind of a minor problem compared to all the complexity
problems, but it's worth mentioning)

Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

[Scott Branden]

Hi Matthew,

On 2020-06-09 6:21 a.m., Matthew Wilcox wrote:
> On Mon, Jun 08, 2020 at 03:29:22PM -0700, Scott Branden wrote:
>> Hi Matthew,
>>
>> I am requesting the experts in the filesystem subsystem to come to a
>> consensus here.
>> This is not my area of expertise at all but every time I have addressed all
>> of the
>> outstanding concerns someone else comes along and raises another one.
> I appreciate it's frustrating for you, but this is the nature of
> patch review.  I haven't even read the first five or so submissions.
> I can see them in my inbox and they look like long threads.  I'm not
> particularly inclined to read them.  I happened to read v6, and reacted
> to the API being ugly.
Thanks for the review.  Yes, I do see the enum being ugly now
and have removed it in v8 of the patch.  Hopefully it addresses
your concerns.  More comments below.
>
>> Please see me comments below.
>>
>> On 2020-06-06 8:52 a.m., Matthew Wilcox wrote:
>>> On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
>>>> -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;
>> Please note that how checkpatch generated the diff here.  The code
>> modifications
>> below are for a new function kernel_pread_file, they do not modify the
>> existing API
>> kernel_read_file.  kernel_read_file requests the ENTIRE file is read.  So we
>> need to be
>> able to differentiate whether it is ok to read just a portion of the file or
>> not.
> You've gone about this in entirely the wrong way though.  This enum to
> read the entire file or a partial is just bad design.
Your point on the enum is valid.
I've removed it from design.  Hopefully it is cleaner now.
>
>>>> +int kernel_pread_file(struct file *file, void **buf, loff_t *size,
>>>> +		      loff_t pos, loff_t max_size,
>>>> +		      enum kernel_pread_opt opt,
>>>> +		      enum kernel_read_file_id id)
>> So, to share common code a new kernel_pread_opt needed to be added in order
>> to specify whether
>> it was ok to read a partial file or not, and provide an offset into the file
>> where to begin reading.
>> The meaning of parameters doesn't change in the bonkers API. max_size still
>> means max size, etc.
>> These options are needed so common code can be shared with kernel_read_file
>> api.
> Does pread() in userspace take seven parameters?  No.  It takes four.
> What you're doing is taking all the complexity of all of the interfaces
> and stuffing it all down into the bottom function instead of handling
> some of the complexity in the wrapper functions.  For example, you
> could support the functionality of 'max_size' in kernel_read_file()
> and leave it out of the kernel_pread_file() interface.
I have removed the enum necessary in the kernel pread call now,
so it is down to 6.
The other 2 parameters are necessary as they are in kernel read.

max_size makes no sense to remove - it serves the same purpose
as in userspace pread and read functions.  To specify the max size
to read.
>>> I think what we actually want is:
>>>
>>> ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
>>> void vunmap_file_range(struct file *, void *buf);
>>>
>>> If end > i_size, limit the allocation to i_size.  Returns the number
>>> of bytes allocated, or a negative errno.  Writes the pointer allocated
>>> to *bufp.  Internally, it should use the page cache to read in the pages
>>> (taking appropriate reference counts).  Then it maps them using vmap()
>>> instead of copying them to a private vmalloc() array.
>>> kernel_read_file() can be converted to use this API.  The users will
>>> need to be changed to call kernel_read_end(struct file *file, void *buf)
>>> instead of vfree() so it can call allow_write_access() for them.
>>>
>>> vmap_file_range() has a lot of potential uses.  I'm surprised we don't
>>> have it already, to be honest.
>> Such a change sounds like it could be done in a later patch series.
>> It's an incomplete solution.  It would work for some of the needed
>> operations but not others.
>> For kernel_read_file, I don't see how in your new API it indicates if the
>> end of the file was reached or not.
> That's the point.  It doesn't.  If a caller needs that, then they can
> figure that out themselves.
No, they can't.  The caller only calls kernel_read_file once and expects
the whole file to be read.  The kernel_read_file doesn't work like 
userspace.
There is no tracking like userspace of where in the file you read?
>
>> Also, please note that buffers may be preallocated  and shouldn't be freed
>> by the kernel in some cases and
>> allocated and freed by the kernel in others.
> You're trying to build the swiss army knife of functions.  Swiss army
> knives are useful, but they're no good for carving a steak.
Hopefully I'm carving steak now.
>> I would like the experts here to decide on what needs to be done so we can
>> move forward
>> and get kernel_pread_file support added soon.
> You know, you haven't even said _why_ you want this.  The cover letter
> just says "I want this", and doesn't say why it's needed.
Cover letter updated.

Thanks,
Scott