Hi Tushar,
On Thu, 2023-10-05 at 11:26 -0700, Tushar Sugandhi wrote:
> The window between kexec 'load' and 'execute' could be arbitrarily long.
> Even with the large chunk of memory allocated at kexec 'load', it may
> run out which would result in missing events in IMA log after the system
> soft reboots to the new Kernel. This would result in IMA measurements
> getting out of sync with the TPM PCR quotes which would result in remote
> attestation failing for that system. There is currently no way for the
> new Kernel to know if the IMA log TPM PCR quote out of sync problem is
> because of the missing measurements during kexec.
>
> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
> measured at kexec 'load' and 'execute' respectively.
>
> IMA measures 'boot_aggregate' as the first event when the system boots -
> either cold boot or kexec soft boot. In case when the system goes
> through multiple soft reboots, the number of 'boot_aggregate' events in
> IMA log corresponds to total number of boots (cold boot plus multiple
> kexec soft reboots). With this change, there would be additional
> 'kexec_load' and 'kexec_execute' events in between the two
> 'boot_aggregate' events. In rare cases, when the system runs out of
> memory during kexec soft reboot, 'kexec_execute' won't be copied since
> its one of the very last event measured just before kexec soft reboot.
> The absence of the event 'kexec_execute' in between the two
> boot_aggregate' events would signal the attestation service that the IMA
> log on the system is out of sync with TPM PCR quotes and the system needs
> to be cold booted for the remote attestation to succeed again.
>
> Signed-off-by: Tushar Sugandhi <tusha...@linux.microsoft.com>
Adding a new type of critical data is fine. The patch description
should describe the reason for it. Please update the Subject line and
the patch description accordingly.
> ---
> security/integrity/ima/ima_kexec.c | 35 +++++++++++++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_kexec.c
> b/security/integrity/ima/ima_kexec.c
> index 6cd5f46a7208..0f9c424fe808 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -17,6 +17,8 @@
> #include "ima.h"
>
> #ifdef CONFIG_IMA_KEXEC
> +#define IMA_KEXEC_EVENT_LEN 128
> +
> struct seq_file ima_kexec_file;
> struct ima_kexec_hdr ima_khdr;
> static void *ima_kexec_buffer;
> @@ -34,6 +36,8 @@ void ima_clear_kexec_file(void)
>
> static int ima_alloc_kexec_buf(size_t kexec_segment_size)
> {
> + char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
> +
> if ((kexec_segment_size == 0) ||
> (kexec_segment_size == ULONG_MAX) ||
> ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
> @@ -64,6 +68,12 @@ static int ima_alloc_kexec_buf(size_t kexec_segment_size)
> memset(&ima_khdr, 0, sizeof(ima_khdr));
> ima_khdr.version = 1;
>
> + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
> + "kexec_segment_size=%lu;", kexec_segment_size);
> +
> + ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
> + strlen(ima_kexec_event), false, NULL, 0);
> +
> return 0;
> }
>
> @@ -198,6 +208,7 @@ void ima_add_kexec_buffer(struct kimage *image)
> static int ima_update_kexec_buffer(struct notifier_block *self,
> unsigned long action, void *data)
> {
> + char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
> void *buf = NULL;
> size_t buf_size;
> bool resume = false;
> @@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct notifier_block
> *self,
> return NOTIFY_OK;
> }
>
> + buf_size = ima_get_binary_runtime_size();
> + scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
> + "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
> + kexec_segment_size, buf_size);
> +
> + /*
> + * This is one of the very last events measured by IMA before kexec
> + * soft rebooting into the new Kernal.
> + * This event can be used as a marker after the system soft reboots
> + * to the new Kernel to check if there was sufficient memory allocated
> + * at kexec 'load' to capture the events measured between the window
> + * of kexec 'load' and 'execute'.
> + * This event needs to be present in the IMA log, in between the two
> + * 'boot_aggregate' events that are logged for the previous boot and
> + * the current soft reboot. If it is not present after the system soft
> + * reboots into the new Kernel, it would mean the IMA log is not
> + * consistent with the TPM PCR quotes, and the system needs to be
> + * cold-booted for the attestation to succeed again.
> + */
Comments like this don't belong in the code. Please refer to section
"8) Commenting" of Documentation/process/coding-style.rst.
> + ima_measure_critical_data("ima_kexec", "kexec_execute",
> + ima_kexec_event, strlen(ima_kexec_event),
> + false, NULL, 0);
> +
> ima_measurements_suspend();
>
> - buf_size = ima_get_binary_runtime_size();
> ret = ima_dump_measurement_list(&buf_size, &buf,
> kexec_segment_size);
>
--
thanks,
Mimi
