Re: [PATCH v3 7/7] ima: measure kexec load and exec events as critical data

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

On 12/20/23 12:41, Mimi Zohar wrote:
> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
>> There could be a potential mismatch between IMA measurements and TPM PCR
>> quotes caused by the indeterminate interval between kexec 'load' and
>> 'execute'.  Memory allocated at kexec 'load' for IMA log buffer may run
>> out. It can lead to missing events in the IMA log after a soft reboot to
>> the new Kernel, resulting in TPM PCR quotes mismatch and remote
>> attestation failures.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured as critical data at kexec 'load' and 'execute' respectively.
>>
>> These events serve as markers in the IMA log to verify the IMA events are
>> captured between kexec 'load' and 'execute' window.  The presence of a
>> 'kexec_load' event in between the last two 'boot_aggregate' events in the
>> IMA log implies this is a kexec soft reboot, and not a cold-boot. And the
>> absence of 'kexec_execute' event after kexec soft reboot implies missing
>> events in that window which results in inconsistency with TPM PCR quotes,
>> necessitating a cold boot for further successful remote attestation.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_kexec.c | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 063da9c834a0..47da41a66dcc 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;
>>   static void *ima_kexec_buffer;
>>   static size_t kexec_segment_size;
>> @@ -33,6 +35,8 @@ void ima_free_kexec_file_buf(struct seq_file *sf)
>>   
>>   static int ima_alloc_kexec_file_buf(size_t segment_size)
>>   {
>> +	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
>> +
>>   	/*
>>   	 * kexec 'load' may be called multiple times.
>>   	 * Free and realloc the buffer only if the segment_size is
>> @@ -42,7 +46,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>>   	    ima_kexec_file.size == segment_size &&
>>   	    ima_kexec_file.read_pos == 0 &&
>>   	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
>> -		return 0;
>> +		goto out;
>>   
>>   	ima_free_kexec_file_buf(&ima_kexec_file);
>>   
>> @@ -55,6 +59,13 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>>   	ima_kexec_file.read_pos = 0;
>>   	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
>>   
>> +out:
>> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
>> +		  "kexec_segment_size=%lu;", segment_size);
>> +
>> +	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
>> +				  strlen(ima_kexec_event), false, NULL, 0);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -179,6 +190,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;
>> @@ -194,6 +206,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>>   		return ret;
>>   	}
>>   
>> +	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);
>> +
>> +	ima_measure_critical_data("ima_kexec", "kexec_execute",
>> +				  ima_kexec_event, strlen(ima_kexec_event),
>> +				  false, NULL, 0);
>> +
> 
> Please consider including the number of measurement records as well.
Will do. I think that would be valuable information.

Per my understanding, I will have to use the function
ima_show_measurements_count() or ima_show_htable_value() to get the
number of measurement records value[1]. So I will have to expose that
function from "ima_fs.c" to "ima_kexec.c". Hope that's ok.

[1] 
https://elixir.bootlin.com/linux/latest/sourcesecurity/integrity/ima/ima_fs.c 


static ssize_t ima_show_measurements_count(struct file *filp,
                                          char __user *buf,
                                          size_t count, loff_t *ppos)
{
         return ima_show_htable_value(buf, count, ppos, &ima_htable.len);

}


~Tushar
> 
>>   	ima_measurements_suspend();
>>   
>>   	ret = ima_dump_measurement_list(&buf_size, &buf,
>