Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter

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

Thanks for the review Mimi.

On 8/3/23 14:42, Mimi Zohar wrote:
> On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
>> Currently TPM update counter is not available external to the system,
>> for instance, a remote attestation service.  It is a problem because
>> the service cannot easily determine if the IMA log entries are missing.
>> The IMA functionality needs to be extended to measure the TPM update
>> counter from various subsystems in Linux kernel to help detect if
>> the IMA log entries are missing.
>>
>> Implement a function, 'ima_measure_update_counter()' which would retrieve
>> the TPM update counter using the previously defined function
>> 'ima_tpm_get_update_counter()'.  Format it as a string with the value
>> "update_counter=<N>;", and measure it using the function
>> 'ima_measure_critical_data()'.
>>
>> The function takes an event name as input, and the update counter value
>> is measured as part of this event.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> Explicit TPM2 quote commands do not return the quoted PCR values or the
> pcrCounter value.  Define and include a new IMA measurement record
> containing the pcrCounter, other TPM info, and IMA info in the IMA
> measurement list to help simplify detecting a trimmed/truncated
> measurement list.
Sounds good.
>> ---
>>   include/linux/ima.h               |  1 +
>>   security/integrity/ima/ima.h      |  1 +
>>   security/integrity/ima/ima_main.c | 28 ++++++++++++++++++++++++++++
>>   3 files changed, 30 insertions(+)
>>
>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> index 86b57757c7b1..f15f3a6a4c72 100644
>> --- a/include/linux/ima.h
>> +++ b/include/linux/ima.h
>> @@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label,
>>   				     const char *event_name,
>>   				     const void *buf, size_t buf_len,
>>   				     bool hash, u8 *digest, size_t digest_len);
>> +int ima_measure_update_counter(const char *event_name);
>>   
>>   #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>>   extern void ima_appraise_parse_cmdline(void);
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 4acd0e5a830f..5484bd362237 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -168,6 +168,7 @@ int __init ima_init_digests(void);
>>   int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>>   			  void *lsm_data);
>>   int ima_tpm_get_update_counter(u32 *cpu_update_counter);
>> +int ima_measure_update_counter(const char *event_name);
>>   
>>   /*
>>    * used to protect h_table and sha_table
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index d66a0a36415e..1bcd45cc5a6a 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label,
>>   }
>>   EXPORT_SYMBOL_GPL(ima_measure_critical_data);
>>   
>> +#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128
>> +int ima_measure_update_counter(const char *event_name)
>> +{
>> +	int result;
>> +	u32 update_counter = 0;
>> +	char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE];
>> +	int buf_len;
>> +
>> +	if (!event_name)
>> +		return -ENOPARAM;
>> +
>> +	result = ima_tpm_get_update_counter(&update_counter);
>> +
>> +	if (result != 0)
>> +		return result;
>> +
>> +	scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
>> +			  update_counter);
>> +
>> +	buf_len = strlen(buf);
>> +
>> +	result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
>> +				  buf, buf_len, false, NULL, 0);
>>
> The new record should contain everything needed to verify the
> pcrCounter.  For example, each IMA measurement record updates the
> pcrCounter for each TPM bank enabled.  So the number of enabled TPM
> banks and number of IMA measurements should also be included in this
> record.
Agreed. That should be valuable information.
How does the below format look like for the buf above?

version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>;

>
> Perhaps include a version number as well, so that if we ever want to
> include other information, we could.
By version number, do you mean kernel_version, or a new version
number specific to this record? Or something else?

~Tushar
> Mimi
>
>
>> +
>> +	return result;
>> +}
>> +EXPORT_SYMBOL_GPL(ima_measure_update_counter);
>> +
>>   static int __init init_ima(void)
>>   {
>>   	int error;


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