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@xxxxxxxxxxxxxxxxxxx>
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.
--
thanks,
Mimi
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
