Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure buffer hash

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

On 2020-11-06 4:11 a.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> Below inline are a few additional comments.
> 
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index ae5da9f3339d..4485d87c0aa5 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -787,12 +787,15 @@ int ima_post_load_data(char *buf, loff_t size,
>>    * @func: IMA hook
>>    * @pcr: pcr to extend the measurement
>>    * @func_data: private data specific to @func, can be NULL.
>> + * @measure_buf_hash: if set to true - will measure hash of the buf,
>> + *                    instead of buf
>>    *
>>    * Based on policy, the buffer is measured into the ima log.
> 
> Both the brief and longer function descriptions need to be updated, as
> well as the last argument description.  The last argument should be
> limited to "measure buffer hash".  How it is used could be included in
> the longer function description.  The longer function description would
> include adding the buffer data or the buffer data hash to the IMA
> measurement list and extending the PCR.
> 
> For example,
> process_buffer_measurement - measure the buffer data or the buffer data
> hash
> 
Thanks Mimi. Will update the brief and longer descriptions accordingly.
> 
>>    */
>>   void process_buffer_measurement(struct inode *inode, const void *buf, int size,
>>   				const char *eventname, enum ima_hooks func,
>> -				int pcr, const char *func_data)
>> +				int pcr, const char *func_data,
>> +				bool measure_buf_hash)
>>   {
>>   	int ret = 0;
>>   	const char *audit_cause = "ENOMEM";
>> @@ -807,6 +810,8 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
>>   		struct ima_digest_data hdr;
>>   		char digest[IMA_MAX_DIGEST_SIZE];
>>   	} hash = {};
>> +	char digest_hash[IMA_MAX_DIGEST_SIZE];
>> +	int hash_len = hash_digest_size[ima_hash_algo];
>>   	int violation = 0;
>>   	int action = 0;
>>   	u32 secid;
>> @@ -855,6 +860,21 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
>>   		goto out;
>>   	}
>>   
>> +	if (measure_buf_hash) {
>> +		memcpy(digest_hash, hash.hdr.digest, hash_len);
> 
> Instead of digest_hash and hash_len, consider naming the variables
> buf_hash and buf_hashlen.
> 
Thanks. Will do.
>> +
>> +		ret = ima_calc_buffer_hash(digest_hash,
>> +					   hash_len,
>> +					   iint.ima_hash);
> 
> There's no need for each variable to be on a separate line.
> 
Thanks, will fix.
~Tushar

> thanks,
> 
> Mimi
> 
>> +		if (ret < 0) {
>> +			audit_cause = "measure_buf_hash_error";
>> +			goto out;
>> +		}
>> +
>> +		event_data.buf = digest_hash;
>> +		event_data.buf_len = hash_len;
>> +	}
>> +
>>   	ret = ima_alloc_init_template(&event_data, &entry, template);
>>   	if (ret < 0) {
>>   		audit_cause = "alloc_entry";