Re: [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Tue, 2017-06-13 at 09:27 +0200, Roberto Sassu wrote:
> On 6/6/2017 3:23 PM, Mimi Zohar wrote:
> > On Tue, 2017-06-06 at 14:45 +0200, Roberto Sassu wrote:
> >> On 6/6/2017 12:56 PM, Mimi Zohar wrote:
> >>> On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote:
> >>>> On 6/5/2017 8:04 AM, Mimi Zohar wrote:
> >>>>> On Tue, 2017-05-16 at 14:53 +0200, Roberto Sassu wrote:
> >>>>>> Through the new interface binary_kexec_runtime_measurements, it will be
> >>>>>> possible to read the same content returned by binary_runtime_measurements,
> >>>>>> with the kexec header prepended.
> >>>>>>
> >>>>>> The new interface has been added for testing ima_restore_measurement_list()
> >>>>>> which, at the moment, works only on PPC systems. The interface for reading
> >>>>>> the binary list with the kexec header will be provided in a separate patch.
> >>>>>>
> >>>>>> The patch reuses ima_measurements_start() and ima_measurements_next()
> >>>>>> to send the measurements list to userspace. Their behavior changes
> >>>>>> depending on the current dentry.
> >>>>>>
> >>>>>> To provide the correct information in the kexec header,
> >>>>>> ima_measurements_start() has to iterate over the whole list and calculate
> >>>>>> the number of entries and the total size. It is not possible to read
> >>>>>> the value of the global variable binary_runtime_size and ima_htable.len
> >>>>>> without taking ima_extend_list_mutex, because there might have been a list
> >>>>>> add between the two read operations.
> >>>>>
> >>>>> Please correct me if I'm wrong.  Your code walks the measurement list
> >>>>> calculating the total number of measurements and the memory size
> >>>>> needed to store in the kexec header.  Can't there be additional
> >>>>> measurements between the time these values - total number of
> >>>>> measurements and memory needed - were calculated and actually saving
> >>>>> the measurements?  How would that be any different than the problem
> >>>>> you're trying to solve?  In both cases, the number of measurements
> >>>>> might be less than the actual number of measurements.
> >>>>>
> >>>>> As long as you query the number of measurements before getting the
> >>>>> memory needed, unless you're trying to verify a TPM quote, having
> >>>>> fewer measurements shouldn't be a problem for testing.
> >>>>
> >>>> The problem is that the total number of entries and the required
> >>>> memory size might be inconsistent without taking ima_extend_list_mutex.
> >>>> ima_measurements_start() could read the entries counter before
> >>>> it is incremented by ima_add_digest_entry() and the required memory
> >>>> size after it is updated. If this happens, the parser returns an error
> >>>> because ENFORCE_BUFEND is set for the last entry and there would be
> >>>> still data to read (the new entry added to the list).
> >>>
> >>> I don't see this as being any different than what happens when the
> >>> kernel saves the measurement list. Originally, the memory size was
> >>> defined at kexec load, but only populated at kexec execute.  There was
> >>> plenty of time between the kexec load and execute for additional
> >>> measurement records to be added.
> >>>
> >>> The upstreamed version defines the buffer size and populates it at
> >>> kexec load.  However kexec load itself generates additional
> >>> measurements, so it has to reserve more memory than what is returned
> >>> by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.)
> >>
> >> ima_dump_measurement_list() determines the total number of entries and
> >> the required memory size (which are written to the kexec header) after
> >> iterating over the whole list. Are new entries added to the kexec buffer
> >> after ima_dump_measurement_list() is called?
> >
> > The upstreamed version allocates the segment in kexec load and then
> > fills the buffer.  However, in between getting the current memory size
> > needed and filling the buffer, additional measurements can be added.
> > Thus the segment size needs to be larger than the current memory
> > size.
> >
> > The header reflects the number of measurements and the actual buffer
> > size, not the segment size.  When restoring the measurement list,
> > however, we rely on the number of measurements and use the buffer size
> > as a reference to prevent accessing memory beyond the buffer.  The
> > buffer size does not need to be exact.
> 
> In this case, I have to modify patch 2/7 and remove ENFORCE_BUFEND
> from the enforcing mask. Otherwise, ima_restore_measurement_list()
> would return an error when parsing the last entry and buffer size
> in the kexec header is greater than the exact size required to
> store the measurements list.

Or the testing patches could relax the ENFORCE_BUFEND requirement.
Without the ENFORCE_BUFEND requirement, I'm not sure this patch (5/7)
will be needed.

> Should I just send the modified patch with the [RESEND] tag
> or should I send the whole patch set with an incremented version
> number?

The testing patches could be upstreamed separately, if you prefer.

> Also, since patches 4-6 are for testing, I would prefer to skip
> them for now and push a new version of the patch set for the
> Crypto Agile format first?

That's fine.  I've pushed patches 1 - 3, and 7 to the next-4.12-
ima_parse_buf branch for a day or so, before staging them in the next
branch to be upstreamed. 

The patches can be found here
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-
integrity.git/next-4.12-ima_parse_buf.

Mimi