Re: [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support

[Scott Branden <scott.branden@broadcom.com>]

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:
>> 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.
>
> -Kees
>
Regards,
Scott