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