Re: [PATCH v3 5/8] ima: Switch to dynamically allocated buffer for template digests

[Mimi Zohar]

On Mon, 2020-02-10 at 11:02 +0100, Roberto Sassu wrote:
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 2f380fb92a7a..f04bec2ab83f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -45,11 +45,15 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
>  #define IMA_TEMPLATE_IMA_NAME "ima"
>  #define IMA_TEMPLATE_IMA_FMT "d|n"
>  
> +#define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
> +
>  /* current content of the policy */
>  extern int ima_policy_flag;
>  
>  /* set during initialization */
>  extern int ima_hash_algo;
> +extern int ima_sha1_idx __ro_after_init;
> +extern int ima_extra_slots __ro_after_init;
>  extern int ima_appraise;
>  extern struct tpm_chip *ima_tpm_chip;
>  
> @@ -92,7 +96,7 @@ struct ima_template_desc {
>  
>  struct ima_template_entry {
>  	int pcr;
> -	u8 digest[TPM_DIGEST_SIZE];	/* sha1 or md5 measurement hash */
> +	struct tpm_digest *digests;
>  	struct ima_template_desc *template_desc; /* template descriptor */
>  	u32 template_data_len;
>  	struct ima_field_data template_data[0];	/* template related data */
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 51f562111864..ea6d834a5fab 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -27,6 +27,7 @@ void ima_free_template_entry(struct ima_template_entry *entry)
>  	for (i = 0; i < entry->template_desc->num_fields; i++)
>  		kfree(entry->template_data[i].data);
>  
> +	kfree(entry->digests);
>  	kfree(entry);
>  }
>  
> @@ -50,6 +51,13 @@ int ima_alloc_init_template(struct ima_event_data *event_data,
>  	if (!*entry)
>  		return -ENOMEM;
>  
> +	(*entry)->digests = kcalloc(NR_BANKS(ima_tpm_chip) + ima_extra_slots,
> +				    sizeof(*(*entry)->digests), GFP_NOFS);

"sizeof(*(*entry)" should be simplified for readability.ÂÂDefining a
local "entry" or "digests" variable might help.ÂÂSimilarly in
ima_restore_template_data().

> +	if (!(*entry)->digests) {
> +		result = -ENOMEM;
> +		goto out;
> +	}
> +
>  	(*entry)->template_desc = template_desc;
>  	for (i = 0; i < template_desc->num_fields; i++) {
>  		const struct ima_template_field *field =
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 2d356ae8e823..ea24d2f6b513 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -59,6 +59,10 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size");
>  static struct crypto_shash *ima_shash_tfm;
>  static struct crypto_ahash *ima_ahash_tfm;
>  
> +int ima_sha1_idx __ro_after_init;
> +/* Additional number of slots to be reserved for SHA1 and IMA default algo */
> +int ima_extra_slots __ro_after_init = 1;

The reasoning behind needing an extra slot is clear from the patch
description, but not the reason for defining a variable.ÂÂLet's
clarify this comment a bit by inserting "reserved, as needed, for
SHA1"

> +
>  int __init ima_init_crypto(void)
>  {
>  	long rc;
> @@ -502,7 +506,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data,
>  	}
>  
>  	if (!rc)
> -		rc = crypto_shash_final(shash, entry->digest);
> +		rc = crypto_shash_final(shash,
> +					entry->digests[ima_sha1_idx].digest);

There's no reason to split this line.ÂÂIt's less than 80 characters.

Mimi

>  
>  	return rc;
>  }