Messages in this thread | | | Date | Tue, 06 Dec 2011 10:32:35 +0100 | From | Roberto Sassu <> | Subject | Re: [PATCH] ima: fix memory leak and invalid memory reference bugs |
| |
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; >> } > >
| |