From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-06 23:23:33
This patch series adds partial read support via a new call
request_partial_firmware_into_buf.
Such support is needed when the whole file is not needed and/or
only a smaller portion of the file will fit into allocated memory
at any one time.
In order to accept the enhanced API it has been requested that kernel
selftests and upstreamed driver utilize the API enhancement and so
are included in this patch series.
Also in this patch series is the addition of a new Broadcom VK driver
utilizing the new request_firmware_into_buf enhanced API.
Further comment followed to add IMA support of the partial reads
originating from request_firmware_into_buf calls. And another request
to move existing kernel_read_file* functions to its own include file.
Changes from v9:
- add patch to move existing kernel_read_file* to its own include file
- driver fixes
Changes from v8:
- correct compilation error when CONFIG_FW_LOADER not defined
Changes from v7:
- removed swiss army knife kernel_pread_* style approach
and simply add offset parameter in addition to those needed
in kernel_read_* functions thus removing need for kernel_pread enum
Changes from v6:
- update ima_post_read_file check on IMA_FIRMWARE_PARTIAL_READ
- adjust new driver i2c-slave-eeprom.c use of request_firmware_into_buf
- remove an extern
Changes from v5:
- add IMA FIRMWARE_PARTIAL_READ support
- change kernel pread flags to enum
- removed legacy support from driver
- driver fixes
Changes from v4:
- handle reset issues if card crashes
- allow driver to have min required msix
- add card utilization information
Changes from v3:
- fix sparse warnings
- fix printf format specifiers for size_t
- fix 32-bit cross-compiling reports 32-bit shifts
- use readl/writel,_relaxed to access pci ioremap memory,
removed memory barriers and volatile keyword with such change
- driver optimizations for interrupt/poll functionalities
Changes from v2:
- remove unnecessary code and mutex locks in lib/test_firmware.c
- remove VK_IOCTL_ACCESS_BAR support from driver and use pci sysfs instead
- remove bitfields
- remove Kconfig default m
- adjust formatting and some naming based on feedback
- fix error handling conditions
- use appropriate return codes
- use memcpy_toio instead of direct access to PCIE bar
Scott Branden (9):
fs: move kernel_read_file* to its own include file
fs: introduce kernel_pread_file* support
firmware: add request_partial_firmware_into_buf
test_firmware: add partial read support for request_firmware_into_buf
firmware: test partial file reads of request_partial_firmware_into_buf
bcm-vk: add bcm_vk UAPI
misc: bcm-vk: add Broadcom VK driver
MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver
ima: add FIRMWARE_PARTIAL_READ support
MAINTAINERS | 7 +
drivers/base/firmware_loader/firmware.h | 5 +
drivers/base/firmware_loader/main.c | 80 +-
drivers/misc/Kconfig | 1 +
drivers/misc/Makefile | 1 +
drivers/misc/bcm-vk/Kconfig | 29 +
drivers/misc/bcm-vk/Makefile | 11 +
drivers/misc/bcm-vk/bcm_vk.h | 419 +++++
drivers/misc/bcm-vk/bcm_vk_dev.c | 1357 +++++++++++++++
drivers/misc/bcm-vk/bcm_vk_msg.c | 1504 +++++++++++++++++
drivers/misc/bcm-vk/bcm_vk_msg.h | 211 +++
drivers/misc/bcm-vk/bcm_vk_sg.c | 275 +++
drivers/misc/bcm-vk/bcm_vk_sg.h | 61 +
drivers/misc/bcm-vk/bcm_vk_tty.c | 352 ++++
fs/exec.c | 92 +-
include/linux/firmware.h | 12 +
include/linux/fs.h | 39 -
include/linux/ima.h | 1 +
include/linux/kernel_read_file.h | 69 +
include/linux/security.h | 1 +
include/uapi/linux/misc/bcm_vk.h | 99 ++
kernel/kexec_file.c | 1 +
kernel/module.c | 1 +
lib/test_firmware.c | 154 +-
security/integrity/digsig.c | 1 +
security/integrity/ima/ima_fs.c | 1 +
security/integrity/ima/ima_main.c | 25 +-
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 1 +
security/security.c | 1 +
security/selinux/hooks.c | 1 +
.../selftests/firmware/fw_filesystem.sh | 80 +
32 files changed, 4802 insertions(+), 91 deletions(-)
create mode 100644 drivers/misc/bcm-vk/Kconfig
create mode 100644 drivers/misc/bcm-vk/Makefile
create mode 100644 drivers/misc/bcm-vk/bcm_vk.h
create mode 100644 drivers/misc/bcm-vk/bcm_vk_dev.c
create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.c
create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.h
create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.c
create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.h
create mode 100644 drivers/misc/bcm-vk/bcm_vk_tty.c
create mode 100644 include/linux/kernel_read_file.h
create mode 100644 include/uapi/linux/misc/bcm_vk.h
--
2.17.1
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-06 23:23:49
Add kernel_pread_file* support to kernel to allow for partial read
of files with an offset into the file.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
fs/exec.c | 93 ++++++++++++++++++++++++--------
include/linux/kernel_read_file.h | 17 ++++++
2 files changed, 87 insertions(+), 23 deletions(-)
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-06 23:23:52
Add request_partial_firmware_into_buf to allow for portions
of firmware file to be read into a buffer. Necessary where firmware
needs to be loaded in portions from file in memory constrained systems.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
drivers/base/firmware_loader/firmware.h | 5 ++
drivers/base/firmware_loader/main.c | 79 +++++++++++++++++++------
include/linux/firmware.h | 12 ++++
3 files changed, 79 insertions(+), 17 deletions(-)
@@ -210,8 +215,11 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)/* Returns 1 for batching firmware requests with the same name */staticintalloc_lookup_fw_priv(constchar*fw_name,structfirmware_cache*fwc,-structfw_priv**fw_priv,void*dbuf,-size_tsize,u32opt_flags)+structfw_priv**fw_priv,+void*dbuf,+size_tsize,+size_toffset,+u32opt_flags){structfw_priv*tmp;
@@ -227,7 +235,7 @@ static int alloc_lookup_fw_priv(const char *fw_name,}}-tmp=__allocate_fw_priv(fw_name,fwc,dbuf,size);+tmp=__allocate_fw_priv(fw_name,fwc,dbuf,size,offset,opt_flags);if(tmp){INIT_LIST_HEAD(&tmp->list);if(!(opt_flags&FW_OPT_NOCACHE))
@@ -473,7 +481,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,/* Already populated data member means we're loading into a buffer */if(!decompress&&fw_priv->data){buffer=fw_priv->data;-id=READING_FIRMWARE_PREALLOC_BUFFER;+if(fw_priv->opt_flags&FW_OPT_PARTIAL)+id=READING_FIRMWARE_PARTIAL_READ;+else+id=READING_FIRMWARE_PREALLOC_BUFFER;+msize=fw_priv->allocated_size;}
@@ -496,8 +508,10 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,fw_priv->size=0;/* load firmware files from the mount namespace of init */-rc=kernel_read_file_from_path_initns(path,&buffer,-&size,msize,id);+rc=kernel_pread_file_from_path_initns(path,&buffer,+&size,msize,+fw_priv->offset,+id);if(rc){if(rc!=-ENOENT)dev_warn(device,"loading %s failed with error %d\n",
@@ -826,7 +840,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,/* Need to pin this module until return */__module_get(THIS_MODULE);-ret=_request_firmware(firmware_p,name,device,NULL,0,+ret=_request_firmware(firmware_p,name,device,NULL,0,0,FW_OPT_UEVENT);module_put(THIS_MODULE);returnret;
@@ -853,7 +867,7 @@ int firmware_request_nowarn(const struct firmware **firmware, const char *name,/* Need to pin this module until return */__module_get(THIS_MODULE);-ret=_request_firmware(firmware,name,device,NULL,0,+ret=_request_firmware(firmware,name,device,NULL,0,0,FW_OPT_UEVENT|FW_OPT_NO_WARN);module_put(THIS_MODULE);returnret;
@@ -877,7 +891,7 @@ int request_firmware_direct(const struct firmware **firmware_p,intret;__module_get(THIS_MODULE);-ret=_request_firmware(firmware_p,name,device,NULL,0,+ret=_request_firmware(firmware_p,name,device,NULL,0,0,FW_OPT_UEVENT|FW_OPT_NO_WARN|FW_OPT_NOFALLBACK_SYSFS);module_put(THIS_MODULE);
@@ -902,7 +916,7 @@ int firmware_request_platform(const struct firmware **firmware,/* Need to pin this module until return */__module_get(THIS_MODULE);-ret=_request_firmware(firmware,name,device,NULL,0,+ret=_request_firmware(firmware,name,device,NULL,0,0,FW_OPT_UEVENT|FW_OPT_FALLBACK_PLATFORM);module_put(THIS_MODULE);returnret;
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-06 23:24:03
Add additional hooks to test_firmware to pass in support
for partial file read using request_firmware_into_buf.
buf_size: size of buffer to request firmware into
partial: indicates that a partial file request is being made
file_offset: to indicate offset into file to request
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
lib/test_firmware.c | 154 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 142 insertions(+), 12 deletions(-)
@@ -315,6 +331,30 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)returnsnprintf(buf,PAGE_SIZE,"%d\n",val);}+staticinttest_dev_config_update_size_t(constchar*buf,+size_tsize,+size_t*cfg)+{+intret;+longnew;++ret=kstrtol(buf,10,&new);+if(ret)+returnret;++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 */+returnsize;+}++staticssize_ttest_dev_config_show_size_t(char*buf,size_tval)+{+returnsnprintf(buf,PAGE_SIZE,"%zu\n",val);+}+staticssize_ttest_dev_config_show_int(char*buf,intval){returnsnprintf(buf,PAGE_SIZE,"%d\n",val);
@@ -207,6 +227,35 @@ read_firmwares()done}+read_partial_firmwares()+{+if["$(cat$DIR/config_into_buf)"=="1"];then+fwfile="${FW_INTO_BUF}"+else+fwfile="${FW}"+fi++if["$1"="xzonly"];then+fwfile="${fwfile}-orig"+fi++# Strip fwfile down to match partial offset and length+partial_data="$(cat$fwfile)"+partial_data="${partial_data:$2:$3}"++foriin$(seq03);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+exit1+fi+done+}+ read_firmwares_expect_nofile(){foriin$(seq03);do
@@ -371,6 +435,22 @@ for i in $(seq 1 5); dotest_batched_request_firmware_into_buf$inormaldone+foriin$(seq15);do+test_batched_request_partial_firmware_into_buf$inormal010+done++foriin$(seq15);do+test_batched_request_partial_firmware_into_buf$inormal05+done++foriin$(seq15);do+test_batched_request_partial_firmware_into_buf$inormal16+done++foriin$(seq15);do+test_batched_request_partial_firmware_into_buf$inormal210+done+foriin$(seq15);dotest_batched_request_firmware_direct$inormaldone
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-06 23:24:14
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
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
Add FIRMWARE_PARTIAL_READ support for integrity
measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle
the "full read" semantics so that individual LSMs don't need to each
duplicate the same efforts. As it happens, only IMA is impacted (SELinux
ignores everything except modules, and LoadPin only cares about origin
not contents).
Next is the problem that enum kernel_read_file_id is an object
TYPE enum, not a HOW enum. (And it seems I missed the addition of
READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.)
That it's a partial read doesn't change _what_ you're reading: that's an
internal API detail. What happens when I attempt to do a partial read of
a kexec image? I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE,
but the LSMs will have no idea it's a partial read.
Finally, what keeps the contents of the file from changing between the
first call (which IMA will read the entire file for) and the next reads
which will bypass IMA? I'd suggested that the open file must have writes
disabled on it (as execve() does).
So, please redesign this:
- do not add an enum
- make the file unwritable for the life of having the handle open
- make the "full read" happen as part of the first partial read so the
LSMs don't have to reimplement everything
-Kees
--
Kees Cook
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-07 17:14:02
Hi Kees,
You and others are certainly more experts in the filesystem and security
infrastructure of the kernel.
What I am trying to accomplish is a simple operation:
request part of a file into a buffer rather than the whole file.
If someone could add such support I would be more than happy to use it.
This has now bubbled into many other designs issues in the existing
codebase.
I will need more details on your comments - see below.
On 2020-07-06 8:08 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
quoted
Add FIRMWARE_PARTIAL_READ support for integrity
measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle
the "full read" semantics so that individual LSMs don't need to each
duplicate the same efforts. As it happens, only IMA is impacted (SELinux
ignores everything except modules, and LoadPin only cares about origin
not contents).
Does your patch series "Fix misused kernel_read_file() enums" handle this
because this suggestion is outside the scope of my change?
Next is the problem that enum kernel_read_file_id is an object
TYPE enum, not a HOW enum. (And it seems I missed the addition of
READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.)
That it's a partial read doesn't change _what_ you're reading: that's an
internal API detail. What happens when I attempt to do a partial read of
a kexec image?
It does not appear there is any user of partial reads of kexec images?
I have been informed by Greg K-H to not add apis that are not used so
such support
doesn't make sense to add at this time.
I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE,
but the LSMs will have no idea it's a partial read.
The addition I am adding is for request_partial_firmware_into_buf.
In order to do so it adds internal support for partial reads of firmware
files,
not kexec image.
The above seems outside the scope of my patch?
Finally, what keeps the contents of the file from changing between the
first call (which IMA will read the entire file for) and the next reads
which will bypass IMA?
The request is for a partial read. IMA ensures the whole file integrity
even though I only do a partial read.
The next partial read will re-read and check integrity of file.
I'd suggested that the open file must have writes
disabled on it (as execve() does).
The file will be reopened and integrity checked on the next partial read
(if there is one).
So I don't think there is any change to be made here.
If writes aren't already disabled for a whole file read then that is
something that needs to be fixed in the existing code.
So, please redesign this:
- do not add an enum
I used existing infrastructure provided by Mimi but now looks like it
will have to fit with your patches from yesterday.
- make the file unwritable for the life of having the handle open
It's no different than a full file read so no change to be made here.
- make the "full read" happen as part of the first partial read so the
LSMs don't have to reimplement everything
Each partial read is an individual operation so I think a "full read" is
performed every time
if your security IMA is enabled. If someone wants to add a file lock
and then partial reads in the kernel
then that would be different than what is needed by the kernel driver.
On Tue, Jul 07, 2020 at 10:13:42AM -0700, Scott Branden wrote:
You and others are certainly more experts in the filesystem and security
infrastructure of the kernel.
What I am trying to accomplish is a simple operation:
request part of a file into a buffer rather than the whole file.
If someone could add such support I would be more than happy to use it.
Sure, and I totally understand that, but as it happens, no one has stepped
up with spare time to do that work. Since you're the person with the need
for the API, it falls to you to do it. And I understand what feature creep
feels like (I needed to fix one design problem[1] with timers, and I spent
months sending hundreds of patches). Some times you get lucky and it's
easy, and sometimes you end up touching something that needs a LOT of work
to refactor before you can make your desired change work well with the
rest of the kernel and be maintainable by other people into the future.
Quick tangent: I can't find in the many many threads where you explain
how large these firmware images are and why existing kernel memory
allocations are insufficient to load them. How large are these[2] files?
/lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin
/lib/firmware/vk-boot2-bcm958401m2_a72.ecdsa.bin
For me, the requirements for partial read support are these things,
which are the characteristics of the existing API:
- the LSM must be able to validate the entire file contents before
any data is available to any reader. (Which was pointed out back in
August 2019[3].) And "any" reader includes having a DMA window open
on the memory range used for reading the contents (which was pointed
out at by Mimi[4] but went unanswered and remains broken still in this
v10, but I will comment separately on that.)
- the integrity of the file contents must be maintained between
validation and delivery (currently this is handled internally via
disallow_writes()).
This has now bubbled into many other designs issues in the existing
codebase.
Correct -- this is one of the many difficulties of contributing to a
large and complex code base with many maintainers. There can be a lot
of requirements for the code that have nothing to do with seemingly more
narrow areas of endeavour.
I will need more details on your comments - see below.
On 2020-07-06 8:08 p.m., Kees Cook wrote:
quoted
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
quoted
Add FIRMWARE_PARTIAL_READ support for integrity
measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle
the "full read" semantics so that individual LSMs don't need to each
duplicate the same efforts. As it happens, only IMA is impacted (SELinux
ignores everything except modules, and LoadPin only cares about origin
not contents).
Does your patch series "Fix misused kernel_read_file() enums" handle this
because this suggestion is outside the scope of my change?
My proposed patch series cleans up a number of mistakes that were made
to the kernel_read_file() API, and helps clarify my point about the
enums being used for *what*, and not *how* or *where*, which needs to
be fixed in this series and shouldn't be a big deal (I will reply to
individual patches).
quoted
Next is the problem that enum kernel_read_file_id is an object
TYPE enum, not a HOW enum. (And it seems I missed the addition of
READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.)
That it's a partial read doesn't change _what_ you're reading: that's an
internal API detail. What happens when I attempt to do a partial read of
a kexec image?
It does not appear there is any user of partial reads of kexec images?
I have been informed by Greg K-H to not add apis that are not used so such
support doesn't make sense to add at this time.
But you are proposing a generic API enhancement that any other user in
the kernel may end up using. Just because the bcm-vk driver is the only
user now, and IMA is the only LSM performing content analysis, it
doesn't mean that there won't be another driver added later, nor another
LSM. In fact, the new BPF LSM means that anything exposed by LSM hooks
is now available for analysis.
quoted
I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE,
but the LSMs will have no idea it's a partial read.
The addition I am adding is for request_partial_firmware_into_buf.
In order to do so it adds internal support for partial reads of firmware
files,
not kexec image.
Yes, but you're changing kernel_read_file() APIs to do it. There are
plenty of users of that API. Maybe they would like to also use partial
reads?
$ git grep kernel_read_file | wc -l
77
The above seems outside the scope of my patch?
Unfortunately, it is not. Part of your responsibility as a kernel
developer making API changes/additions is that those changes need to
interact correctly with the rest of the kernel (and be maintainable).
quoted
Finally, what keeps the contents of the file from changing between the
first call (which IMA will read the entire file for) and the next reads
which will bypass IMA?
The request is for a partial read. IMA ensures the whole file integrity
even though I only do a partial read.
The next partial read will re-read and check integrity of file.
So, while terribly inefficient, I guess this approach is tenable. It
means that each partial read will trigger a full read for LSMs that care
about the hook.
So, to that end, I wonder why IMA doesn't do this for all file types?
It also means that we won't have a strict pairing of
security_kernel_read_file() to security_kernel_post_read_file() in the
LSMs, but it seems that only IMA currently explicitly cares about this
(or maybe not at all).
I'm not entirely happy about using this design, but it does appear
sufficient.
quoted
I'd suggested that the open file must have writes
disabled on it (as execve() does).
The file will be reopened and integrity checked on the next partial read (if
there is one).
So I don't think there is any change to be made here.
If writes aren't already disabled for a whole file read then that is
something that needs to be fixed in the existing code.
My suggestion quoted here was operating on the idea that whole-file
verification wasn't happening on every partial read, so this isn't a
problem in that case.
quoted
So, please redesign this:
- do not add an enum
I used existing infrastructure provided by Mimi but now looks like it will
have to fit with your patches from yesterday.
Right, this won't be hard. I will send a v2 of my patches to clarify the
purpose of the 3 file content hooks (load_data, read_file,
post_read_file), which might need renaming...
quoted
- make the file unwritable for the life of having the handle open
It's no different than a full file read so no change to be made here.
Correct.
quoted
- make the "full read" happen as part of the first partial read so the
LSMs don't have to reimplement everything
Each partial read is an individual operation so I think a "full read" is
performed every time
if your security IMA is enabled. If someone wants to add a file lock and
then partial reads in the kernel
then that would be different than what is needed by the kernel driver.
On Mon, Jul 06, 2020 at 04:23:01PM -0700, Scott Branden wrote:
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
include file. That header gets pulled in just about everywhere
and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/base/firmware_loader/main.c | 1 +
fs/exec.c | 1 +
include/linux/fs.h | 39 ----------------------
include/linux/ima.h | 1 +
include/linux/kernel_read_file.h | 52 +++++++++++++++++++++++++++++
include/linux/security.h | 1 +
kernel/kexec_file.c | 1 +
kernel/module.c | 1 +
security/integrity/digsig.c | 1 +
security/integrity/ima/ima_fs.c | 1 +
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 1 +
security/security.c | 1 +
security/selinux/hooks.c | 1 +
15 files changed, 65 insertions(+), 39 deletions(-)
create mode 100644 include/linux/kernel_read_file.h
This looks like too many files are getting touched. If it got added to
security.h, very few of the above .c files will need it explicitly
added (maybe none). You can test future versions of this change with an
allmodconfig build and make sure you have a matching .o for each .c
file that calls kernel_read_file(). :)
But otherwise, sure, seems good.
--
Kees Cook
On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
quoted hunk
Add kernel_pread_file* support to kernel to allow for partial read
of files with an offset into the file.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
fs/exec.c | 93 ++++++++++++++++++++++++--------
include/linux/kernel_read_file.h | 17 ++++++
2 files changed, 87 insertions(+), 23 deletions(-)
@@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,ret=-EINVAL;gotoout;}-if(i_size>SIZE_MAX||(max_size>0&&i_size>max_size)){++/* Default read to end of file */+read_end=i_size;++/* Allow reading partial portion of file */+if((id==READING_FIRMWARE_PARTIAL_READ)&&+(i_size>(pos+max_size)))+read_end=pos+max_size;
There's no need to involve "id" here. There are other signals about
what's happening (i.e. pos != 0, max_size != i_size, etc).
+
+ alloc_size = read_end - pos;
+ if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) {
ret = -EFBIG;
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+ (id != READING_FIRMWARE_PREALLOC_BUFFER))
+ *buf = vmalloc(alloc_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
}
The id usage here was a mistake in upstream, and the series I sent is
trying to clean that up.
Greg, it seems this series is going to end up in your tree due to it
being drivers/misc? I guess I need to direct my series to Greg then, but
get LSM folks Acks.
@@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, if (bytes == 0) break;++ buf_pos += bytes; }- if (pos != i_size) {+ if (pos != read_end) { ret = -EIO; goto out_free; }- ret = security_kernel_post_read_file(file, *buf, i_size, id);+ ret = security_kernel_post_read_file(file, *buf, alloc_size, id); if (!ret) *size = pos;
This call cannot be inside kernel_pread_file(): any future LSMs will see
a moving window of contents, etc. It'll need to be in kernel_read_file()
proper.
quoted hunk
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;
}
For each of these execution path, the mapping to LSM hooks is:
- all path must call security_kernel_read_file(file, id) before reading
(this appears to be fine as-is in your series).
- anything doing a "full" read needs to call
security_kernel_post_read_file() with the file and full buffer, size,
etc (so all the kernel_read_file*() paths). I imagine instead of
adding 3 copy/pasted versions of this, it may be possible to refactor
the helpers into a single core "full" caller that takes struct file,
or doing some logic in kernel_pread_file() that notices it has the
entire file in the buffer and doing the call then.
As an example of what I mean about doing the call, here's how I might
imagine it for one of the paths if it took struct file:
int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)
{
int ret;
ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id);
if (ret)
return ret;
return security_kernel_post_read_file(file, buf, *size, id);
}
On Mon, Jul 06, 2020 at 04:23:03PM -0700, Scott Branden wrote:
Add request_partial_firmware_into_buf to allow for portions
of firmware file to be read into a buffer. Necessary where firmware
needs to be loaded in portions from file in memory constrained systems.
Just tear out the differing "id" and just use FW_OPT_PARTIAL and I think
if Luis is happy, you're all set.
--
Kees Cook
On Mon, Jul 06, 2020 at 04:23:04PM -0700, Scott Branden wrote:
Add additional hooks to test_firmware to pass in support
for partial file read using request_firmware_into_buf.
buf_size: size of buffer to request firmware into
partial: indicates that a partial file request is being made
file_offset: to indicate offset into file to request
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
I am a fan of tests. :) If Luis gives an Ack here, you're good.
--
Kees Cook
ret = -EINVAL;
goto out;
}
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+
+ /* Default read to end of file */
+ read_end = i_size;
+
+ /* Allow reading partial portion of file */
+ if ((id == READING_FIRMWARE_PARTIAL_READ) &&
+ (i_size > (pos + max_size)))
+ read_end = pos + max_size;
There's no need to involve "id" here. There are other signals about
what's happening (i.e. pos != 0, max_size != i_size, etc).
Both the pre and post security kernel_read_file hooks are called here,
but there isn't enough information being passed to the LSM/IMA to be
able to different which hook is applicable. One method of providing
that additional information is by enumeration. The other option would
be to pass some additional information.
For example, on the post kernel_read_file hook, the file is read once
into memory. IMA calculates the firmware file hash based on the
buffer contents. On the pre kernel_read_file hook, IMA would need to
read the entire file, calculating the file hash. Both methods of
calculating the file hash work, but the post hook is more efficient.
Mimi
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-08 03:35:59
On 2020-07-07 4:36 p.m., Kees Cook wrote:
On Tue, Jul 07, 2020 at 10:13:42AM -0700, Scott Branden wrote:
quoted
You and others are certainly more experts in the filesystem and security
infrastructure of the kernel.
What I am trying to accomplish is a simple operation:
request part of a file into a buffer rather than the whole file.
If someone could add such support I would be more than happy to use it.
Sure, and I totally understand that, but as it happens, no one has stepped
up with spare time to do that work. Since you're the person with the need
for the API, it falls to you to do it. And I understand what feature creep
feels like (I needed to fix one design problem[1] with timers, and I spent
months sending hundreds of patches). Some times you get lucky and it's
easy, and sometimes you end up touching something that needs a LOT of work
to refactor before you can make your desired change work well with the
rest of the kernel and be maintainable by other people into the future.
Quick tangent: I can't find in the many many threads where you explain
how large these firmware images are and why existing kernel memory
allocations are insufficient to load them. How large are these[2] files?
/lib/firmware/vk-boot1-bcm958401m2.ecdsa.bin
This is on the order of a few MB at most.
/lib/firmware/vk-boot2-bcm958401m2_a72.ecdsa.bin
Some of these images are current 250MB. At we anticipate them growing
to 512MB.
And, we do have systems with the driver loading 16 cards in parallel
with no requirement that they are the same images
(although loading 16 different images to 16 different cards would be
strange but possible).
For me, the requirements for partial read support are these things,
which are the characteristics of the existing API:
- the LSM must be able to validate the entire file contents before
any data is available to any reader. (Which was pointed out back in
August 2019[3].)
I thought this was addressed in patch v6 "ima: aad FIRMWARE_PARTIAL_READ
support"
https://lkml.org/lkml/2020/6/5/1126
(although implementation not to your liking in current review)?
And "any" reader includes having a DMA window open
on the memory range used for reading the contents (which was pointed
out at by Mimi[4] but went unanswered and remains broken still in this
v10, but I will comment separately on that.)
- the integrity of the file contents must be maintained between
validation and delivery (currently this is handled internally via
disallow_writes()).
I don't understand what you are staying here: I am request a partial
firmware read into a buf.
At the time the partial firmware is read into a buf it is validated by
the security module if such integrity checks are enabled.
If, at another time I wish to request another partial firmware into a
buffer (could be the same part of the file or a different part of a file
or from another file), the integrity check is performed again and the
portion of the file I request should be put into the buffer.
If a lock on a file is needed by somebody between these partial reads
that is a different API and out of the scope of my patch series.
quoted
This has now bubbled into many other designs issues in the existing
codebase.
Correct -- this is one of the many difficulties of contributing to a
large and complex code base with many maintainers. There can be a lot
of requirements for the code that have nothing to do with seemingly more
narrow areas of endeavour.
Thanks at least for helping with guidance, I see your review is thought
out and
hopefully we can come to a conclusion as I feel we are fairly close with
your changes.
quoted
I will need more details on your comments - see below.
On 2020-07-06 8:08 p.m., Kees Cook wrote:
quoted
On Mon, Jul 06, 2020 at 04:23:09PM -0700, Scott Branden wrote:
quoted
Add FIRMWARE_PARTIAL_READ support for integrity
measurement on partial reads of firmware files.
Hi,
Several versions ago I'd suggested that the LSM infrastructure handle
the "full read" semantics so that individual LSMs don't need to each
duplicate the same efforts. As it happens, only IMA is impacted (SELinux
ignores everything except modules, and LoadPin only cares about origin
not contents).
Does your patch series "Fix misused kernel_read_file() enums" handle this
because this suggestion is outside the scope of my change?
My proposed patch series cleans up a number of mistakes that were made
to the kernel_read_file() API, and helps clarify my point about the
enums being used for *what*, and not *how* or *where*, which needs to
be fixed in this series and shouldn't be a big deal (I will reply to
individual patches).
quoted
quoted
Next is the problem that enum kernel_read_file_id is an object
TYPE enum, not a HOW enum. (And it seems I missed the addition of
READING_FIRMWARE_PREALLOC_BUFFER, which may share a similar problem.)
That it's a partial read doesn't change _what_ you're reading: that's an
internal API detail. What happens when I attempt to do a partial read of
a kexec image?
It does not appear there is any user of partial reads of kexec images?
I have been informed by Greg K-H to not add apis that are not used so such
support doesn't make sense to add at this time.
But you are proposing a generic API enhancement that any other user in
the kernel may end up using. Just because the bcm-vk driver is the only
user now, and IMA is the only LSM performing content analysis, it
doesn't mean that there won't be another driver added later, nor another
LSM. In fact, the new BPF LSM means that anything exposed by LSM hooks
is now available for analysis.
quoted
quoted
I'll use kernel_pread_file() and pass READING_KEXEC_IMAGE,
but the LSMs will have no idea it's a partial read.
The addition I am adding is for request_partial_firmware_into_buf.
In order to do so it adds internal support for partial reads of firmware
files,
not kexec image.
Yes, but you're changing kernel_read_file() APIs to do it. There are
plenty of users of that API. Maybe they would like to also use partial
reads?
$ git grep kernel_read_file | wc -l
77
quoted
The above seems outside the scope of my patch?
Unfortunately, it is not. Part of your responsibility as a kernel
developer making API changes/additions is that those changes need to
interact correctly with the rest of the kernel (and be maintainable).
quoted
quoted
Finally, what keeps the contents of the file from changing between the
first call (which IMA will read the entire file for) and the next reads
which will bypass IMA?
The request is for a partial read. IMA ensures the whole file integrity
even though I only do a partial read.
The next partial read will re-read and check integrity of file.
So, while terribly inefficient, I guess this approach is tenable. It
means that each partial read will trigger a full read for LSMs that care
about the hook.
So, to that end, I wonder why IMA doesn't do this for all file types?
It also means that we won't have a strict pairing of
security_kernel_read_file() to security_kernel_post_read_file() in the
LSMs, but it seems that only IMA currently explicitly cares about this
(or maybe not at all).
I'm not entirely happy about using this design, but it does appear
sufficient.
Yes, terribly inefficient, but if somebody wants to do some optimization
they are welcome to it.
In fact, I want to call a partial read of a file with NO security. Can I
add such a call instead?
If we did that now then the inefficient read of the file multiple times
to authenticate it each time wouldn't be introduced.
In my use case the linux kernel security on the file is quite
meaningless and a waste of time to even perform.
Whether the file has been compromised, corrupt or otherwise the image is
validated by the hardware after
the image is loaded to the card to ensure it passes authentication.
quoted
quoted
I'd suggested that the open file must have writes
disabled on it (as execve() does).
The file will be reopened and integrity checked on the next partial read (if
there is one).
So I don't think there is any change to be made here.
If writes aren't already disabled for a whole file read then that is
something that needs to be fixed in the existing code.
My suggestion quoted here was operating on the idea that whole-file
verification wasn't happening on every partial read, so this isn't a
problem in that case.
quoted
quoted
So, please redesign this:
- do not add an enum
I used existing infrastructure provided by Mimi but now looks like it will
have to fit with your patches from yesterday.
Right, this won't be hard. I will send a v2 of my patches to clarify the
purpose of the 3 file content hooks (load_data, read_file,
post_read_file), which might need renaming...
I see your cleanup and merged with it. Will need to test everything again.
quoted
quoted
- make the file unwritable for the life of having the handle open
It's no different than a full file read so no change to be made here.
Correct.
quoted
quoted
- make the "full read" happen as part of the first partial read so the
LSMs don't have to reimplement everything
Each partial read is an individual operation so I think a "full read" is
performed every time
if your security IMA is enabled. If someone wants to add a file lock and
then partial reads in the kernel
then that would be different than what is needed by the kernel driver.
So, given that Mimi is (I think?) satisfied with your approach here, I
can't realistically complain. I still don't like the idea of each LSM
needing to perform it's full read loop for the contents, but so be it:
IMA will have the code, SELinux doesn't care (yet), and LoadPin doesn't
care about contents.
I obviously am not aware of all of the security hooks and architecture
in place but I do see your cleanups as a simplification over what was there.
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-08 03:39:45
On 2020-07-07 4:40 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:01PM -0700, Scott Branden wrote:
quoted
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
include file. That header gets pulled in just about everywhere
and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/base/firmware_loader/main.c | 1 +
fs/exec.c | 1 +
include/linux/fs.h | 39 ----------------------
include/linux/ima.h | 1 +
include/linux/kernel_read_file.h | 52 +++++++++++++++++++++++++++++
include/linux/security.h | 1 +
kernel/kexec_file.c | 1 +
kernel/module.c | 1 +
security/integrity/digsig.c | 1 +
security/integrity/ima/ima_fs.c | 1 +
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 1 +
security/security.c | 1 +
security/selinux/hooks.c | 1 +
15 files changed, 65 insertions(+), 39 deletions(-)
create mode 100644 include/linux/kernel_read_file.h
This looks like too many files are getting touched. If it got added to
security.h, very few of the above .c files will need it explicitly
added (maybe none).
Some people want the header file added to each file that uses it,
others want it in a common header file. I tried to add it to each file
that uses it.
But if the other approach is to be followed that could be done.
You can test future versions of this change with an
allmodconfig build and make sure you have a matching .o for each .c
file that calls kernel_read_file(). :)
But otherwise, sure, seems good.
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-08 04:01:14
On 2020-07-07 4:56 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
quoted
Add kernel_pread_file* support to kernel to allow for partial read
of files with an offset into the file.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
fs/exec.c | 93 ++++++++++++++++++++++++--------
include/linux/kernel_read_file.h | 17 ++++++
2 files changed, 87 insertions(+), 23 deletions(-)
@@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,ret=-EINVAL;gotoout;}-if(i_size>SIZE_MAX||(max_size>0&&i_size>max_size)){++/* Default read to end of file */+read_end=i_size;++/* Allow reading partial portion of file */+if((id==READING_FIRMWARE_PARTIAL_READ)&&+(i_size>(pos+max_size)))+read_end=pos+max_size;
There's no need to involve "id" here. There are other signals about
what's happening (i.e. pos != 0, max_size != i_size, etc).
There are other signals other than the fact that kernel_read_file requires
the entire file to be read while kernel_pread_file allows partial files
to be read.
So if you do a pread at pos = 0 you need another key to indicate it is
"ok" if max_size < i_size.
If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to share
99% of the code
between kernel_read_file and kernel_pread_file then I need to add
another parameter to a common function
called between these functions. And adding another parameter was
rejected previously in the review as a "swiss army knife approach" by
another reviewer. I am happy to add it back in because it is necessary
to share code and differentiate whether we are performing a partial read
or not.
quoted
+
+ alloc_size = read_end - pos;
+ if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) {
ret = -EFBIG;
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+ (id != READING_FIRMWARE_PREALLOC_BUFFER))
+ *buf = vmalloc(alloc_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
}
The id usage here was a mistake in upstream, and the series I sent is
trying to clean that up.
I see that cleanup and it works fine with the pread. Other than I need
some sort of key to share code and indicate whether it is "ok" to do a
partial read of the file or not.
Greg, it seems this series is going to end up in your tree due to it
being drivers/misc? I guess I need to direct my series to Greg then, but
get LSM folks Acks.
@@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, if (bytes == 0) break;++ buf_pos += bytes; }- if (pos != i_size) {+ if (pos != read_end) { ret = -EIO; goto out_free; }- ret = security_kernel_post_read_file(file, *buf, i_size, id);+ ret = security_kernel_post_read_file(file, *buf, alloc_size, id); if (!ret) *size = pos;
This call cannot be inside kernel_pread_file(): any future LSMs will see
a moving window of contents, etc. It'll need to be in kernel_read_file()
proper.
If IMA still passes (after testing my next patch series with your
changes and my modifications)
I will need some more help here.
quoted
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;
}
For each of these execution path, the mapping to LSM hooks is:
- all path must call security_kernel_read_file(file, id) before reading
(this appears to be fine as-is in your series).
- anything doing a "full" read needs to call
security_kernel_post_read_file() with the file and full buffer, size,
etc (so all the kernel_read_file*() paths). I imagine instead of
adding 3 copy/pasted versions of this, it may be possible to refactor
the helpers into a single core "full" caller that takes struct file,
or doing some logic in kernel_pread_file() that notices it has the
entire file in the buffer and doing the call then.
As an example of what I mean about doing the call, here's how I might
imagine it for one of the paths if it took struct file:
int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)
{
int ret;
ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id);
if (ret)
return ret;
return security_kernel_post_read_file(file, buf, *size, id);
}
I remain concerned that adding these helpers will lead a poor
interaction with LSMs, but I guess I get to hold my tongue. :)
We could add pread functions that are "unsafe" in nature instead then?
As I certainly do not need any integrity checks on the file for my
driver. The real check is done by the card the data is loaded to
whether is passes the linux security checks or not.
And then, if someone does want to do something "safe" with preads
another kernel_read_file_securelock/unlock could be added for those that
need security for their partial reads?
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-08 04:07:16
On 2020-07-07 4:58 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:03PM -0700, Scott Branden wrote:
quoted
Add request_partial_firmware_into_buf to allow for portions
of firmware file to be read into a buffer. Necessary where firmware
needs to be loaded in portions from file in memory constrained systems.
Just tear out the differing "id" and just use FW_OPT_PARTIAL and I think
if Luis is happy, you're all set.
I hope so. Also, I will need to call
kernel_pread_file_from_path_initns() if FW_OPT_PARTIAL is set
and kernel_read_file_from_path_initns() otherwise to avoid a swiss
army-knife approach of calling a common function with multiple parameters.
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-08 04:10:02
On 2020-07-07 4:59 p.m., Kees Cook wrote:
On Mon, Jul 06, 2020 at 04:23:04PM -0700, Scott Branden wrote:
quoted
Add additional hooks to test_firmware to pass in support
for partial file read using request_firmware_into_buf.
buf_size: size of buffer to request firmware into
partial: indicates that a partial file request is being made
file_offset: to indicate offset into file to request
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
I am a fan of tests. :) If Luis gives an Ack here, you're good.
There were not even any tests for request_firmware_into_buf before I
started this partial read support.
Fortunately those base changes have already been accepted so I think
this change is a simple addition to those accepted patches.
This patch series adds partial read support via a new call
request_partial_firmware_into_buf.
Such support is needed when the whole file is not needed and/or
only a smaller portion of the file will fit into allocated memory
at any one time.
In order to accept the enhanced API it has been requested that kernel
selftests and upstreamed driver utilize the API enhancement and so
are included in this patch series.
Also in this patch series is the addition of a new Broadcom VK driver
utilizing the new request_firmware_into_buf enhanced API.
Further comment followed to add IMA support of the partial reads
originating from request_firmware_into_buf calls. And another request
to move existing kernel_read_file* functions to its own include file.
Do you have any way to separate the VK drivers submission from the
request_partial_firmware_into_buf() work that you are doing? It looks
like it is going to require quite a few iterations of this patch set for
the firmware/fs/IMA part to be ironed out, so if you could get your
driver separated out, it might help you achieve partial success here.
--
Florian
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-08 04:42:02
Hi Kees,
one more comment below.
On 2020-07-07 9:01 p.m., Scott Branden wrote:
On 2020-07-07 4:56 p.m., Kees Cook wrote:
quoted
On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:
quoted
Add kernel_pread_file* support to kernel to allow for partial read
of files with an offset into the file.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
fs/exec.c | 93
++++++++++++++++++++++++--------
include/linux/kernel_read_file.h | 17 ++++++
2 files changed, 87 insertions(+), 23 deletions(-)
There's no need to involve "id" here. There are other signals about
what's happening (i.e. pos != 0, max_size != i_size, etc).
There are other signals other than the fact that kernel_read_file
requires
the entire file to be read while kernel_pread_file allows partial
files to be read.
So if you do a pread at pos = 0 you need another key to indicate it is
"ok" if max_size < i_size.
If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to
share 99% of the code
between kernel_read_file and kernel_pread_file then I need to add
another parameter to a common function
called between these functions. And adding another parameter was
rejected previously in the review as a "swiss army knife approach" by
another reviewer. I am happy to add it back in because it is
necessary to share code and differentiate whether we are performing a
partial read or not.
quoted
quoted
+
+ alloc_size = read_end - pos;
+ if (i_size > SIZE_MAX || (max_size > 0 && alloc_size >
max_size)) {
ret = -EFBIG;
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+ (id != READING_FIRMWARE_PREALLOC_BUFFER))
+ *buf = vmalloc(alloc_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
}
The id usage here was a mistake in upstream, and the series I sent is
trying to clean that up.
I see that cleanup and it works fine with the pread. Other than I
need some sort of key to share code and indicate whether it is "ok" to
do a partial read of the file or not.
quoted
Greg, it seems this series is going to end up in your tree due to it
being drivers/misc? I guess I need to direct my series to Greg then, but
get LSM folks Acks.
@@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void
**buf, loff_t *size,
if (bytes == 0)
break;
+
+ buf_pos += bytes;
}
- if (pos != i_size) {
+ if (pos != read_end) {
ret = -EIO;
goto out_free;
}
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ ret = security_kernel_post_read_file(file, *buf, alloc_size, id);
if (!ret)
*size = pos;
This call cannot be inside kernel_pread_file(): any future LSMs will see
a moving window of contents, etc. It'll need to be in kernel_read_file()
proper.
If IMA still passes (after testing my next patch series with your
changes and my modifications)
I will need some more help here.
quoted
quoted
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+ (id != READING_FIRMWARE_PREALLOC_BUFFER)) {
vfree(*buf);
*buf = NULL;
}
@@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void
For each of these execution path, the mapping to LSM hooks is:
- all path must call security_kernel_read_file(file, id) before reading
(this appears to be fine as-is in your series).
- anything doing a "full" read needs to call
security_kernel_post_read_file() with the file and full buffer, size,
etc (so all the kernel_read_file*() paths). I imagine instead of
adding 3 copy/pasted versions of this, it may be possible to refactor
the helpers into a single core "full" caller that takes struct file,
or doing some logic in kernel_pread_file() that notices it has the
entire file in the buffer and doing the call then.
As an example of what I mean about doing the call, here's how I might
imagine it for one of the paths if it took struct file:
int kernel_read_file_from_file(struct file *file, void **buf, loff_t
*size,
loff_t max_size, enum kernel_read_file_id id)
{
int ret;
ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id);
if (ret)
return ret;
return security_kernel_post_read_file(file, buf, *size, id);
}
I remain concerned that adding these helpers will lead a poor
interaction with LSMs, but I guess I get to hold my tongue. :)
I only need kernel_pread_file and kernel_pread_file_from_path_initns.
kernel_pread_file_from_fd and kernel_pread_file_from_path were only
added for completeness.
And are really only helper functions called by their kernel_read_file*
counterparts at this time. So they can be removed from this patch if
that helps?
We could add pread functions that are "unsafe" in nature instead then?
As I certainly do not need any integrity checks on the file for my
driver. The real check is done by the card the data is loaded to
whether is passes the linux security checks or not.
And then, if someone does want to do something "safe" with preads
another kernel_read_file_securelock/unlock could be added for those
that need security for their partial reads?
From: Scott Branden <scott.branden@broadcom.com> Date: 2020-07-08 04:52:09
Hi Florian,
On 2020-07-07 9:38 p.m., Florian Fainelli wrote:
On 7/6/2020 4:23 PM, Scott Branden wrote:
quoted
This patch series adds partial read support via a new call
request_partial_firmware_into_buf.
Such support is needed when the whole file is not needed and/or
only a smaller portion of the file will fit into allocated memory
at any one time.
In order to accept the enhanced API it has been requested that kernel
selftests and upstreamed driver utilize the API enhancement and so
are included in this patch series.
Also in this patch series is the addition of a new Broadcom VK driver
utilizing the new request_firmware_into_buf enhanced API.
Further comment followed to add IMA support of the partial reads
originating from request_firmware_into_buf calls. And another request
to move existing kernel_read_file* functions to its own include file.
Do you have any way to separate the VK drivers submission from the
request_partial_firmware_into_buf() work that you are doing? It looks
like it is going to require quite a few iterations of this patch set for
the firmware/fs/IMA part to be ironed out, so if you could get your
driver separated out, it might help you achieve partial success here.
Originally I did not submit the driver.
But Greg K-H rejected the pread support unless there was an actual user
in the kernel.
Hence the need to submit this all in the patch series.