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@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.
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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
