Re: [PATCH v2 7/7] ima: record log size at kexec load and execute

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

On 10/27/23 07:56, Mimi Zohar wrote:
> 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 <tusharsu@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.
> 
Thanks.  I described the reasons in the code comment below, because I 
thought it is important enough to stay with the code to give context. 
But I can move it from the code comment to a patch description.
>> ---
>>   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.
Will do.

~Tushar
> 
>> +	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);
>>   

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