Messages in this thread | | | Subject | Re: [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header | From | Roberto Sassu <> | Date | Tue, 13 Jun 2017 09:27:16 +0200 |
| |
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
| |