Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute

[Tushar Sugandhi <tusharsu@linux.microsoft.com>]

On 10/27/23 06:08, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
>> In the current IMA implementation, ima_dump_measurement_list() is called
>> during the kexec 'load' operation.  This can result in loss of IMA
>> measurements taken between the 'load' and 'execute' phases when the
>> system goes through Kexec soft reboot to a new Kernel.  The call to the
>> function ima_dump_measurement_list() needs to be moved out of the
>> function ima_add_kexec_buffer() and needs to be called during the kexec
>> 'execute' operation.
>>
>> Implement a function ima_update_kexec_buffer() that is called during
>> kexec 'execute', allowing IMA to update the measurement list with the
>> events between kexec 'load' and 'execute'.  Move the
>> ima_dump_measurement_list() call from ima_add_kexec_buffer() to
>> ima_update_kexec_buffer().  Make ima_kexec_buffer and kexec_segment_size
>> variables global, so that they can be accessed during both kexec 'load'
>> and 'execute'.  Add functions ima_measurements_suspend() and
>> ima_measurements_resume() to set and reset the 'suspend_ima_measurements'
>> variable respectively, to suspend/resume IMA measurements.  Use
>> the existing 'ima_extend_list_mutex' to ensure that the operations are
>> thread-safe.  These function calls will help maintaining the integrity
>> of the IMA log while it is being copied to the new Kernel's buffer.
>> Add a reboot notifier_block 'update_buffer_nb' to ensure
>> the function ima_update_kexec_buffer() gets called during kexec
>> soft-reboot.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> The lengthiness and complexity of the patch description is an
> indication that the patch  needs to be broken up.  Please refer to
> Documentation/process/submitting-patches.rst  for further info.
> 
> In addition, this patch moves the function ima_dump_measurement_list()
> to a new function named ima_update_kexec_buffer(), which is never
> called.   The patch set is thus not bisect safe.
>  > [...]
>> +void ima_measurements_suspend(void)
>> +{
>> +	mutex_lock(&ima_extend_list_mutex);
>> +	atomic_set(&suspend_ima_measurements, 1);
>> +	mutex_unlock(&ima_extend_list_mutex);
>> +}
>> +
>> +void ima_measurements_resume(void)
>> +{
>> +	mutex_lock(&ima_extend_list_mutex);
>> +	atomic_set(&suspend_ima_measurements, 0);
>> +	mutex_unlock(&ima_extend_list_mutex);
>> +}
> 
> These function are being defined and called here, but are not enforced
> until a later patch.   It would make more sense to introduce and
> enforce these functions in a single patch with an explanation as to why
> suspend/resume is needed.
> 
> The cover letter describes the problem as "... new IMA measurements are
> added between kexec 'load' and kexec 'execute'".    Please include in
> the patch description the reason for needing suspend/resume, since
> saving the measurement records is done during kexec execute.
> 
Since I am introducing a few new functions in this patch set,
I am having hard time keeping the patches bisect safe and at the same 
time managing the length/complexity of the individual patches in the series.

I will go back and revisit the bisect-safeness of the patches in this 
series again.

Thanks for the feedback, appreciate it.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec