Messages in this thread | | | Subject | Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support | From | Scott Branden <> | Date | Tue, 7 Jul 2020 21:41:46 -0700 |
| |
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? >> >
| |