| Message ID | 20200706232309.12010-3-scott.branden@broadcom.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: add request_partial_firmware_into_buf | expand |
On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote: > Add kernel_pread_file* support to kernel to allow for partial read > of files with an offset into the file. > > Signed-off-by: Scott Branden <scott.branden@broadcom.com> > --- > fs/exec.c | 93 ++++++++++++++++++++++++-------- > include/linux/kernel_read_file.h | 17 ++++++ > 2 files changed, 87 insertions(+), 23 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 4ea87db5e4d5..e6a8a65f7478 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) > } > EXPORT_SYMBOL(open_exec); > > -int kernel_read_file(struct file *file, void **buf, loff_t *size, > - loff_t max_size, enum kernel_read_file_id id) > -{ > - loff_t i_size, pos; > +int kernel_pread_file(struct file *file, void **buf, loff_t *size, > + loff_t max_size, loff_t pos, > + enum kernel_read_file_id id) > +{ > + loff_t alloc_size; > + loff_t buf_pos; > + loff_t read_end; > + loff_t i_size; > ssize_t bytes = 0; > int ret; > > @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > ret = -EINVAL; > goto out; > } > - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { > + > + /* Default read to end of file */ > + read_end = i_size; > + > + /* Allow reading partial portion of file */ > + if ((id == READING_FIRMWARE_PARTIAL_READ) && > + (i_size > (pos + max_size))) > + read_end = pos + max_size; There's no need to involve "id" here. There are other signals about what's happening (i.e. pos != 0, max_size != i_size, etc). > + > + alloc_size = read_end - pos; > + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { > ret = -EFBIG; > goto out; > } > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) > - *buf = vmalloc(i_size); > + if ((id != READING_FIRMWARE_PARTIAL_READ) && > + (id != READING_FIRMWARE_PREALLOC_BUFFER)) > + *buf = vmalloc(alloc_size); > if (!*buf) { > ret = -ENOMEM; > goto out; > } The id usage here was a mistake in upstream, and the series I sent is trying to clean that up. Greg, it seems this series is going to end up in your tree due to it being drivers/misc? I guess I need to direct my series to Greg then, but get LSM folks Acks. > > - pos = 0; > - while (pos < i_size) { > - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos); > + buf_pos = 0; > + while (pos < read_end) { > + bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos); > if (bytes < 0) { > ret = bytes; > goto out_free; > @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > > if (bytes == 0) > break; > + > + buf_pos += bytes; > } > > - if (pos != i_size) { > + if (pos != read_end) { > ret = -EIO; > goto out_free; > } > > - ret = security_kernel_post_read_file(file, *buf, i_size, id); > + ret = security_kernel_post_read_file(file, *buf, alloc_size, id); > if (!ret) > *size = pos; This call cannot be inside kernel_pread_file(): any future LSMs will see a moving window of contents, etc. It'll need to be in kernel_read_file() proper. > > out_free: > if (ret < 0) { > - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { > + if ((id != READING_FIRMWARE_PARTIAL_READ) && > + (id != READING_FIRMWARE_PREALLOC_BUFFER)) { > vfree(*buf); > *buf = NULL; > } > @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > allow_write_access(file); > return ret; > } > + > +int kernel_read_file(struct file *file, void **buf, loff_t *size, > + loff_t max_size, enum kernel_read_file_id id) > +{ > + return kernel_pread_file(file, buf, size, max_size, 0, id); > +} > EXPORT_SYMBOL_GPL(kernel_read_file); > > -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, > - loff_t max_size, enum kernel_read_file_id id) > +int kernel_pread_file_from_path(const char *path, void **buf, > + loff_t *size, > + loff_t max_size, loff_t pos, > + enum kernel_read_file_id id) > { > struct file *file; > int ret; > @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, > if (IS_ERR(file)) > return PTR_ERR(file); > > - ret = kernel_read_file(file, buf, size, max_size, id); > + ret = kernel_pread_file(file, buf, size, max_size, pos, id); > fput(file); > return ret; > } > + > +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, > + loff_t max_size, enum kernel_read_file_id id) > +{ > + return kernel_pread_file_from_path(path, buf, size, max_size, 0, id); > +} > EXPORT_SYMBOL_GPL(kernel_read_file_from_path); > > -int kernel_read_file_from_path_initns(const char *path, void **buf, > - loff_t *size, loff_t max_size, > - enum kernel_read_file_id id) > +int kernel_pread_file_from_path_initns(const char *path, void **buf, > + loff_t *size, > + loff_t max_size, loff_t pos, > + enum kernel_read_file_id id) > { > struct file *file; > struct path root; > @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, > if (IS_ERR(file)) > return PTR_ERR(file); > > - ret = kernel_read_file(file, buf, size, max_size, id); > + ret = kernel_pread_file(file, buf, size, max_size, pos, id); > fput(file); > return ret; > } > + > +int kernel_read_file_from_path_initns(const char *path, void **buf, > + loff_t *size, loff_t max_size, > + enum kernel_read_file_id id) > +{ > + return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id); > +} > EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns); > > -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, > - enum kernel_read_file_id id) > +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, > + loff_t max_size, loff_t pos, > + enum kernel_read_file_id id) > { > struct fd f = fdget(fd); > int ret = -EBADF; > @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, > if (!f.file) > goto out; > > - ret = kernel_read_file(f.file, buf, size, max_size, id); > + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id); > out: > fdput(f); > return ret; > } > + > +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, > + enum kernel_read_file_id id) > +{ > + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id); > +} > EXPORT_SYMBOL_GPL(kernel_read_file_from_fd); For each of these execution path, the mapping to LSM hooks is: - all path must call security_kernel_read_file(file, id) before reading (this appears to be fine as-is in your series). - anything doing a "full" read needs to call security_kernel_post_read_file() with the file and full buffer, size, etc (so all the kernel_read_file*() paths). I imagine instead of adding 3 copy/pasted versions of this, it may be possible to refactor the helpers into a single core "full" caller that takes struct file, or doing some logic in kernel_pread_file() that notices it has the entire file in the buffer and doing the call then. As an example of what I mean about doing the call, here's how I might imagine it for one of the paths if it took struct file: int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id) { int ret; ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id); if (ret) return ret; return security_kernel_post_read_file(file, buf, *size, id); } > > #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ > diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h > index 53f5ca41519a..f061ccb8d0b4 100644 > --- a/include/linux/kernel_read_file.h > +++ b/include/linux/kernel_read_file.h > @@ -8,6 +8,7 @@ > #define __kernel_read_file_id(id) \ > id(UNKNOWN, unknown) \ > id(FIRMWARE, firmware) \ > + id(FIRMWARE_PARTIAL_READ, firmware) \ > id(FIRMWARE_PREALLOC_BUFFER, firmware) \ > id(FIRMWARE_EFI_EMBEDDED, firmware) \ And again, sorry that this was in here as a misleading example. > id(MODULE, kernel-module) \ > @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) > return kernel_read_file_str[id]; > } > > +int kernel_pread_file(struct file *file, > + void **buf, loff_t *size, loff_t pos, > + loff_t max_size, > + enum kernel_read_file_id id); > int kernel_read_file(struct file *file, > void **buf, loff_t *size, loff_t max_size, > enum kernel_read_file_id id); > +int kernel_pread_file_from_path(const char *path, > + void **buf, loff_t *size, loff_t pos, > + loff_t max_size, > + enum kernel_read_file_id id); > int kernel_read_file_from_path(const char *path, > void **buf, loff_t *size, loff_t max_size, > enum kernel_read_file_id id); > +int kernel_pread_file_from_path_initns(const char *path, > + void **buf, loff_t *size, loff_t pos, > + loff_t max_size, > + enum kernel_read_file_id id); > int kernel_read_file_from_path_initns(const char *path, > void **buf, loff_t *size, loff_t max_size, > enum kernel_read_file_id id); > +int kernel_pread_file_from_fd(int fd, > + void **buf, loff_t *size, loff_t pos, > + loff_t max_size, > + enum kernel_read_file_id id); > int kernel_read_file_from_fd(int fd, > void **buf, loff_t *size, loff_t max_size, > enum kernel_read_file_id id); I remain concerned that adding these helpers will lead a poor interaction with LSMs, but I guess I get to hold my tongue. :)
On Tue, 2020-07-07 at 16:56 -0700, Kees Cook wrote: > > @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, > > ret = -EINVAL; > > goto out; > > } > > - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { > > + > > + /* Default read to end of file */ > > + read_end = i_size; > > + > > + /* Allow reading partial portion of file */ > > + if ((id == READING_FIRMWARE_PARTIAL_READ) && > > + (i_size > (pos + max_size))) > > + read_end = pos + max_size; > > There's no need to involve "id" here. There are other signals about > what's happening (i.e. pos != 0, max_size != i_size, etc). Both the pre and post security kernel_read_file hooks are called here, but there isn't enough information being passed to the LSM/IMA to be able to different which hook is applicable. One method of providing that additional information is by enumeration. The other option would be to pass some additional information. For example, on the post kernel_read_file hook, the file is read once into memory. IMA calculates the firmware file hash based on the buffer contents. On the pre kernel_read_file hook, IMA would need to read the entire file, calculating the file hash. Both methods of calculating the file hash work, but the post hook is more efficient. Mimi
On 2020-07-07 4:56 p.m., Kees Cook wrote: > On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote: >> Add kernel_pread_file* support to kernel to allow for partial read >> of files with an offset into the file. >> >> Signed-off-by: Scott Branden <scott.branden@broadcom.com> >> --- >> fs/exec.c | 93 ++++++++++++++++++++++++-------- >> include/linux/kernel_read_file.h | 17 ++++++ >> 2 files changed, 87 insertions(+), 23 deletions(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 4ea87db5e4d5..e6a8a65f7478 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) >> } >> EXPORT_SYMBOL(open_exec); >> >> -int kernel_read_file(struct file *file, void **buf, loff_t *size, >> - loff_t max_size, enum kernel_read_file_id id) >> -{ >> - loff_t i_size, pos; >> +int kernel_pread_file(struct file *file, void **buf, loff_t *size, >> + loff_t max_size, loff_t pos, >> + enum kernel_read_file_id id) >> +{ >> + loff_t alloc_size; >> + loff_t buf_pos; >> + loff_t read_end; >> + loff_t i_size; >> ssize_t bytes = 0; >> int ret; >> >> @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, >> ret = -EINVAL; >> goto out; >> } >> - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { >> + >> + /* Default read to end of file */ >> + read_end = i_size; >> + >> + /* Allow reading partial portion of file */ >> + if ((id == READING_FIRMWARE_PARTIAL_READ) && >> + (i_size > (pos + max_size))) >> + read_end = pos + max_size; > There's no need to involve "id" here. There are other signals about > what's happening (i.e. pos != 0, max_size != i_size, etc). There are other signals other than the fact that kernel_read_file requires the entire file to be read while kernel_pread_file allows partial files to be read. So if you do a pread at pos = 0 you need another key to indicate it is "ok" if max_size < i_size. If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to share 99% of the code between kernel_read_file and kernel_pread_file then I need to add another parameter to a common function called between these functions. And adding another parameter was rejected previously in the review as a "swiss army knife approach" by another reviewer. I am happy to add it back in because it is necessary to share code and differentiate whether we are performing a partial read or not. > >> + >> + alloc_size = read_end - pos; >> + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { >> ret = -EFBIG; >> goto out; >> } >> >> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) >> - *buf = vmalloc(i_size); >> + if ((id != READING_FIRMWARE_PARTIAL_READ) && >> + (id != READING_FIRMWARE_PREALLOC_BUFFER)) >> + *buf = vmalloc(alloc_size); >> if (!*buf) { >> ret = -ENOMEM; >> goto out; >> } > The id usage here was a mistake in upstream, and the series I sent is > trying to clean that up. I see that cleanup and it works fine with the pread. Other than I need some sort of key to share code and indicate whether it is "ok" to do a partial read of the file or not. > > Greg, it seems this series is going to end up in your tree due to it > being drivers/misc? I guess I need to direct my series to Greg then, but > get LSM folks Acks. > >> >> - pos = 0; >> - while (pos < i_size) { >> - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos); >> + buf_pos = 0; >> + while (pos < read_end) { >> + bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos); >> if (bytes < 0) { >> ret = bytes; >> goto out_free; >> @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, >> >> if (bytes == 0) >> break; >> + >> + buf_pos += bytes; >> } >> >> - if (pos != i_size) { >> + if (pos != read_end) { >> ret = -EIO; >> goto out_free; >> } >> >> - ret = security_kernel_post_read_file(file, *buf, i_size, id); >> + ret = security_kernel_post_read_file(file, *buf, alloc_size, id); >> if (!ret) >> *size = pos; > This call cannot be inside kernel_pread_file(): any future LSMs will see > a moving window of contents, etc. It'll need to be in kernel_read_file() > proper. If IMA still passes (after testing my next patch series with your changes and my modifications) I will need some more help here. > >> >> out_free: >> if (ret < 0) { >> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { >> + if ((id != READING_FIRMWARE_PARTIAL_READ) && >> + (id != READING_FIRMWARE_PREALLOC_BUFFER)) { >> vfree(*buf); >> *buf = NULL; >> } >> @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, >> allow_write_access(file); >> return ret; >> } >> + >> +int kernel_read_file(struct file *file, void **buf, loff_t *size, >> + loff_t max_size, enum kernel_read_file_id id) >> +{ >> + return kernel_pread_file(file, buf, size, max_size, 0, id); >> +} >> EXPORT_SYMBOL_GPL(kernel_read_file); >> >> -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, >> - loff_t max_size, enum kernel_read_file_id id) >> +int kernel_pread_file_from_path(const char *path, void **buf, >> + loff_t *size, >> + loff_t max_size, loff_t pos, >> + enum kernel_read_file_id id) >> { >> struct file *file; >> int ret; >> @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, >> if (IS_ERR(file)) >> return PTR_ERR(file); >> >> - ret = kernel_read_file(file, buf, size, max_size, id); >> + ret = kernel_pread_file(file, buf, size, max_size, pos, id); >> fput(file); >> return ret; >> } >> + >> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, >> + loff_t max_size, enum kernel_read_file_id id) >> +{ >> + return kernel_pread_file_from_path(path, buf, size, max_size, 0, id); >> +} >> EXPORT_SYMBOL_GPL(kernel_read_file_from_path); >> >> -int kernel_read_file_from_path_initns(const char *path, void **buf, >> - loff_t *size, loff_t max_size, >> - enum kernel_read_file_id id) >> +int kernel_pread_file_from_path_initns(const char *path, void **buf, >> + loff_t *size, >> + loff_t max_size, loff_t pos, >> + enum kernel_read_file_id id) >> { >> struct file *file; >> struct path root; >> @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, >> if (IS_ERR(file)) >> return PTR_ERR(file); >> >> - ret = kernel_read_file(file, buf, size, max_size, id); >> + ret = kernel_pread_file(file, buf, size, max_size, pos, id); >> fput(file); >> return ret; >> } >> + >> +int kernel_read_file_from_path_initns(const char *path, void **buf, >> + loff_t *size, loff_t max_size, >> + enum kernel_read_file_id id) >> +{ >> + return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id); >> +} >> EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns); >> >> -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, >> - enum kernel_read_file_id id) >> +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, >> + loff_t max_size, loff_t pos, >> + enum kernel_read_file_id id) >> { >> struct fd f = fdget(fd); >> int ret = -EBADF; >> @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, >> if (!f.file) >> goto out; >> >> - ret = kernel_read_file(f.file, buf, size, max_size, id); >> + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id); >> out: >> fdput(f); >> return ret; >> } >> + >> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, >> + enum kernel_read_file_id id) >> +{ >> + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id); >> +} >> EXPORT_SYMBOL_GPL(kernel_read_file_from_fd); > For each of these execution path, the mapping to LSM hooks is: > > - all path must call security_kernel_read_file(file, id) before reading > (this appears to be fine as-is in your series). > > - anything doing a "full" read needs to call > security_kernel_post_read_file() with the file and full buffer, size, > etc (so all the kernel_read_file*() paths). I imagine instead of > adding 3 copy/pasted versions of this, it may be possible to refactor > the helpers into a single core "full" caller that takes struct file, > or doing some logic in kernel_pread_file() that notices it has the > entire file in the buffer and doing the call then. > As an example of what I mean about doing the call, here's how I might > imagine it for one of the paths if it took struct file: > > int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size, > loff_t max_size, enum kernel_read_file_id id) > { > int ret; > > ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id); > if (ret) > return ret; > return security_kernel_post_read_file(file, buf, *size, id); > } > >> >> #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ >> diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h >> index 53f5ca41519a..f061ccb8d0b4 100644 >> --- a/include/linux/kernel_read_file.h >> +++ b/include/linux/kernel_read_file.h >> @@ -8,6 +8,7 @@ >> #define __kernel_read_file_id(id) \ >> id(UNKNOWN, unknown) \ >> id(FIRMWARE, firmware) \ >> + id(FIRMWARE_PARTIAL_READ, firmware) \ >> id(FIRMWARE_PREALLOC_BUFFER, firmware) \ >> id(FIRMWARE_EFI_EMBEDDED, firmware) \ > And again, sorry that this was in here as a misleading example. > >> id(MODULE, kernel-module) \ >> @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) >> return kernel_read_file_str[id]; >> } >> >> +int kernel_pread_file(struct file *file, >> + void **buf, loff_t *size, loff_t pos, >> + loff_t max_size, >> + enum kernel_read_file_id id); >> int kernel_read_file(struct file *file, >> void **buf, loff_t *size, loff_t max_size, >> enum kernel_read_file_id id); >> +int kernel_pread_file_from_path(const char *path, >> + void **buf, loff_t *size, loff_t pos, >> + loff_t max_size, >> + enum kernel_read_file_id id); >> int kernel_read_file_from_path(const char *path, >> void **buf, loff_t *size, loff_t max_size, >> enum kernel_read_file_id id); >> +int kernel_pread_file_from_path_initns(const char *path, >> + void **buf, loff_t *size, loff_t pos, >> + loff_t max_size, >> + enum kernel_read_file_id id); >> int kernel_read_file_from_path_initns(const char *path, >> void **buf, loff_t *size, loff_t max_size, >> enum kernel_read_file_id id); >> +int kernel_pread_file_from_fd(int fd, >> + void **buf, loff_t *size, loff_t pos, >> + loff_t max_size, >> + enum kernel_read_file_id id); >> int kernel_read_file_from_fd(int fd, >> void **buf, loff_t *size, loff_t max_size, >> enum kernel_read_file_id id); > I remain concerned that adding these helpers will lead a poor > interaction with LSMs, but I guess I get to hold my tongue. :) We could add pread functions that are "unsafe" in nature instead then? As I certainly do not need any integrity checks on the file for my driver. The real check is done by the card the data is loaded to whether is passes the linux security checks or not. And then, if someone does want to do something "safe" with preads another kernel_read_file_securelock/unlock could be added for those that need security for their partial reads? >
Hi Kees, one more comment below. On 2020-07-07 9:01 p.m., Scott Branden wrote: > > > On 2020-07-07 4:56 p.m., Kees Cook wrote: >> On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote: >>> Add kernel_pread_file* support to kernel to allow for partial read >>> of files with an offset into the file. >>> >>> Signed-off-by: Scott Branden <scott.branden@broadcom.com> >>> --- >>> fs/exec.c | 93 >>> ++++++++++++++++++++++++-------- >>> include/linux/kernel_read_file.h | 17 ++++++ >>> 2 files changed, 87 insertions(+), 23 deletions(-) >>> >>> diff --git a/fs/exec.c b/fs/exec.c >>> index 4ea87db5e4d5..e6a8a65f7478 100644 >>> --- a/fs/exec.c >>> +++ b/fs/exec.c >>> @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) >>> } >>> EXPORT_SYMBOL(open_exec); >>> -int kernel_read_file(struct file *file, void **buf, loff_t *size, >>> - loff_t max_size, enum kernel_read_file_id id) >>> -{ >>> - loff_t i_size, pos; >>> +int kernel_pread_file(struct file *file, void **buf, loff_t *size, >>> + loff_t max_size, loff_t pos, >>> + enum kernel_read_file_id id) >>> +{ >>> + loff_t alloc_size; >>> + loff_t buf_pos; >>> + loff_t read_end; >>> + loff_t i_size; >>> ssize_t bytes = 0; >>> int ret; >>> @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void >>> **buf, loff_t *size, >>> ret = -EINVAL; >>> goto out; >>> } >>> - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { >>> + >>> + /* Default read to end of file */ >>> + read_end = i_size; >>> + >>> + /* Allow reading partial portion of file */ >>> + if ((id == READING_FIRMWARE_PARTIAL_READ) && >>> + (i_size > (pos + max_size))) >>> + read_end = pos + max_size; >> There's no need to involve "id" here. There are other signals about >> what's happening (i.e. pos != 0, max_size != i_size, etc). > There are other signals other than the fact that kernel_read_file > requires > the entire file to be read while kernel_pread_file allows partial > files to be read. > So if you do a pread at pos = 0 you need another key to indicate it is > "ok" if max_size < i_size. > If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to > share 99% of the code > between kernel_read_file and kernel_pread_file then I need to add > another parameter to a common function > called between these functions. And adding another parameter was > rejected previously in the review as a "swiss army knife approach" by > another reviewer. I am happy to add it back in because it is > necessary to share code and differentiate whether we are performing a > partial read or not. >> >>> + >>> + alloc_size = read_end - pos; >>> + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > >>> max_size)) { >>> ret = -EFBIG; >>> goto out; >>> } >>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) >>> - *buf = vmalloc(i_size); >>> + if ((id != READING_FIRMWARE_PARTIAL_READ) && >>> + (id != READING_FIRMWARE_PREALLOC_BUFFER)) >>> + *buf = vmalloc(alloc_size); >>> if (!*buf) { >>> ret = -ENOMEM; >>> goto out; >>> } >> The id usage here was a mistake in upstream, and the series I sent is >> trying to clean that up. > I see that cleanup and it works fine with the pread. Other than I > need some sort of key to share code and indicate whether it is "ok" to > do a partial read of the file or not. >> >> Greg, it seems this series is going to end up in your tree due to it >> being drivers/misc? I guess I need to direct my series to Greg then, but >> get LSM folks Acks. >> >>> - pos = 0; >>> - while (pos < i_size) { >>> - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos); >>> + buf_pos = 0; >>> + while (pos < read_end) { >>> + bytes = kernel_read(file, *buf + buf_pos, read_end - pos, >>> &pos); >>> if (bytes < 0) { >>> ret = bytes; >>> goto out_free; >>> @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void >>> **buf, loff_t *size, >>> if (bytes == 0) >>> break; >>> + >>> + buf_pos += bytes; >>> } >>> - if (pos != i_size) { >>> + if (pos != read_end) { >>> ret = -EIO; >>> goto out_free; >>> } >>> - ret = security_kernel_post_read_file(file, *buf, i_size, id); >>> + ret = security_kernel_post_read_file(file, *buf, alloc_size, id); >>> if (!ret) >>> *size = pos; >> This call cannot be inside kernel_pread_file(): any future LSMs will see >> a moving window of contents, etc. It'll need to be in kernel_read_file() >> proper. > If IMA still passes (after testing my next patch series with your > changes and my modifications) > I will need some more help here. >> >>> out_free: >>> if (ret < 0) { >>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { >>> + if ((id != READING_FIRMWARE_PARTIAL_READ) && >>> + (id != READING_FIRMWARE_PREALLOC_BUFFER)) { >>> vfree(*buf); >>> *buf = NULL; >>> } >>> @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void >>> **buf, loff_t *size, >>> allow_write_access(file); >>> return ret; >>> } >>> + >>> +int kernel_read_file(struct file *file, void **buf, loff_t *size, >>> + loff_t max_size, enum kernel_read_file_id id) >>> +{ >>> + return kernel_pread_file(file, buf, size, max_size, 0, id); >>> +} >>> EXPORT_SYMBOL_GPL(kernel_read_file); >>> -int kernel_read_file_from_path(const char *path, void **buf, >>> loff_t *size, >>> - loff_t max_size, enum kernel_read_file_id id) >>> +int kernel_pread_file_from_path(const char *path, void **buf, >>> + loff_t *size, >>> + loff_t max_size, loff_t pos, >>> + enum kernel_read_file_id id) >>> { >>> struct file *file; >>> int ret; >>> @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char >>> *path, void **buf, loff_t *size, >>> if (IS_ERR(file)) >>> return PTR_ERR(file); >>> - ret = kernel_read_file(file, buf, size, max_size, id); >>> + ret = kernel_pread_file(file, buf, size, max_size, pos, id); >>> fput(file); >>> return ret; >>> } >>> + >>> +int kernel_read_file_from_path(const char *path, void **buf, loff_t >>> *size, >>> + loff_t max_size, enum kernel_read_file_id id) >>> +{ >>> + return kernel_pread_file_from_path(path, buf, size, max_size, >>> 0, id); >>> +} >>> EXPORT_SYMBOL_GPL(kernel_read_file_from_path); >>> -int kernel_read_file_from_path_initns(const char *path, void **buf, >>> - loff_t *size, loff_t max_size, >>> - enum kernel_read_file_id id) >>> +int kernel_pread_file_from_path_initns(const char *path, void **buf, >>> + loff_t *size, >>> + loff_t max_size, loff_t pos, >>> + enum kernel_read_file_id id) >>> { >>> struct file *file; >>> struct path root; >>> @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const >>> char *path, void **buf, >>> if (IS_ERR(file)) >>> return PTR_ERR(file); >>> - ret = kernel_read_file(file, buf, size, max_size, id); >>> + ret = kernel_pread_file(file, buf, size, max_size, pos, id); >>> fput(file); >>> return ret; >>> } >>> + >>> +int kernel_read_file_from_path_initns(const char *path, void **buf, >>> + loff_t *size, loff_t max_size, >>> + enum kernel_read_file_id id) >>> +{ >>> + return kernel_pread_file_from_path_initns(path, buf, size, >>> max_size, 0, id); >>> +} >>> EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns); >>> -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, >>> loff_t max_size, >>> - enum kernel_read_file_id id) >>> +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, >>> + loff_t max_size, loff_t pos, >>> + enum kernel_read_file_id id) >>> { >>> struct fd f = fdget(fd); >>> int ret = -EBADF; >>> @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void >>> **buf, loff_t *size, loff_t max_size, >>> if (!f.file) >>> goto out; >>> - ret = kernel_read_file(f.file, buf, size, max_size, id); >>> + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id); >>> out: >>> fdput(f); >>> return ret; >>> } >>> + >>> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, >>> loff_t max_size, >>> + enum kernel_read_file_id id) >>> +{ >>> + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id); >>> +} >>> EXPORT_SYMBOL_GPL(kernel_read_file_from_fd); >> For each of these execution path, the mapping to LSM hooks is: >> >> - all path must call security_kernel_read_file(file, id) before reading >> (this appears to be fine as-is in your series). >> >> - anything doing a "full" read needs to call >> security_kernel_post_read_file() with the file and full buffer, size, >> etc (so all the kernel_read_file*() paths). I imagine instead of >> adding 3 copy/pasted versions of this, it may be possible to refactor >> the helpers into a single core "full" caller that takes struct file, >> or doing some logic in kernel_pread_file() that notices it has the >> entire file in the buffer and doing the call then. >> As an example of what I mean about doing the call, here's how I might >> imagine it for one of the paths if it took struct file: >> >> int kernel_read_file_from_file(struct file *file, void **buf, loff_t >> *size, >> loff_t max_size, enum kernel_read_file_id id) >> { >> int ret; >> >> ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id); >> if (ret) >> return ret; >> return security_kernel_post_read_file(file, buf, *size, id); >> } >> >>> #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ >>> diff --git a/include/linux/kernel_read_file.h >>> b/include/linux/kernel_read_file.h >>> index 53f5ca41519a..f061ccb8d0b4 100644 >>> --- a/include/linux/kernel_read_file.h >>> +++ b/include/linux/kernel_read_file.h >>> @@ -8,6 +8,7 @@ >>> #define __kernel_read_file_id(id) \ >>> id(UNKNOWN, unknown) \ >>> id(FIRMWARE, firmware) \ >>> + id(FIRMWARE_PARTIAL_READ, firmware) \ >>> id(FIRMWARE_PREALLOC_BUFFER, firmware) \ >>> id(FIRMWARE_EFI_EMBEDDED, firmware) \ >> And again, sorry that this was in here as a misleading example. >> >>> id(MODULE, kernel-module) \ >>> @@ -36,15 +37,31 @@ static inline const char >>> *kernel_read_file_id_str(enum kernel_read_file_id id) >>> return kernel_read_file_str[id]; >>> } >>> +int kernel_pread_file(struct file *file, >>> + void **buf, loff_t *size, loff_t pos, >>> + loff_t max_size, >>> + enum kernel_read_file_id id); >>> int kernel_read_file(struct file *file, >>> void **buf, loff_t *size, loff_t max_size, >>> enum kernel_read_file_id id); >>> +int kernel_pread_file_from_path(const char *path, >>> + void **buf, loff_t *size, loff_t pos, >>> + loff_t max_size, >>> + enum kernel_read_file_id id); >>> int kernel_read_file_from_path(const char *path, >>> void **buf, loff_t *size, loff_t max_size, >>> enum kernel_read_file_id id); >>> +int kernel_pread_file_from_path_initns(const char *path, >>> + void **buf, loff_t *size, loff_t pos, >>> + loff_t max_size, >>> + enum kernel_read_file_id id); >>> int kernel_read_file_from_path_initns(const char *path, >>> void **buf, loff_t *size, loff_t max_size, >>> enum kernel_read_file_id id); >>> +int kernel_pread_file_from_fd(int fd, >>> + void **buf, loff_t *size, loff_t pos, >>> + loff_t max_size, >>> + enum kernel_read_file_id id); >>> int kernel_read_file_from_fd(int fd, >>> void **buf, loff_t *size, loff_t max_size, >>> enum kernel_read_file_id id); >> I remain concerned that adding these helpers will lead a poor >> interaction with LSMs, but I guess I get to hold my tongue. :) I only need kernel_pread_file and kernel_pread_file_from_path_initns. kernel_pread_file_from_fd and kernel_pread_file_from_path were only added for completeness. And are really only helper functions called by their kernel_read_file* counterparts at this time. So they can be removed from this patch if that helps? > We could add pread functions that are "unsafe" in nature instead then? > As I certainly do not need any integrity checks on the file for my > driver. The real check is done by the card the data is loaded to > whether is passes the linux security checks or not. > And then, if someone does want to do something "safe" with preads > another kernel_read_file_securelock/unlock could be added for those > that need security for their partial reads? >> >
diff --git a/fs/exec.c b/fs/exec.c index 4ea87db5e4d5..e6a8a65f7478 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -928,10 +928,14 @@ struct file *open_exec(const char *name) } EXPORT_SYMBOL(open_exec); -int kernel_read_file(struct file *file, void **buf, loff_t *size, - loff_t max_size, enum kernel_read_file_id id) -{ - loff_t i_size, pos; +int kernel_pread_file(struct file *file, void **buf, loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) +{ + loff_t alloc_size; + loff_t buf_pos; + loff_t read_end; + loff_t i_size; ssize_t bytes = 0; int ret; @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, ret = -EINVAL; goto out; } - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) { + + /* Default read to end of file */ + read_end = i_size; + + /* Allow reading partial portion of file */ + if ((id == READING_FIRMWARE_PARTIAL_READ) && + (i_size > (pos + max_size))) + read_end = pos + max_size; + + alloc_size = read_end - pos; + if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { ret = -EFBIG; goto out; } - if (id != READING_FIRMWARE_PREALLOC_BUFFER) - *buf = vmalloc(i_size); + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) + *buf = vmalloc(alloc_size); if (!*buf) { ret = -ENOMEM; goto out; } - pos = 0; - while (pos < i_size) { - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos); + buf_pos = 0; + while (pos < read_end) { + bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos); if (bytes < 0) { ret = bytes; goto out_free; @@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, if (bytes == 0) break; + + buf_pos += bytes; } - if (pos != i_size) { + if (pos != read_end) { ret = -EIO; goto out_free; } - ret = security_kernel_post_read_file(file, *buf, i_size, id); + ret = security_kernel_post_read_file(file, *buf, alloc_size, id); if (!ret) *size = pos; out_free: if (ret < 0) { - if (id != READING_FIRMWARE_PREALLOC_BUFFER) { + if ((id != READING_FIRMWARE_PARTIAL_READ) && + (id != READING_FIRMWARE_PREALLOC_BUFFER)) { vfree(*buf); *buf = NULL; } @@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, allow_write_access(file); return ret; } + +int kernel_read_file(struct file *file, void **buf, loff_t *size, + loff_t max_size, enum kernel_read_file_id id) +{ + return kernel_pread_file(file, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file); -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, - loff_t max_size, enum kernel_read_file_id id) +int kernel_pread_file_from_path(const char *path, void **buf, + loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct file *file; int ret; @@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, if (IS_ERR(file)) return PTR_ERR(file); - ret = kernel_read_file(file, buf, size, max_size, id); + ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; } + +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, + loff_t max_size, enum kernel_read_file_id id) +{ + return kernel_pread_file_from_path(path, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_path); -int kernel_read_file_from_path_initns(const char *path, void **buf, - loff_t *size, loff_t max_size, - enum kernel_read_file_id id) +int kernel_pread_file_from_path_initns(const char *path, void **buf, + loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct file *file; struct path root; @@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, if (IS_ERR(file)) return PTR_ERR(file); - ret = kernel_read_file(file, buf, size, max_size, id); + ret = kernel_pread_file(file, buf, size, max_size, pos, id); fput(file); return ret; } + +int kernel_read_file_from_path_initns(const char *path, void **buf, + loff_t *size, loff_t max_size, + enum kernel_read_file_id id) +{ + return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns); -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, - enum kernel_read_file_id id) +int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size, + loff_t max_size, loff_t pos, + enum kernel_read_file_id id) { struct fd f = fdget(fd); int ret = -EBADF; @@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, if (!f.file) goto out; - ret = kernel_read_file(f.file, buf, size, max_size, id); + ret = kernel_pread_file(f.file, buf, size, max_size, pos, id); out: fdput(f); return ret; } + +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, + enum kernel_read_file_id id) +{ + return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id); +} EXPORT_SYMBOL_GPL(kernel_read_file_from_fd); #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \ diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h index 53f5ca41519a..f061ccb8d0b4 100644 --- a/include/linux/kernel_read_file.h +++ b/include/linux/kernel_read_file.h @@ -8,6 +8,7 @@ #define __kernel_read_file_id(id) \ id(UNKNOWN, unknown) \ id(FIRMWARE, firmware) \ + id(FIRMWARE_PARTIAL_READ, firmware) \ id(FIRMWARE_PREALLOC_BUFFER, firmware) \ id(FIRMWARE_EFI_EMBEDDED, firmware) \ id(MODULE, kernel-module) \ @@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) return kernel_read_file_str[id]; } +int kernel_pread_file(struct file *file, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file(struct file *file, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path(const char *path, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_path_initns(const char *path, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_path_initns(const char *path, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id); +int kernel_pread_file_from_fd(int fd, + void **buf, loff_t *size, loff_t pos, + loff_t max_size, + enum kernel_read_file_id id); int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, enum kernel_read_file_id id);
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(-)