Re: [PATCH] ima: fix memory leak and invalid memory reference bugs

[Roberto Sassu <roberto.sassu@polito.it>]

On 12/05/2011 08:10 PM, Mimi Zohar wrote:
> On Mon, 2011-12-05 at 16:38 +0100, Roberto Sassu wrote:
>> This patch frees the memory of measurements entries that have already
>> been inserted in the measurements list and prevent the release when the
>> PCR extend operation failed.
>>
>> Signed-off-by: Roberto Sassu<roberto.sassu@polito.it>
>
> Thanks for the updated patch. I still need to test it, but it looks
> good.  The patch description should probably include an explanation of
> how a duplicate measurement is being added.  Something like:
>
> When a new measurement is added to the measurement list, this info is
> cached in the iint for performance.  When the inode is flushed from
> cache, the associated iint is flushed as well.  Subsequent access to the
> inode will cause it to be re-measured and will attempt to unnecessarily
> add it to the measurement list.
>
> This patch fixes a memory leak, by freeing the memory of the unnecessary
> duplicate measurement, and also fixes an invalid memory reference, by
> preventing the freeing of a new valid measurement entry, when the PCR
> extend operation fails.
>

Hi Mimi

do you mean that can i resend this patch as is or should i wait
until you tested it?

Thanks

Roberto Sassu


> thanks,
>
> Mimi
>
>> ---
>>   security/integrity/ima/ima_api.c   |    4 ++--
>>   security/integrity/ima/ima_queue.c |   10 ++++++----
>>   2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
>> index 0d50df0..88a2788 100644
>> --- a/security/integrity/ima/ima_api.c
>> +++ b/security/integrity/ima/ima_api.c
>> @@ -178,8 +178,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
>>   	strncpy(entry->template.file_name, filename, IMA_EVENT_NAME_LEN_MAX);
>>
>>   	result = ima_store_template(entry, violation, inode);
>> -	if (!result)
>> +	if (!result || result == -EEXIST)
>>   		iint->flags |= IMA_MEASURED;
>> -	else
>> +	if (result<  0)
>>   		kfree(entry);
>>   }
>> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
>> index 8e28f04..0fe41b81 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -94,7 +94,8 @@ static int ima_pcr_extend(const u8 *hash)
>>
>>   	result = tpm_pcr_extend(TPM_ANY_NUM, CONFIG_IMA_MEASURE_PCR_IDX, hash);
>>   	if (result != 0)
>> -		pr_err("IMA: Error Communicating to TPM chip\n");
>> +		pr_err("IMA: Error Communicating to TPM chip, result: %d\n",
>> +		       result);
>>   	return result;
>>   }
>>
>> @@ -107,13 +108,14 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   	u8 digest[IMA_DIGEST_SIZE];
>>   	const char *audit_cause = "hash_added";
>>   	int audit_info = 1;
>> -	int result = 0;
>> +	int result = 0, tpmresult = 0;
>>
>>   	mutex_lock(&ima_extend_list_mutex);
>>   	if (!violation) {
>>   		memcpy(digest, entry->digest, sizeof digest);
>>   		if (ima_lookup_digest_entry(digest)) {
>>   			audit_cause = "hash_exists";
>> +			result = -EEXIST;
>>   			goto out;
>>   		}
>>   	}
>> @@ -128,8 +130,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   	if (violation)		/* invalidate pcr */
>>   		memset(digest, 0xff, sizeof digest);
>>
>> -	result = ima_pcr_extend(digest);
>> -	if (result != 0) {
>> +	tpmresult = ima_pcr_extend(digest);
>> +	if (tpmresult != 0) {
>>   		audit_cause = "TPM error";
>>   		audit_info = 0;
>>   	}
>
>