Re: [PATCH RFC 2/3] security: IMA: introduce ima_mr structure

[sashiko-bot@kernel.org]

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 <yeoreum.yun@arm.com>

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.

> 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

[ ... ]

> @@ -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.

> +
> +		algo = bank_info.crypto_id;
>  		if (algo == HASH_ALGO_SHA1)
>  			ima_sha1_idx = i;

[ ... ]

> @@ -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.

> +
> +		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;

[ ... ]

> 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

[ ... ]

> @@ -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.

> +
> +		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