Messages in this thread | | | Subject | Re: [PATCH 0/3] fs: reduce export usage of kerne_read*() calls | From | Scott Branden <> | Date | Fri, 22 May 2020 16:25:03 -0700 |
| |
Hi Kees,
On 2020-05-22 4:04 p.m., Kees Cook wrote: > On Fri, May 22, 2020 at 03:24:32PM -0700, Scott Branden wrote: >> On 2020-05-18 5:37 a.m., Mimi Zohar wrote: >>> On Sun, 2020-05-17 at 23:22 -0700, Christoph Hellwig wrote: >>>> On Fri, May 15, 2020 at 09:29:33PM +0000, Luis Chamberlain wrote: >>>>> On Wed, May 13, 2020 at 11:17:36AM -0700, Christoph Hellwig wrote: >>>>>> Can you also move kernel_read_* out of fs.h? That header gets pulled >>>>>> in just about everywhere and doesn't really need function not related >>>>>> to the general fs interface. >>>>> Sure, where should I dump these? >>>> Maybe a new linux/kernel_read_file.h? Bonus points for a small top >>>> of the file comment explaining the point of the interface, which I >>>> still don't get :) >>> Instead of rolling your own method of having the kernel read a file, >>> which requires call specific security hooks, this interface provides a >>> single generic set of pre and post security hooks. The >>> kernel_read_file_id enumeration permits the security hook to >>> differentiate between callers. >>> >>> To comply with secure and trusted boot concepts, a file cannot be >>> accessible to the caller until after it has been measured and/or the >>> integrity (hash/signature) appraised. >>> >>> In some cases, the file was previously read twice, first to measure >>> and/or appraise the file and then read again into a buffer for >>> use. This interface reads the file into a buffer once, calls the >>> generic post security hook, before providing the buffer to the caller. >>> (Note using firmware pre-allocated memory might be an issue.) >>> >>> Partial reading firmware will result in needing to pre-read the entire >>> file, most likely on the security pre hook. >> The entire file may be very large and not fit into a buffer. >> Hence one of the reasons for a partial read of the file. >> For security purposes, you need to change your code to limit the amount >> of data it reads into a buffer at one time to not consume or run out of much >> memory. > Hm? That's not how whole-file hashing works. :)
> > These hooks need to finish their hashing and policy checking before they > can allow the rest of the code to move forward. (That's why it's a > security hook.) If kernel memory utilization is the primary concern, > then sure, things could be rearranged to do partial read and update the > hash incrementally, but the entire file still needs to be locked, > entirely hashed by hook, then read by the caller, then unlocked and > released. > > So, if you want to have partial file reads work, you'll need to > rearchitect the way this works to avoid regressing the security coverage > of these operations. I am not familiar with how the security handling code works at all. Is the same security check run on files opened from user space? A file could be huge.
If it assumes there is there is enough memory available to read the entire file into kernel space then the improvement below can be left as a memory optimization to be done in an independent (or future) patch series.
> So, probably, the code will look something like: > > > file = kernel_open_file_for_reading(...) > file = open... > disallow_writes(file); > while (processed < size-of-file) { > buf = read(file, size...) > security_file_read_partial(buf) > } > ret = security_file_read_finished(file); > if (ret < 0) { > allow_writes(file); > return PTR_ERR(ret); > } > return file; > > while (processed < size-of-file) { > buf = read(file, size...) > firmware_send_partial(buf); > } > > kernel_close_file_for_reading(file) > allow_writes(file); > >
| |