Re: [PATCH RFC 2/3] security: IMA: introduce ima_mr structure
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-30 13:55:18
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ima: memory leak of ima_shash_tfm and ima_algo_array in ima_init_crypto()
- [Medium] ima: memory leak of digests array in ima_init_digests()
--
commit babc42a28171ec6623537f00a8d1222d9f72a6e2
Author: Yeoreum Yun [off-list ref]
security: IMA: introduce ima_mr structure
This commit introduces the ima_mr structure to abstract measurement registers
and operations, preparing the subsystem to integrate TSM measurement registers
with IMA. It also migrates the TPM device using PCRs as measurement registers
to use this new interface.
quoted hunk ↗ jump to hunk
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index 0d72b48249ee6..efa27ce5f1282 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -77,6 +77,7 @@ int __init ima_init_crypto(void) enum hash_algo algo; long rc; int i; + mr_bank_info_t bank_info; rc = ima_init_ima_crypto(); if (rc)@@ -85,8 +86,12 @@ int __init ima_init_crypto(void) ima_sha1_idx = -1; ima_hash_algo_idx = -1; - for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) { - algo = ima_tpm_chip->allocated_banks[i].crypto_id; + for (i = 0; i < NR_BANKS(ima_mr); i++) { + rc = ima_mr->ops->mr_get_bank_info(ima_mr, i, &bank_info); + if (rc) + return rc;
[Severity: Medium] Does this direct return bypass the cleanup for ima_shash_tfm? Since ima_init_ima_crypto() is called earlier and allocates it, it looks like returning rc here skips the out label where the cleanup happens.
quoted hunk ↗ jump to hunk
+ + algo = bank_info.crypto_id; if (algo == HASH_ALGO_SHA1) ima_sha1_idx = i;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -95,24 +100,28 @@ int __init ima_init_crypto(void) if (ima_hash_algo_idx < 0) - ima_hash_algo_idx = NR_BANKS(ima_tpm_chip) + ima_extra_slots++; + ima_hash_algo_idx = NR_BANKS(ima_mr) + ima_extra_slots++; ima_algo_array = kzalloc_objs(*ima_algo_array, - NR_BANKS(ima_tpm_chip) + ima_extra_slots); + NR_BANKS(ima_mr) + ima_extra_slots); if (!ima_algo_array) { rc = -ENOMEM; goto out; } - for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) { - algo = ima_tpm_chip->allocated_banks[i].crypto_id; - digest_size = ima_tpm_chip->allocated_banks[i].digest_size; + for (i = 0; i < NR_BANKS(ima_mr); i++) { + rc = ima_mr->ops->mr_get_bank_info(ima_mr, i, &bank_info); + if (rc) + return rc;
[Severity: Medium] Could this return cause a memory leak of ima_algo_array and any hash transforms allocated in previous loop iterations? Taking this path bypasses the explicitly defined out_array cleanup label.
quoted hunk ↗ jump to hunk
+ + algo = bank_info.crypto_id; + digest_size = bank_info.digest_size; ima_algo_array[i].algo = algo; ima_algo_array[i].digest_size = digest_size;
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 67782c004cdf7..037979829242c 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -548,25 +547,27 @@ void __init ima_init_reboot_notifier(void) int __init ima_init_digests(void) { + int rc, i; + mr_bank_info_t bank_info; u16 digest_size; - u16 crypto_id; - int i; - if (!ima_tpm_chip) + if (!ima_mr) return 0; - digests = kzalloc_objs(*digests, ima_tpm_chip->nr_allocated_banks, - GFP_NOFS); + digests = kzalloc_objs(*digests, NR_BANKS(ima_mr), GFP_NOFS); if (!digests) return -ENOMEM; - for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) { - digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; - digest_size = ima_tpm_chip->allocated_banks[i].digest_size; - crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id; + for (i = 0; i < NR_BANKS(ima_mr); i++) { + rc = ima_mr->ops->mr_get_bank_info(ima_mr, i, &bank_info); + if (rc) + return rc;
[Severity: Medium] If mr_get_bank_info() fails here, does this return abandon the newly allocated digests array? It appears kfree(digests) might be needed before returning rc to prevent a memory leak.
quoted hunk ↗ jump to hunk
+ + digests[i].alg_id = bank_info.alg_id; + digest_size = bank_info.digest_size; /* for unmapped TPM algorithms digest is still a padded SHA1 */
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260930-ima_tgx_integration_v2-v1-0-722c35370548@arm.com?part=2