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
