| Message ID | 20200707081926.3688096-3-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix misused kernel_read_file() enums | expand |
On 2020-07-07 1:19 a.m., Kees Cook wrote: > FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs > that are interested in filtering between types of things. The "how" > should be an internal detail made uninteresting to the LSMs. > > Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer") > Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)") > Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)") > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/base/firmware_loader/main.c | 5 ++--- > fs/exec.c | 7 ++++--- > include/linux/fs.h | 2 +- > security/integrity/ima/ima_main.c | 6 ++---- > 4 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c > index ca871b13524e..c2f57cedcd6f 100644 > --- a/drivers/base/firmware_loader/main.c > +++ b/drivers/base/firmware_loader/main.c > @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, > int i, len; > int rc = -ENOENT; > char *path; > - enum kernel_read_file_id id = READING_FIRMWARE; > size_t msize = INT_MAX; > void *buffer = NULL; > > /* 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; > msize = fw_priv->allocated_size; > } > > @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, > > /* load firmware files from the mount namespace of init */ > rc = kernel_read_file_from_path_initns(path, &buffer, > - &size, msize, id); > + &size, msize, > + READING_FIRMWARE); > if (rc) { > if (rc != -ENOENT) > dev_warn(device, "loading %s failed with error %d\n", > diff --git a/fs/exec.c b/fs/exec.c > index e6e8a9a70327..2bf549757ce7 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > { > loff_t i_size, pos; > ssize_t bytes = 0; > + void *allocated = NULL; > int ret; > > if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0) > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > goto out; > } > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) > - *buf = vmalloc(i_size); > + if (!*buf) > + *buf = allocated = vmalloc(i_size); > if (!*buf) { > ret = -ENOMEM; > goto out; > @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > > out_free: > if (ret < 0) { > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { > + if (allocated) { > vfree(*buf); > *buf = NULL; > } > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 3f881a892ea7..95fc775ed937 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode) > #endif > extern int do_pipe_flags(int *, int); > > +/* This is a list of *what* is being read, not *how*. */ > #define __kernel_read_file_id(id) \ > id(UNKNOWN, unknown) \ > id(FIRMWARE, firmware) \ With this change, I'm trying to figure out how the partial firmware read is going to work on top of this reachitecture. Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a "what"? > - id(FIRMWARE_PREALLOC_BUFFER, firmware) \ My patch series gets rejected any time I make a change to the kernel_read_file* region in linux/fs.h. The requirement is for this api to move to another header file outside of linux/fs.h It seems the same should apply to your change. Could you please add the following patch to the start of you patch series to move the kernel_read_file* to its own include file? https://patchwork.kernel.org/patch/11647063/ > id(FIRMWARE_EFI_EMBEDDED, firmware) \ > id(MODULE, kernel-module) \ > id(KEXEC_IMAGE, kexec-image) \ > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index c1583d98c5e5..f80ee4ce4669 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry) > int ima_read_file(struct file *file, enum kernel_read_file_id read_id) > { > /* > - * READING_FIRMWARE_PREALLOC_BUFFER > - * > * Do devices using pre-allocated memory run the risk of the > * firmware being accessible to the device prior to the completion > * of IMA's signature verification any more than when using two > - * buffers? > + * buffers? It may be desirable to include the buffer address > + * in this API and walk all the dma_map_single() mappings to check. > */ > return 0; > } > > const int read_idmap[READING_MAX_ID] = { > [READING_FIRMWARE] = FIRMWARE_CHECK, > - [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK, > [READING_MODULE] = MODULE_CHECK, > [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, > [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
On Tue, Jul 07, 2020 at 09:42:02AM -0700, Scott Branden wrote: > On 2020-07-07 1:19 a.m., Kees Cook wrote: > > FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs > > that are interested in filtering between types of things. The "how" > > should be an internal detail made uninteresting to the LSMs. > > > > Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer") > > Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)") > > Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)") > > [...] > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 3f881a892ea7..95fc775ed937 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode) > > #endif > > extern int do_pipe_flags(int *, int); > > +/* This is a list of *what* is being read, not *how*. */ > > #define __kernel_read_file_id(id) \ > > id(UNKNOWN, unknown) \ > > id(FIRMWARE, firmware) \ > With this change, I'm trying to figure out how the partial firmware read is > going to work on top of this reachitecture. > Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a > "what"? No, that's why I said you need to do the implementation within the API and not expect each LSM to implement their own (as I mentioned both times): https://lore.kernel.org/lkml/202005221551.5CA1372@keescook/ https://lore.kernel.org/lkml/202007061950.F6B3D9E6A@keescook/ I will reply in the thread above. > > - id(FIRMWARE_PREALLOC_BUFFER, firmware) \ > My patch series gets rejected any time I make a change to the > kernel_read_file* region in linux/fs.h. > The requirement is for this api to move to another header file outside of > linux/fs.h > It seems the same should apply to your change. Well I'm hardly making the same level of changes, but yeah, sure, if that helps move things along, I can include that here. > Could you please add the following patch to the start of you patch series to > move the kernel_read_file* to its own include file? > https://patchwork.kernel.org/patch/11647063/ https://lore.kernel.org/lkml/20200706232309.12010-2-scott.branden@broadcom.com/ You've included it in include/linux/security.h and that should be pretty comprehensive, it shouldn't be needed in so many .c files.
Hi Kees, Thanks for looking at my patch series to see how it relates. I see what you're trying to accomplish in various areas of cleanup. I'll comment as I go through your individual emails. 1 comment below. On 2020-07-07 2:55 p.m., Kees Cook wrote: > On Tue, Jul 07, 2020 at 09:42:02AM -0700, Scott Branden wrote: >> On 2020-07-07 1:19 a.m., Kees Cook wrote: >>> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs >>> that are interested in filtering between types of things. The "how" >>> should be an internal detail made uninteresting to the LSMs. >>> >>> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer") >>> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)") >>> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)") >>> [...] >>> diff --git a/include/linux/fs.h b/include/linux/fs.h >>> index 3f881a892ea7..95fc775ed937 100644 >>> --- a/include/linux/fs.h >>> +++ b/include/linux/fs.h >>> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode) >>> #endif >>> extern int do_pipe_flags(int *, int); >>> +/* This is a list of *what* is being read, not *how*. */ >>> #define __kernel_read_file_id(id) \ >>> id(UNKNOWN, unknown) \ >>> id(FIRMWARE, firmware) \ >> With this change, I'm trying to figure out how the partial firmware read is >> going to work on top of this reachitecture. >> Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a >> "what"? > No, that's why I said you need to do the implementation within the API > and not expect each LSM to implement their own (as I mentioned both > times): > > https://lore.kernel.org/lkml/202005221551.5CA1372@keescook/ > https://lore.kernel.org/lkml/202007061950.F6B3D9E6A@keescook/ > > I will reply in the thread above. > >>> - id(FIRMWARE_PREALLOC_BUFFER, firmware) \ >> My patch series gets rejected any time I make a change to the >> kernel_read_file* region in linux/fs.h. >> The requirement is for this api to move to another header file outside of >> linux/fs.h >> It seems the same should apply to your change. > Well I'm hardly making the same level of changes, but yeah, sure, if > that helps move things along, I can include that here. > >> Could you please add the following patch to the start of you patch series to >> move the kernel_read_file* to its own include file? >> https://patchwork.kernel.org/patch/11647063/ > https://lore.kernel.org/lkml/20200706232309.12010-2-scott.branden@broadcom.com/ > > You've included it in include/linux/security.h and that should be pretty > comprehensive, it shouldn't be needed in so many .c files. Some people want the header files included in each c file they are used. Others want header files not included if they are included in another header file. I chose the first approach: every file that uses the api includes the header file. I didn't know there was a standard approach to only put it in security.h >
On Tue, Jul 07, 2020 at 08:06:23PM -0700, Scott Branden wrote: > Some people want the header files included in each c file they are used. > Others want header files not included if they are included in another header > file. Ah, yes. That's fine then, leave them in the .c files.
Hi Kees, This patch fails during booting of my system - see below. On 2020-07-07 1:19 a.m., Kees Cook wrote: > FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs > that are interested in filtering between types of things. The "how" > should be an internal detail made uninteresting to the LSMs. > > Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer") > Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)") > Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)") > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/base/firmware_loader/main.c | 5 ++--- > fs/exec.c | 7 ++++--- > include/linux/fs.h | 2 +- > security/integrity/ima/ima_main.c | 6 ++---- > 4 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c > index ca871b13524e..c2f57cedcd6f 100644 > --- a/drivers/base/firmware_loader/main.c > +++ b/drivers/base/firmware_loader/main.c > @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, > int i, len; > int rc = -ENOENT; > char *path; > - enum kernel_read_file_id id = READING_FIRMWARE; > size_t msize = INT_MAX; > void *buffer = NULL; > > /* 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; > msize = fw_priv->allocated_size; > } > > @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, > > /* load firmware files from the mount namespace of init */ > rc = kernel_read_file_from_path_initns(path, &buffer, > - &size, msize, id); > + &size, msize, > + READING_FIRMWARE); > if (rc) { > if (rc != -ENOENT) > dev_warn(device, "loading %s failed with error %d\n", > diff --git a/fs/exec.c b/fs/exec.c > index e6e8a9a70327..2bf549757ce7 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > { > loff_t i_size, pos; > ssize_t bytes = 0; > + void *allocated = NULL; > int ret; > > if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0) > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > goto out; > } > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) > - *buf = vmalloc(i_size); > + if (!*buf) The assumption that *buf is always NULL when id != READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct. I get unhandled page faults due to this change on boot. > + *buf = allocated = vmalloc(i_size); > if (!*buf) { > ret = -ENOMEM; > goto out; > @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > > out_free: > if (ret < 0) { > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { > + if (allocated) { > vfree(*buf); > *buf = NULL; > } > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 3f881a892ea7..95fc775ed937 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode) > #endif > extern int do_pipe_flags(int *, int); > > +/* This is a list of *what* is being read, not *how*. */ > #define __kernel_read_file_id(id) \ > id(UNKNOWN, unknown) \ > id(FIRMWARE, firmware) \ > - id(FIRMWARE_PREALLOC_BUFFER, firmware) \ > id(FIRMWARE_EFI_EMBEDDED, firmware) \ > id(MODULE, kernel-module) \ > id(KEXEC_IMAGE, kexec-image) \ > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index c1583d98c5e5..f80ee4ce4669 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry) > int ima_read_file(struct file *file, enum kernel_read_file_id read_id) > { > /* > - * READING_FIRMWARE_PREALLOC_BUFFER > - * > * Do devices using pre-allocated memory run the risk of the > * firmware being accessible to the device prior to the completion > * of IMA's signature verification any more than when using two > - * buffers? > + * buffers? It may be desirable to include the buffer address > + * in this API and walk all the dma_map_single() mappings to check. > */ > return 0; > } > > const int read_idmap[READING_MAX_ID] = { > [READING_FIRMWARE] = FIRMWARE_CHECK, > - [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK, > [READING_MODULE] = MODULE_CHECK, > [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, > [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote: > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > > goto out; > > } > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) > > - *buf = vmalloc(i_size); > > + if (!*buf) > The assumption that *buf is always NULL when id != > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct. > I get unhandled page faults due to this change on boot. Did it give you a stack backtrace?
On 2020-07-10 3:04 p.m., Matthew Wilcox wrote: > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote: >>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, >>> goto out; >>> } >>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) >>> - *buf = vmalloc(i_size); >>> + if (!*buf) >> The assumption that *buf is always NULL when id != >> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct. >> I get unhandled page faults due to this change on boot. > Did it give you a stack backtrace? Yes, but there's no requirement that *buf need to be NULL when calling this function. To fix my particular crash I added the following locally: --- a/kernel/module.c +++ b/kernel/module.c @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) { struct load_info info = { }; loff_t size; - void *hdr; + void *hdr = NULL; int err; err = may_init_module(); >
On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote: > > > On 2020-07-10 3:04 p.m., Matthew Wilcox wrote: > > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote: > > > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > > > > goto out; > > > > } > > > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) > > > > - *buf = vmalloc(i_size); > > > > + if (!*buf) > > > The assumption that *buf is always NULL when id != > > > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct. > > > I get unhandled page faults due to this change on boot. > > Did it give you a stack backtrace? > Yes, but there's no requirement that *buf need to be NULL when calling this > function. > To fix my particular crash I added the following locally: > > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char > __user *, uargs, int, flags) > { > struct load_info info = { }; > loff_t size; > - void *hdr; > + void *hdr = NULL; > int err; > > err = may_init_module(); > > > Thanks for the diagnosis and fix! I haven't had time to cycle back around to this series yet. Hopefully soon. :)
Hi Kees, On 2020-07-10 3:44 p.m., Kees Cook wrote: > On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote: >> >> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote: >>> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote: >>>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, >>>>> goto out; >>>>> } >>>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) >>>>> - *buf = vmalloc(i_size); >>>>> + if (!*buf) >>>> The assumption that *buf is always NULL when id != >>>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct. >>>> I get unhandled page faults due to this change on boot. >>> Did it give you a stack backtrace? >> Yes, but there's no requirement that *buf need to be NULL when calling this >> function. >> To fix my particular crash I added the following locally: >> >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char >> __user *, uargs, int, flags) >> { >> struct load_info info = { }; >> loff_t size; >> - void *hdr; >> + void *hdr = NULL; >> int err; >> >> err = may_init_module(); > Thanks for the diagnosis and fix! I haven't had time to cycle back > around to this series yet. Hopefully soon. :) I don't consider this a complete fix as there may be other callers which do not initialize the *buf param to NULL before calling kernel_read_file. But, it does boot my system. Also, I was able to make modifications for my pread changes that pass (and the IMA works with IMA patch in my series is dropped completely with your changes in place). So your changes work for me other than the hack needed above. >
Hi Kees On 2020-07-10 3:44 p.m., Kees Cook wrote: > On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote: >> >> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote: >>> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote: >>>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, >>>>> goto out; >>>>> } >>>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) >>>>> - *buf = vmalloc(i_size); >>>>> + if (!*buf) >>>> The assumption that *buf is always NULL when id != >>>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct. >>>> I get unhandled page faults due to this change on boot. >>> Did it give you a stack backtrace? >> Yes, but there's no requirement that *buf need to be NULL when calling this >> function. >> To fix my particular crash I added the following locally: >> >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char >> __user *, uargs, int, flags) >> { >> struct load_info info = { }; >> loff_t size; >> - void *hdr; >> + void *hdr = NULL; >> int err; >> >> err = may_init_module(); > Thanks for the diagnosis and fix! I haven't had time to cycle back > around to this series yet. Hopefully soon. :) > In order to assist in your patchset I have combined it with my patch series here: https://github.com/sbranden/linux/tree/kernel_read_file_for_kees Please let me know if this matches your expectations for my patches or if there is something else I need to change. Thanks, Scott
On Thu, Jul 16, 2020 at 01:35:17PM -0700, Scott Branden wrote: > On 2020-07-10 3:44 p.m., Kees Cook wrote: > > On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote: > > > > > > On 2020-07-10 3:04 p.m., Matthew Wilcox wrote: > > > > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote: > > > > > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > > > > > > goto out; > > > > > > } > > > > > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) > > > > > > - *buf = vmalloc(i_size); > > > > > > + if (!*buf) > > > > > The assumption that *buf is always NULL when id != > > > > > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct. > > > > > I get unhandled page faults due to this change on boot. > > > > Did it give you a stack backtrace? > > > Yes, but there's no requirement that *buf need to be NULL when calling this > > > function. > > > To fix my particular crash I added the following locally: > > > > > > --- a/kernel/module.c > > > +++ b/kernel/module.c > > > @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char > > > __user *, uargs, int, flags) > > > { > > > struct load_info info = { }; > > > loff_t size; > > > - void *hdr; > > > + void *hdr = NULL; > > > int err; > > > > > > err = may_init_module(); > > Thanks for the diagnosis and fix! I haven't had time to cycle back > > around to this series yet. Hopefully soon. :) > > > In order to assist in your patchset I have combined it with my patch series > here: > https://github.com/sbranden/linux/tree/kernel_read_file_for_kees > > Please let me know if this matches your expectations for my patches or if > there is something else I need to change. Thanks! I was working on the next revision of this last night, and I'm trying to get through today's email to finish it. I'll take a look!
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index ca871b13524e..c2f57cedcd6f 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, int i, len; int rc = -ENOENT; char *path; - enum kernel_read_file_id id = READING_FIRMWARE; size_t msize = INT_MAX; void *buffer = NULL; /* 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; msize = fw_priv->allocated_size; } @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv, /* load firmware files from the mount namespace of init */ rc = kernel_read_file_from_path_initns(path, &buffer, - &size, msize, id); + &size, msize, + READING_FIRMWARE); if (rc) { if (rc != -ENOENT) dev_warn(device, "loading %s failed with error %d\n", diff --git a/fs/exec.c b/fs/exec.c index e6e8a9a70327..2bf549757ce7 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, { loff_t i_size, pos; ssize_t bytes = 0; + void *allocated = NULL; int ret; if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0) @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, goto out; } - if (id != READING_FIRMWARE_PREALLOC_BUFFER) - *buf = vmalloc(i_size); + if (!*buf) + *buf = allocated = vmalloc(i_size); if (!*buf) { ret = -ENOMEM; goto out; @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, out_free: if (ret < 0) { - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { + if (allocated) { vfree(*buf); *buf = NULL; } diff --git a/include/linux/fs.h b/include/linux/fs.h index 3f881a892ea7..95fc775ed937 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode) #endif extern int do_pipe_flags(int *, int); +/* This is a list of *what* is being read, not *how*. */ #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown) \ id(FIRMWARE, firmware) \ - id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(FIRMWARE_EFI_EMBEDDED, firmware) \ id(MODULE, kernel-module) \ id(KEXEC_IMAGE, kexec-image) \ diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index c1583d98c5e5..f80ee4ce4669 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry) int ima_read_file(struct file *file, enum kernel_read_file_id read_id) { /* - * READING_FIRMWARE_PREALLOC_BUFFER - * * Do devices using pre-allocated memory run the risk of the * firmware being accessible to the device prior to the completion * of IMA's signature verification any more than when using two - * buffers? + * buffers? It may be desirable to include the buffer address + * in this API and walk all the dma_map_single() mappings to check. */ return 0; } const int read_idmap[READING_MAX_ID] = { [READING_FIRMWARE] = FIRMWARE_CHECK, - [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK, [READING_MODULE] = MODULE_CHECK, [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs that are interested in filtering between types of things. The "how" should be an internal detail made uninteresting to the LSMs. Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer") Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)") Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)") Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/base/firmware_loader/main.c | 5 ++--- fs/exec.c | 7 ++++--- include/linux/fs.h | 2 +- security/integrity/ima/ima_main.c | 6 ++---- 4 files changed, 9 insertions(+), 11 deletions(-)