Please consider this version series ready for upstream acceptance.
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.
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 (7): 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
MAINTAINERS | 7 + drivers/base/firmware_loader/firmware.h | 5 + drivers/base/firmware_loader/main.c | 52 +- drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/bcm-vk/Kconfig | 29 + drivers/misc/bcm-vk/Makefile | 12 + drivers/misc/bcm-vk/bcm_vk.h | 435 +++++ drivers/misc/bcm-vk/bcm_vk_dev.c | 1256 +++++++++++++++ drivers/misc/bcm-vk/bcm_vk_legacy.c | 89 + drivers/misc/bcm-vk/bcm_vk_msg.c | 1425 +++++++++++++++++ drivers/misc/bcm-vk/bcm_vk_msg.h | 196 +++ 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 | 96 +- include/linux/firmware.h | 8 +- include/linux/fs.h | 20 + include/uapi/linux/misc/bcm_vk.h | 99 ++ lib/test_firmware.c | 144 +- .../selftests/firmware/fw_filesystem.sh | 80 + 22 files changed, 4596 insertions(+), 49 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_legacy.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
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 flags=KERNEL_PREAD_FLAG_WHOLE.
Signed-off-by: Scott Branden scott.branden@broadcom.com --- fs/exec.c | 96 ++++++++++++++++++++++++++++++++++++---------- include/linux/fs.h | 20 ++++++++++ 2 files changed, 95 insertions(+), 21 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c index 06b4c550af5d..cfab212fab9d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -896,10 +896,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 pos, loff_t max_size, unsigned int flags, + 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;
@@ -919,21 +923,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 ((flags & KERNEL_PREAD_FLAG_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; @@ -941,14 +955,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;
@@ -964,10 +980,20 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, allow_write_access(file); return ret; } +EXPORT_SYMBOL_GPL(kernel_pread_file); + +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_FLAG_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, unsigned int flags, + enum kernel_read_file_id id) { struct file *file; int ret; @@ -979,15 +1005,25 @@ 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, flags, id); fput(file); return ret; } +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path); + +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_FLAG_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) +extern int kernel_pread_file_from_path_initns(const char *path, void **buf, + loff_t *size, loff_t pos, + loff_t max_size, + unsigned int flags, + enum kernel_read_file_id id) { struct file *file; struct path root; @@ -1005,14 +1041,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, flags, id); fput(file); return ret; } +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path_initns); + +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_FLAG_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, unsigned int flags, + enum kernel_read_file_id id) { struct fd f = fdget(fd); int ret = -EBADF; @@ -1020,11 +1066,19 @@ 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, flags, id); out: fdput(f); return ret; } +EXPORT_SYMBOL_GPL(kernel_pread_file_from_fd); + +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_FLAG_WHOLE, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t len) diff --git a/include/linux/fs.h b/include/linux/fs.h index 45cc10cdf6dd..4bba930b77d7 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -3014,12 +3014,32 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) return kernel_read_file_str[id]; }
+/* Flags used by kernel_pread_file functions */ +#define KERNEL_PREAD_FLAG_WHOLE 0x0000 /* Only Allow reading of whole file */ +#define KERNEL_PREAD_FLAG_PART 0x0001 /* Allow reading part of file */ + +extern int kernel_pread_file(struct file *file, void **buf, loff_t *size, + loff_t pos, loff_t max_size, unsigned int flags, + enum kernel_read_file_id id); extern int kernel_read_file(struct file *, void **, loff_t *, loff_t, enum kernel_read_file_id); +extern int kernel_pread_file_from_path(const char *path, void **buf, + loff_t *size, loff_t pos, + loff_t max_size, unsigned int flags, + 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); +extern int kernel_pread_file_from_path_initns(const char *path, void **buf, + loff_t *size, loff_t pos, + loff_t max_size, + unsigned int flags, + 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); +extern int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, + loff_t pos, loff_t max_size, + unsigned int flags, + 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 *);
On Thu, May 07, 2020 at 05:27:33PM -0700, Scott Branden wrote:
diff --git a/fs/exec.c b/fs/exec.c index 06b4c550af5d..cfab212fab9d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -896,10 +896,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 pos, loff_t max_size, unsigned int flags,
You use int flags, but.. these are mutually exclusive operations, and so flags is a misnomer. Just use an enum instead of a define, that way we can use kdoc for documentation.
+EXPORT_SYMBOL_GPL(kernel_pread_file); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path_initns); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_fd);
If no one is using these don't export them. I think you only use one of these. In fact just remove the code from the ones which are not used.
Luis
Hi Luis,
A few comments inline before I cleanup.
On 2020-05-12 5:27 p.m., Luis Chamberlain wrote:
On Thu, May 07, 2020 at 05:27:33PM -0700, Scott Branden wrote:
diff --git a/fs/exec.c b/fs/exec.c index 06b4c550af5d..cfab212fab9d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -896,10 +896,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 pos, loff_t max_size, unsigned int flags,You use int flags, but.. these are mutually exclusive operations, and so flags is a misnomer. Just use an enum instead of a define, that way we can use kdoc for documentation.
OK, flags could be used to expand with additional flag options in the future (without change to function prototype, but will change to enum if that is what is desired.
+EXPORT_SYMBOL_GPL(kernel_pread_file); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path_initns); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_fd);
If no one is using these don't export them. I think you only use one of these. In fact just remove the code from the ones which are not used.
I do not use them but added them to provide consistent api with kernel_read_file_* functions. That way someone can take advantage of the _from_path and from_fd variants in the future if desired. But if you want them removed it is simple to drop the EXPORT_SYMBOL_GPL and then add that back when first driver that calls them needs them in the future.
Note: Existing kernel_read_file_from_path_initns is not used in the kernel. Should we delete that as well?
Luis
Thanks, Scott
On Tue, May 12, 2020 at 11:23:27PM -0700, Scott Branden wrote:
Hi Luis,
A few comments inline before I cleanup.
On 2020-05-12 5:27 p.m., Luis Chamberlain wrote:
On Thu, May 07, 2020 at 05:27:33PM -0700, Scott Branden wrote:
diff --git a/fs/exec.c b/fs/exec.c index 06b4c550af5d..cfab212fab9d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -896,10 +896,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 pos, loff_t max_size, unsigned int flags,You use int flags, but.. these are mutually exclusive operations, and so flags is a misnomer. Just use an enum instead of a define, that way we can use kdoc for documentation.
OK, flags could be used to expand with additional flag options in the future (without change to function prototype, but will change to enum if that is what is desired.
+EXPORT_SYMBOL_GPL(kernel_pread_file); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path_initns); +EXPORT_SYMBOL_GPL(kernel_pread_file_from_fd);
If no one is using these don't export them. I think you only use one of these. In fact just remove the code from the ones which are not used.
I do not use them but added them to provide consistent api with kernel_read_file_* functions. That way someone can take advantage of the _from_path and from_fd variants in the future if desired. But if you want them removed it is simple to drop the EXPORT_SYMBOL_GPL and then add that back when first driver that calls them needs them in the future.
We do not export symbols when there are no in-kernel users.
Note: Existing kernel_read_file_from_path_initns is not used in the kernel. Should we delete that as well?
Probably, yes.
thanks,
greg k-h
On 2020-05-12 11:51 p.m., Greg Kroah-Hartman wrote:
On Tue, May 12, 2020 at 11:23:27PM -0700, Scott Branden wrote:
Hi Luis,
A few comments inline before I cleanup.
We do not export symbols when there are no in-kernel users.
Note: Existing kernel_read_file_from_path_initns is not used in the kernel. Should we delete that as well?
Probably, yes.
I found drivers/base/firmware_loader calls kernel_read_file_from_path_initns so EXPORT_SYMBOL_GPL can stay there.
thanks,
greg k-h
[Cc'ing linux-security-module, linux-integrity]
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
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 flags=KERNEL_PREAD_FLAG_WHOLE.
Signed-off-by: Scott Branden scott.branden@broadcom.com
<snip>
@@ -941,14 +955,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;
Prior to the patch set that introduced this security hook, firmware would be read twice, once for measuring/appraising the firmware and again reading the file contents into memory. Partial reads will break both IMA's measuring the file and appraising the file signatures.
Mimi
Hi Mimi,
On 2020-05-13 11:39 a.m., Mimi Zohar wrote:
[Cc'ing linux-security-module, linux-integrity]
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
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 flags=KERNEL_PREAD_FLAG_WHOLE.
Signed-off-by: Scott Branden scott.branden@broadcom.com
<snip>
@@ -941,14 +955,16 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
The checkpatch shows this as kernel_read_file when it is actually the new function kernel_pread_file. Please see the call to kernel_pread_file from kernel_read_file in the complete patch rather this snippet.
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;
Prior to the patch set that introduced this security hook, firmware would be read twice, once for measuring/appraising the firmware and again reading the file contents into memory. Partial reads will break both IMA's measuring the file and appraising the file signatures.
The partial file read support is needed for request_firmware_into_buf from drivers. The EXPORT_SYMBOL_GPL is being removed so that there can be no abuse of the partial file read support. Such file integrity checks are not needed for this use case. The partial file (firmware image) is actually downloaded in portions and verified on the device it is loaded to.
Regards, Scott
On 2020-05-13 11:53 a.m., Scott Branden wrote:
Hi Mimi,
On 2020-05-13 11:39 a.m., Mimi Zohar wrote:
[Cc'ing linux-security-module, linux-integrity]
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
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 flags=KERNEL_PREAD_FLAG_WHOLE.
Signed-off-by: Scott Branden scott.branden@broadcom.com
<snip>
@@ -941,14 +955,16 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
The checkpatch shows this as kernel_read_file when it is actually the new function kernel_pread_file.
typo: "checkpatch" should be "patch diff"
Please see the call to kernel_pread_file from kernel_read_file in the complete patch rather this snippet.
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;
Prior to the patch set that introduced this security hook, firmware would be read twice, once for measuring/appraising the firmware and again reading the file contents into memory. Partial reads will break both IMA's measuring the file and appraising the file signatures.
The partial file read support is needed for request_firmware_into_buf from drivers. The EXPORT_SYMBOL_GPL is being removed so that there can be no abuse of the partial file read support. Such file integrity checks are not needed for this use case. The partial file (firmware image) is actually downloaded in portions and verified on the device it is loaded to.
Regards, Scott
On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote:
Hi Mimi,
On 2020-05-13 11:39 a.m., Mimi Zohar wrote:
[Cc'ing linux-security-module, linux-integrity]
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
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 flags=KERNEL_PREAD_FLAG_WHOLE.
Signed-off-by: Scott Branden scott.branden@broadcom.com
<snip>
@@ -941,14 +955,16 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
The checkpatch shows this as kernel_read_file when it is actually the new function kernel_pread_file. Please see the call to kernel_pread_file from kernel_read_file in the complete patch rather this snippet.
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;
Prior to the patch set that introduced this security hook, firmware would be read twice, once for measuring/appraising the firmware and again reading the file contents into memory. Partial reads will break both IMA's measuring the file and appraising the file signatures.
The partial file read support is needed for request_firmware_into_buf from drivers. The EXPORT_SYMBOL_GPL is being removed so that there can be no abuse of the partial file read support. Such file integrity checks are not needed for this use case. The partial file (firmware image) is actually downloaded in portions and verified on the device it is loaded to.
It's all fine that the device will verify the firmware, but shouldn't the kernel be able to also verify the firmware file signature it is providing to the device, before providing it?
The device firmware is being downloaded piecemeal from somewhere and won't be measured?
Mimi
On 2020-05-13 12:03 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote:
Hi Mimi,
On 2020-05-13 11:39 a.m., Mimi Zohar wrote:
[Cc'ing linux-security-module, linux-integrity]
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
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 flags=KERNEL_PREAD_FLAG_WHOLE.
Signed-off-by: Scott Branden scott.branden@broadcom.com
<snip>
@@ -941,14 +955,16 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
The checkpatch shows this as kernel_read_file when it is actually the new function kernel_pread_file. Please see the call to kernel_pread_file from kernel_read_file in the complete patch rather this snippet.
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;
Prior to the patch set that introduced this security hook, firmware would be read twice, once for measuring/appraising the firmware and again reading the file contents into memory. Partial reads will break both IMA's measuring the file and appraising the file signatures.
The partial file read support is needed for request_firmware_into_buf from drivers. The EXPORT_SYMBOL_GPL is being removed so that there can be no abuse of the partial file read support. Such file integrity checks are not needed for this use case. The partial file (firmware image) is actually downloaded in portions and verified on the device it is loaded to.
It's all fine that the device will verify the firmware, but shouldn't the kernel be able to also verify the firmware file signature it is providing to the device, before providing it?
Even if the kernel successfully verified the firmware file signature it would just be wasting its time. The kernel in these use cases is not always trusted. The device needs to authenticate the firmware image itself.
The device firmware is being downloaded piecemeal from somewhere and won't be measured?
It doesn't need to be measured for current driver needs. If someone has such need the infrastructure could be added to the kernel at a later date. Existing functionality is not broken in any way by this patch series.
Mimi
On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote:
On 2020-05-13 12:03 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote:
Even if the kernel successfully verified the firmware file signature it would just be wasting its time. The kernel in these use cases is not always trusted. The device needs to authenticate the firmware image itself.
There are also environments where the kernel is trusted and limits the firmware being provided to the device to one which they signed.
The device firmware is being downloaded piecemeal from somewhere and won't be measured?
It doesn't need to be measured for current driver needs.
Sure the device doesn't need the kernel measuring the firmware, but hardened environments do measure firmware.
If someone has such need the infrastructure could be added to the kernel at a later date. Existing functionality is not broken in any way by this patch series.
Wow! You're saying that your patch set takes precedence over the existing expectations and can break them.
Mimi
On 2020-05-13 12:39 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote:
On 2020-05-13 12:03 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote:
Even if the kernel successfully verified the firmware file signature it would just be wasting its time. The kernel in these use cases is not always trusted. The device needs to authenticate the firmware image itself.
There are also environments where the kernel is trusted and limits the firmware being provided to the device to one which they signed.
The device firmware is being downloaded piecemeal from somewhere and won't be measured?
It doesn't need to be measured for current driver needs.
Sure the device doesn't need the kernel measuring the firmware, but hardened environments do measure firmware.
If someone has such need the infrastructure could be added to the kernel at a later date. Existing functionality is not broken in any way by this patch series.
Wow! You're saying that your patch set takes precedence over the existing expectations and can break them.
Huh? I said existing functionality is NOT broken by this patch series.
Mimi
On Wed, 2020-05-13 at 12:41 -0700, Scott Branden wrote:
On 2020-05-13 12:39 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote:
On 2020-05-13 12:03 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote:
Even if the kernel successfully verified the firmware file signature it would just be wasting its time. The kernel in these use cases is not always trusted. The device needs to authenticate the firmware image itself.
There are also environments where the kernel is trusted and limits the firmware being provided to the device to one which they signed.
The device firmware is being downloaded piecemeal from somewhere and won't be measured?
It doesn't need to be measured for current driver needs.
Sure the device doesn't need the kernel measuring the firmware, but hardened environments do measure firmware.
If someone has such need the infrastructure could be added to the kernel at a later date. Existing functionality is not broken in any way by this patch series.
Wow! You're saying that your patch set takes precedence over the existing expectations and can break them.
Huh? I said existing functionality is NOT broken by this patch series.
Assuming a system is configured to measure and appraise firmware (rules below), with this change the firmware file will not be properly measured and will fail signature verification.
Sample IMA policy rules: measure func=FIRMWARE_CHECK appraise func=FIRMWARE_CHECK appraise_type=imasig
Mimi
On Wed, May 13, 2020 at 05:20:14PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:41 -0700, Scott Branden wrote:
On 2020-05-13 12:39 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote:
On 2020-05-13 12:03 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote:
Even if the kernel successfully verified the firmware file signature it would just be wasting its time. The kernel in these use cases is not always trusted. The device needs to authenticate the firmware image itself.
There are also environments where the kernel is trusted and limits the firmware being provided to the device to one which they signed.
The device firmware is being downloaded piecemeal from somewhere and won't be measured?
It doesn't need to be measured for current driver needs.
Sure the device doesn't need the kernel measuring the firmware, but hardened environments do measure firmware.
If someone has such need the infrastructure could be added to the kernel at a later date. Existing functionality is not broken in any way by this patch series.
Wow! You're saying that your patch set takes precedence over the existing expectations and can break them.
Huh? I said existing functionality is NOT broken by this patch series.
Assuming a system is configured to measure and appraise firmware (rules below), with this change the firmware file will not be properly measured and will fail signature verification.
Sample IMA policy rules: measure func=FIRMWARE_CHECK appraise func=FIRMWARE_CHECK appraise_type=imasig
Would a pre and post lsm hook for pread do it?
Luis
On Wed, 2020-05-13 at 21:28 +0000, Luis Chamberlain wrote:
On Wed, May 13, 2020 at 05:20:14PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:41 -0700, Scott Branden wrote:
On 2020-05-13 12:39 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote:
On 2020-05-13 12:03 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote:
Even if the kernel successfully verified the firmware file signature it would just be wasting its time. The kernel in these use cases is not always trusted. The device needs to authenticate the firmware image itself.
There are also environments where the kernel is trusted and limits the firmware being provided to the device to one which they signed.
The device firmware is being downloaded piecemeal from somewhere and won't be measured?
It doesn't need to be measured for current driver needs.
Sure the device doesn't need the kernel measuring the firmware, but hardened environments do measure firmware.
If someone has such need the infrastructure could be added to the kernel at a later date. Existing functionality is not broken in any way by this patch series.
Wow! You're saying that your patch set takes precedence over the existing expectations and can break them.
Huh? I said existing functionality is NOT broken by this patch series.
Assuming a system is configured to measure and appraise firmware (rules below), with this change the firmware file will not be properly measured and will fail signature verification.
Sample IMA policy rules: measure func=FIRMWARE_CHECK appraise func=FIRMWARE_CHECK appraise_type=imasig
Would a pre and post lsm hook for pread do it?
IMA currently measures and verifies the firmware file signature on the post hook. The file is read once into a buffer. With this change, IMA would need to be on the pre hook, to read the entire file, calculating the file hash and verifying the file signature. Basically the firmware would be read once for IMA and again for the device.
Mimi
On 2020-05-13 3:12 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 21:28 +0000, Luis Chamberlain wrote:
On Wed, May 13, 2020 at 05:20:14PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:41 -0700, Scott Branden wrote:
On 2020-05-13 12:39 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote:
On 2020-05-13 12:03 p.m., Mimi Zohar wrote: > On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote: Even if the kernel successfully verified the firmware file signature it would just be wasting its time. The kernel in these use cases is not always trusted. The device needs to authenticate the firmware image itself.
There are also environments where the kernel is trusted and limits the firmware being provided to the device to one which they signed.
> The device firmware is being downloaded piecemeal from somewhere and > won't be measured? It doesn't need to be measured for current driver needs.
Sure the device doesn't need the kernel measuring the firmware, but hardened environments do measure firmware.
If someone has such need the infrastructure could be added to the kernel at a later date. Existing functionality is not broken in any way by this patch series.
Wow! You're saying that your patch set takes precedence over the existing expectations and can break them.
Huh? I said existing functionality is NOT broken by this patch series.
Assuming a system is configured to measure and appraise firmware (rules below), with this change the firmware file will not be properly measured and will fail signature verification.
So no existing functionality has been broken.
Sample IMA policy rules: measure func=FIRMWARE_CHECK appraise func=FIRMWARE_CHECK appraise_type=imasig
Would a pre and post lsm hook for pread do it?
IMA currently measures and verifies the firmware file signature on the post hook. The file is read once into a buffer. With this change, IMA would need to be on the pre hook, to read the entire file, calculating the file hash and verifying the file signature. Basically the firmware would be read once for IMA and again for the device.
The entire file may not fit into available memory to measure and verify. Hence the reason for a partial read.
Is there some way we could add a flag when calling the request_firmware_into_buf to indicate it is ok that the data requested does not need to be measured?
Mimi
On Wed, 2020-05-13 at 15:48 -0700, Scott Branden wrote:
On 2020-05-13 3:12 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 21:28 +0000, Luis Chamberlain wrote:
On Wed, May 13, 2020 at 05:20:14PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:41 -0700, Scott Branden wrote:
On 2020-05-13 12:39 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote: > On 2020-05-13 12:03 p.m., Mimi Zohar wrote: >> On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote: > Even if the kernel successfully verified the firmware file signature it > would just be wasting its time. The kernel in these use cases is not always > trusted. The device needs to authenticate the firmware image itself. There are also environments where the kernel is trusted and limits the firmware being provided to the device to one which they signed.
>> The device firmware is being downloaded piecemeal from somewhere and >> won't be measured? > It doesn't need to be measured for current driver needs. Sure the device doesn't need the kernel measuring the firmware, but hardened environments do measure firmware.
> If someone has such need the infrastructure could be added to the kernel > at a later date. Existing functionality is not broken in any way by > this patch series. Wow! You're saying that your patch set takes precedence over the existing expectations and can break them.
Huh? I said existing functionality is NOT broken by this patch series.
Assuming a system is configured to measure and appraise firmware (rules below), with this change the firmware file will not be properly measured and will fail signature verification.
So no existing functionality has been broken.
Sample IMA policy rules: measure func=FIRMWARE_CHECK appraise func=FIRMWARE_CHECK appraise_type=imasig
Would a pre and post lsm hook for pread do it?
IMA currently measures and verifies the firmware file signature on the post hook. The file is read once into a buffer. With this change, IMA would need to be on the pre hook, to read the entire file, calculating the file hash and verifying the file signature. Basically the firmware would be read once for IMA and again for the device.
The entire file may not fit into available memory to measure and verify. Hence the reason for a partial read.
Previously, IMA pre-read the file in page size chunks in order to calculate the file hash. To avoid reading the file twice, the file is now read into a buffer.
Is there some way we could add a flag when calling the request_firmware_into_buf to indicate it is ok that the data requested does not need to be measured?
The decision as to what needs to be measured is a policy decision left up to the system owner, which they express by loading an IMA policy.
Mimi
On Wed, May 13, 2020 at 07:00:43PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 15:48 -0700, Scott Branden wrote:
On 2020-05-13 3:12 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 21:28 +0000, Luis Chamberlain wrote:
On Wed, May 13, 2020 at 05:20:14PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:41 -0700, Scott Branden wrote:
On 2020-05-13 12:39 p.m., Mimi Zohar wrote: > On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote: >> On 2020-05-13 12:03 p.m., Mimi Zohar wrote: >>> On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote: >> Even if the kernel successfully verified the firmware file signature it >> would just be wasting its time. The kernel in these use cases is not always >> trusted. The device needs to authenticate the firmware image itself. > There are also environments where the kernel is trusted and limits the > firmware being provided to the device to one which they signed. > >>> The device firmware is being downloaded piecemeal from somewhere and >>> won't be measured? >> It doesn't need to be measured for current driver needs. > Sure the device doesn't need the kernel measuring the firmware, but > hardened environments do measure firmware. > >> If someone has such need the infrastructure could be added to the kernel >> at a later date. Existing functionality is not broken in any way by >> this patch series. > Wow! You're saying that your patch set takes precedence over the > existing expectations and can break them. Huh? I said existing functionality is NOT broken by this patch series.
Assuming a system is configured to measure and appraise firmware (rules below), with this change the firmware file will not be properly measured and will fail signature verification.
So no existing functionality has been broken.
Sample IMA policy rules: measure func=FIRMWARE_CHECK appraise func=FIRMWARE_CHECK appraise_type=imasig
Would a pre and post lsm hook for pread do it?
IMA currently measures and verifies the firmware file signature on the post hook. The file is read once into a buffer. With this change, IMA would need to be on the pre hook, to read the entire file, calculating the file hash and verifying the file signature. Basically the firmware would be read once for IMA and again for the device.
The entire file may not fit into available memory to measure and verify. Hence the reason for a partial read.
Previously, IMA pre-read the file in page size chunks in order to calculate the file hash. To avoid reading the file twice, the file is now read into a buffer.
Can the VFS be locked in some way and then using the partial reads would trigger the "read twice" mode? I.e. something like:
open first partial read: lock file contents (?) perform full page-at-a-time-read-and-measure rewind, read partial other partial reads final partial read unlock
On Wed, 2020-05-13 at 16:34 -0700, Kees Cook wrote:
On Wed, May 13, 2020 at 07:00:43PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 15:48 -0700, Scott Branden wrote:
On 2020-05-13 3:12 p.m., Mimi Zohar wrote:
On Wed, 2020-05-13 at 21:28 +0000, Luis Chamberlain wrote:
On Wed, May 13, 2020 at 05:20:14PM -0400, Mimi Zohar wrote:
On Wed, 2020-05-13 at 12:41 -0700, Scott Branden wrote: > On 2020-05-13 12:39 p.m., Mimi Zohar wrote: >> On Wed, 2020-05-13 at 12:18 -0700, Scott Branden wrote: >>> On 2020-05-13 12:03 p.m., Mimi Zohar wrote: >>>> On Wed, 2020-05-13 at 11:53 -0700, Scott Branden wrote: >>> Even if the kernel successfully verified the firmware file signature it >>> would just be wasting its time. The kernel in these use cases is not always >>> trusted. The device needs to authenticate the firmware image itself. >> There are also environments where the kernel is trusted and limits the >> firmware being provided to the device to one which they signed. >> >>>> The device firmware is being downloaded piecemeal from somewhere and >>>> won't be measured? >>> It doesn't need to be measured for current driver needs. >> Sure the device doesn't need the kernel measuring the firmware, but >> hardened environments do measure firmware. >> >>> If someone has such need the infrastructure could be added to the kernel >>> at a later date. Existing functionality is not broken in any way by >>> this patch series. >> Wow! You're saying that your patch set takes precedence over the >> existing expectations and can break them. > Huh? I said existing functionality is NOT broken by this patch series. Assuming a system is configured to measure and appraise firmware (rules below), with this change the firmware file will not be properly measured and will fail signature verification.
So no existing functionality has been broken.
Sample IMA policy rules: measure func=FIRMWARE_CHECK appraise func=FIRMWARE_CHECK appraise_type=imasig
Would a pre and post lsm hook for pread do it?
IMA currently measures and verifies the firmware file signature on the post hook. The file is read once into a buffer. With this change, IMA would need to be on the pre hook, to read the entire file, calculating the file hash and verifying the file signature. Basically the firmware would be read once for IMA and again for the device.
The entire file may not fit into available memory to measure and verify. Hence the reason for a partial read.
Previously, IMA pre-read the file in page size chunks in order to calculate the file hash. To avoid reading the file twice, the file is now read into a buffer.
Can the VFS be locked in some way and then using the partial reads would trigger the "read twice" mode? I.e. something like:
open first partial read: lock file contents (?) perform full page-at-a-time-read-and-measure rewind, read partial other partial reads final partial read unlock
The security_kernel_read_file(), the pre-hook, would need to be moved after getting the file size, but yes that's exactly what would be done in the pre-hook, when the current offset is 0 and the file size and buffer size aren't the same.
Mimi
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 | 52 +++++++++++++++++-------- drivers/soc/qcom/mdt_loader.c | 7 +++- include/linux/firmware.h | 8 +++- lib/test_firmware.c | 4 +- 5 files changed, 55 insertions(+), 21 deletions(-)
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h index 25836a6afc9f..1147dae01148 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; + unsigned int 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 76f79913916d..4552b7bb819f 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -167,7 +167,8 @@ 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, unsigned int flags) { struct fw_priv *fw_priv;
@@ -185,6 +186,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->flags = flags; fw_state_init(fw_priv); #ifdef CONFIG_FW_LOADER_USER_HELPER INIT_LIST_HEAD(&fw_priv->pending_list); @@ -210,9 +213,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, enum fw_opt opt_flags) + size_t size, enum fw_opt opt_flags, + size_t offset) { struct fw_priv *tmp; + unsigned int pread_flags;
spin_lock(&fwc->lock); if (!(opt_flags & FW_OPT_NOCACHE)) { @@ -226,7 +231,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_flags = KERNEL_PREAD_FLAG_PART; + else + pread_flags = KERNEL_PREAD_FLAG_WHOLE; + + tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags); if (tmp) { INIT_LIST_HEAD(&tmp->list); if (!(opt_flags & FW_OPT_NOCACHE)) @@ -495,8 +505,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->flags, id); if (rc) { if (rc != -ENOENT) dev_warn(device, "loading %s failed with error %d\n", @@ -687,7 +699,7 @@ int assign_fw(struct firmware *fw, struct device *device, static int _request_firmware_prepare(struct firmware **firmware_p, const char *name, struct device *device, void *dbuf, size_t size, - enum fw_opt opt_flags) + enum fw_opt opt_flags, size_t offset) { struct firmware *firmware; struct fw_priv *fw_priv; @@ -706,7 +718,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 @@ -753,7 +765,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, - enum fw_opt opt_flags) + enum fw_opt opt_flags, size_t offset) { struct firmware *fw = NULL; int ret; @@ -767,7 +779,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;
@@ -830,7 +842,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; } @@ -857,7 +869,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; } @@ -882,7 +894,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; } @@ -906,7 +918,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; } @@ -943,6 +955,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_flags: KERNEL_PREAD_FLAG_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 @@ -953,16 +967,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, unsigned int pread_flags) { int ret; + enum fw_opt 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_flags & KERNEL_PREAD_FLAG_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; } @@ -1001,7 +1021,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/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index 24cd193dec55..00f3359f4f61 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_FLAG_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 4bbd0afd91b7..b98d17fb3bd1 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 @@ -53,7 +54,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, unsigned int pread_flags);
void release_firmware(const struct firmware *fw); #else @@ -98,7 +101,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, unsigned int pread_flags) { return -EINVAL; } diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 9fee2b93a8d1..55176b0a8318 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_FLAG_WHOLE); if (!req->fw) kfree(test_buf); } else {
On Thu, May 07, 2020 at 05:27:34PM -0700, Scott Branden wrote:
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 | 52 +++++++++++++++++-------- drivers/soc/qcom/mdt_loader.c | 7 +++- include/linux/firmware.h | 8 +++- lib/test_firmware.c | 4 +- 5 files changed, 55 insertions(+), 21 deletions(-)
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h index 25836a6afc9f..1147dae01148 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.
See, this allows us use kdoc to document his nicely. Do that with the kernel pread stuff.
@@ -68,6 +71,8 @@ struct fw_priv { void *data; size_t size; size_t allocated_size;
- size_t offset;
- unsigned int flags;
But flags is a misnomer, you just do two operations, just juse an enum here to classify the read operation.
index 76f79913916d..4552b7bb819f 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -167,7 +167,8 @@ 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, unsigned int flags)
And likewise just use an enum here too.
@@ -210,9 +213,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, enum fw_opt opt_flags)
size_t size, enum fw_opt opt_flags,size_t offset)
flags? But its a single variable enum!
{ struct fw_priv *tmp;
- unsigned int pread_flags;
spin_lock(&fwc->lock); if (!(opt_flags & FW_OPT_NOCACHE)) { @@ -226,7 +231,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_flags = KERNEL_PREAD_FLAG_PART;- else
pread_flags = KERNEL_PREAD_FLAG_WHOLE;- tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags);
One of the advantages of using an enum is that you can then use a switch here, and the compiler will warn if you haven't handled all the cases.
/* 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->flags, id);
And here you'd just pass the kernel enum.
You get the idea, I stopped reviewing after this.
Luis
Hi Luis,
Another question.
On 2020-05-12 5:33 p.m., Luis Chamberlain wrote:
On Thu, May 07, 2020 at 05:27:34PM -0700, Scott Branden wrote:
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 | 52 +++++++++++++++++-------- drivers/soc/qcom/mdt_loader.c | 7 +++- include/linux/firmware.h | 8 +++- lib/test_firmware.c | 4 +- 5 files changed, 55 insertions(+), 21 deletions(-)
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h index 25836a6afc9f..1147dae01148 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.
See, this allows us use kdoc to document his nicely. Do that with the kernel pread stuff.
@@ -68,6 +71,8 @@ struct fw_priv { void *data; size_t size; size_t allocated_size;
- size_t offset;
- unsigned int flags;
But flags is a misnomer, you just do two operations, just juse an enum here to classify the read operation.
index 76f79913916d..4552b7bb819f 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -167,7 +167,8 @@ 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, unsigned int flags)And likewise just use an enum here too.
@@ -210,9 +213,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, enum fw_opt opt_flags)
size_t size, enum fw_opt opt_flags,size_t offset)flags? But its a single variable enum!
fw_opt is an existing enum which doesn't really act like an enum. It is a series of BIT defines in an enum that are then OR'd together in the (existing) code?
{ struct fw_priv *tmp;
- unsigned int pread_flags;
spin_lock(&fwc->lock); if (!(opt_flags & FW_OPT_NOCACHE)) {
Have a look here - enum used as series of flags.
@@ -226,7 +231,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_flags = KERNEL_PREAD_FLAG_PART;- else
pread_flags = KERNEL_PREAD_FLAG_WHOLE;- tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags);
One of the advantages of using an enum is that you can then use a switch here, and the compiler will warn if you haven't handled all the cases.
pread_flags is currently such. I will change to enum pread_opt.
/* 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->flags, id);And here you'd just pass the kernel enum.
Yes, will change to fw_priv->opt and use enum for this one.
You get the idea, I stopped reviewing after this.
Luis
On Wed, May 13, 2020 at 11:35:06AM -0700, Scott Branden wrote:
On 2020-05-12 5:33 p.m., Luis Chamberlain wrote:
On Thu, May 07, 2020 at 05:27:34PM -0700, Scott Branden wrote: flags? But its a single variable enum!
fw_opt is an existing enum which doesn't really act like an enum. It is a series of BIT defines in an enum that are then OR'd together in the (existing) code?
Indeed, in retrospect that is odd, it should be a u32 then. Please feel free to fix.
Luis
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 55176b0a8318..d1d2b48ed40e 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 flag value 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; + unsigned int pread_flags;
test_buf = kzalloc(TEST_FIRMWARE_BUF_SIZE, GFP_KERNEL); if (!test_buf) return -ENOSPC;
+ if (test_fw_config->partial) + pread_flags = KERNEL_PREAD_FLAG_PART; + else + pread_flags = KERNEL_PREAD_FLAG_WHOLE; + req->rc = request_firmware_into_buf(&req->fw, req->name, req->dev, test_buf, - TEST_FIRMWARE_BUF_SIZE, - 0, - KERNEL_PREAD_FLAG_WHOLE); + test_fw_config->buf_size, + test_fw_config->file_offset, + pread_flags); 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),
On Thu, May 07, 2020 at 05:27:35PM -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
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 55176b0a8318..d1d2b48ed40e 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -645,18 +762,24 @@ static int test_fw_run_batch_request(void *data) if (test_fw_config->into_buf) { void *test_buf;
unsigned int pread_flags;
Same thing here.
Luis
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
Add user space api for bcm-vk driver.
Signed-off-by: Scott Branden scott.branden@broadcom.com --- include/uapi/linux/misc/bcm_vk.h | 99 ++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 include/uapi/linux/misc/bcm_vk.h
diff --git a/include/uapi/linux/misc/bcm_vk.h b/include/uapi/linux/misc/bcm_vk.h new file mode 100644 index 000000000000..783087b7c31f --- /dev/null +++ b/include/uapi/linux/misc/bcm_vk.h @@ -0,0 +1,99 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */ +/* + * Copyright 2018-2020 Broadcom. + */ + +#ifndef __UAPI_LINUX_MISC_BCM_VK_H +#define __UAPI_LINUX_MISC_BCM_VK_H + +#include <linux/ioctl.h> +#include <linux/types.h> + +#define BCM_VK_MAX_FILENAME 64 + +struct vk_image { + __u32 type; /* Type of image */ +#define VK_IMAGE_TYPE_BOOT1 1 /* 1st stage (load to SRAM) */ +#define VK_IMAGE_TYPE_BOOT2 2 /* 2nd stage (load to DDR) */ + char filename[BCM_VK_MAX_FILENAME]; /* Filename of image */ +}; + +struct vk_reset { + __u32 arg1; + __u32 arg2; +}; + +#define VK_MAGIC 0x5e + +/* Load image to Valkyrie */ +#define VK_IOCTL_LOAD_IMAGE _IOW(VK_MAGIC, 0x2, struct vk_image) + +/* Send Reset to Valkyrie */ +#define VK_IOCTL_RESET _IOW(VK_MAGIC, 0x4, struct vk_reset) + +/* + * message block - basic unit in the message where a message's size is always + * N x sizeof(basic_block) + */ +struct vk_msg_blk { + __u8 function_id; +#define VK_FID_TRANS_BUF 5 +#define VK_FID_SHUTDOWN 8 + __u8 size; + __u16 trans_id; /* transport id, queue & msg_id */ + __u32 context_id; + __u32 args[2]; +#define VK_CMD_PLANES_MASK 0x000f /* number of planes to up/download */ +#define VK_CMD_UPLOAD 0x0400 /* memory transfer to vk */ +#define VK_CMD_DOWNLOAD 0x0500 /* memory transfer from vk */ +#define VK_CMD_MASK 0x0f00 /* command mask */ +}; + +#define VK_BAR_FWSTS 0x41c +#define VK_BAR_COP_FWSTS 0x428 +/* VK_FWSTS definitions */ +#define VK_FWSTS_RELOCATION_ENTRY BIT(0) +#define VK_FWSTS_RELOCATION_EXIT BIT(1) +#define VK_FWSTS_INIT_START BIT(2) +#define VK_FWSTS_ARCH_INIT_DONE BIT(3) +#define VK_FWSTS_PRE_KNL1_INIT_DONE BIT(4) +#define VK_FWSTS_PRE_KNL2_INIT_DONE BIT(5) +#define VK_FWSTS_POST_KNL_INIT_DONE BIT(6) +#define VK_FWSTS_INIT_DONE BIT(7) +#define VK_FWSTS_APP_INIT_START BIT(8) +#define VK_FWSTS_APP_INIT_DONE BIT(9) +#define VK_FWSTS_MASK 0xffffffff +#define VK_FWSTS_READY (VK_FWSTS_INIT_START | \ + VK_FWSTS_ARCH_INIT_DONE | \ + VK_FWSTS_PRE_KNL1_INIT_DONE | \ + VK_FWSTS_PRE_KNL2_INIT_DONE | \ + VK_FWSTS_POST_KNL_INIT_DONE | \ + VK_FWSTS_INIT_DONE | \ + VK_FWSTS_APP_INIT_START | \ + VK_FWSTS_APP_INIT_DONE) +/* Deinit */ +#define VK_FWSTS_APP_DEINIT_START BIT(23) +#define VK_FWSTS_APP_DEINIT_DONE BIT(24) +#define VK_FWSTS_DRV_DEINIT_START BIT(25) +#define VK_FWSTS_DRV_DEINIT_DONE BIT(26) +#define VK_FWSTS_RESET_DONE BIT(27) +#define VK_FWSTS_DEINIT_TRIGGERED (VK_FWSTS_APP_DEINIT_START | \ + VK_FWSTS_APP_DEINIT_DONE | \ + VK_FWSTS_DRV_DEINIT_START | \ + VK_FWSTS_DRV_DEINIT_DONE) +/* Last nibble for reboot reason */ +#define VK_FWSTS_RESET_REASON_SHIFT 28 +#define VK_FWSTS_RESET_REASON_MASK (0xf << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_SYS_PWRUP (0x0 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_MBOX_DB (0x1 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_M7_WDOG (0x2 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_TEMP (0x3 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_FLR (0x4 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_HOT (0x5 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_WARM (0x6 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_PCI_COLD (0x7 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_L1 (0x8 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_L0 (0x9 << VK_FWSTS_RESET_REASON_SHIFT) +#define VK_FWSTS_RESET_UNKNOWN (0xf << VK_FWSTS_RESET_REASON_SHIFT) + +#endif /* __UAPI_LINUX_MISC_BCM_VK_H */
Add maintainer entry for new Broadcom VK Driver
Signed-off-by: Scott Branden scott.branden@broadcom.com --- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS index 88bf36ab2b22..63eec54250f0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3612,6 +3612,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
Hi Scott,
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
Please consider this version series ready for upstream acceptance.
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.
Up to now, the firmware blob was read into memory allowing IMA to verify the file signature. With this change, ima_post_read_file() will not be able to verify the file signature.
(I don't think any of the other LSMs are on this hook, but you might want to Cc the LSM or integrity mailing list.)
Mimi
On Wed, May 13, 2020 at 12:23:59PM -0400, Mimi Zohar wrote:
Hi Scott,
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
Please consider this version series ready for upstream acceptance.
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.
Up to now, the firmware blob was read into memory allowing IMA to verify the file signature. With this change, ima_post_read_file() will not be able to verify the file signature.
(I don't think any of the other LSMs are on this hook, but you might want to Cc the LSM or integrity mailing list.)
Scott, so it sounds we need a resolution for pread for IMA for file signature verification. It seems that can be addressed though. Feel free to submit the u32 flag changes which you picked up on though in the meantime.
Luis
Hi Luis,
On 2020-05-15 1:47 p.m., Luis Chamberlain wrote:
On Wed, May 13, 2020 at 12:23:59PM -0400, Mimi Zohar wrote:
Hi Scott,
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
Please consider this version series ready for upstream acceptance.
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.
Up to now, the firmware blob was read into memory allowing IMA to verify the file signature. With this change, ima_post_read_file() will not be able to verify the file signature.
(I don't think any of the other LSMs are on this hook, but you might want to Cc the LSM or integrity mailing list.)
Scott, so it sounds we need a resolution for pread for IMA for file signature verification. It seems that can be addressed though. Feel free to submit the u32 flag changes which you picked up on though in the meantime.
Sure, I can submit a new patch to change the existing enum to a u32. For the new pread flags I am adding I could also leave as a u32 or change from a u32 to an enum since there is currently only one flag in use. Then, in the future if another flag was added we would need to change it back to a u32.
Please let me know what you prefer for the pread_flags.
Luis
Thanks, Scott
On Fri, May 15, 2020 at 04:28:08PM -0700, Scott Branden wrote:
Hi Luis,
On 2020-05-15 1:47 p.m., Luis Chamberlain wrote:
On Wed, May 13, 2020 at 12:23:59PM -0400, Mimi Zohar wrote:
Hi Scott,
On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
Please consider this version series ready for upstream acceptance.
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.
Up to now, the firmware blob was read into memory allowing IMA to verify the file signature. With this change, ima_post_read_file() will not be able to verify the file signature.
(I don't think any of the other LSMs are on this hook, but you might want to Cc the LSM or integrity mailing list.)
Scott, so it sounds we need a resolution for pread for IMA for file signature verification. It seems that can be addressed though. Feel free to submit the u32 flag changes which you picked up on though in the meantime.
Sure, I can submit a new patch to change the existing enum to a u32.
Great thanks!
For the new pread flags I am adding I could also leave as a u32 or change from a u32 to an enum since there is currently only one flag in use. Then, in the future if another flag was added we would need to change it back to a u32.
Yes that approach works well, enum for now.
Luis
linux-kselftest-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Kees Cook -
Luis Chamberlain -
Mimi Zohar -
Scott Branden