Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1664549 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/7] ima: add securityfs interface to save a measurements  list with kexec header Roberto Sassu <roberto.sassu@huawei.com> - 2017-06-13 09:30 +0200
    Re: [PATCH 5/7] ima: add securityfs interface to save a  measurements list with kexec header Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-06-13 14:10 +0200
      Re: [PATCH 5/7] ima: add securityfs interface to save a measurements  list with kexec header Roberto Sassu <roberto.sassu@huawei.com> - 2017-06-13 14:40 +0200

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

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.

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

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.

Thanks

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1664798

On 6/13/2017 2:09 PM, Mimi Zohar wrote:
> 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.

The ENFORCE_BUFEND bit is set in patch 2/7:
---
+		enforce_mask |= (count == khdr->count) ? ENFORCE_BUFEND : 0;
---

If this requirement is too strict for restoring measurements,
then patch 2/7 should be modified.

Regarding this patch, I agree that it is not strictly necessary.
With an userspace tool, it would be possible to prepend the
kexec header to the binary list after parsing the entries.


>> 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.

Ok.

Thanks

Roberto


>> 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
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web