Re: [RFC PATCH v2 08/13] ima: track the set of PCRs ever extended

[Mimi Zohar]

On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> Right now, PCR banks with unsupported hash algorithms are getting
> invalidated over and over again for each new measurement list entry
> recorded.
> 
> A subsequent patch will make IMA to invalidate PCR banks associated with
> unsupported hash algorithms only once at a PCR's first use. To prepare for
> that, make it track the set of PCRs ever extended.
> 
> Maintain the set of touched PCRs in an unsigned long bitmask,
> 'ima_extended_pcrs_mask'.
> 
> Amend the IMA_INVALID_PCR() #define to check that a given PCR can get
> represented in that bitmask. Note that this is only for improving code
> maintainablity, it does not actually constain the set of allowed PCR
> indices any further.
> 
> Make ima_pcr_extend() to maintain the ima_extended_pcrs_mask, i.e. to set
> the currently extented PCR's corresponding bit.
> 
> Note that at this point there's no provision to restore the
> ima_extended_pcrs_mask value after kexecs yet, that will be the subject of
> later patches.
> 
> Signed-off-by: Nicolai Stange <nstange@xxxxxxx>

Hi Nicolai,

IMA extends measurements in the default TPM PCR based on the Kconfig
CONFIG_IMA_MEASURE_PCR_IDX option.  Normally that is set to PCR 10.  The IMA
policy rules may override the default PCR with a per policy rule specific PCR.

INVALID_PCR() checks the IMA policy rule specified is a valid PCR register.

Is the purpose of this patch to have a single per TPM bank violation or multiple
violations, one for each PCR used within the TPM bank?

thanks,

Mimi

> ---
>  security/integrity/ima/ima.h       |  8 ++++++--
>  security/integrity/ima/ima_queue.c | 17 +++++++++++++----
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 1158a7b8bf6b..f99b1f81b35c 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -20,6 +20,7 @@
>  #include <linux/hash.h>
>  #include <linux/tpm.h>
>  #include <linux/audit.h>
> +#include <linux/minmax.h>
>  #include <crypto/hash_info.h>
>  
>  #include "../integrity.h"
> @@ -62,6 +63,8 @@ extern int ima_hash_algo_idx __ro_after_init;
>  extern int ima_extra_slots __ro_after_init;
>  extern struct ima_algo_desc *ima_algo_array __ro_after_init;
>  
> +extern unsigned long ima_extended_pcrs_mask;
> +
>  extern int ima_appraise;
>  extern struct tpm_chip *ima_tpm_chip;
>  extern const char boot_aggregate_name[];
> @@ -198,8 +201,9 @@ struct ima_iint_cache {
>  	struct ima_digest_data *ima_hash;
>  };
>  
> -#define IMA_INVALID_PCR(a) (((a) < 0) || \
> -	(a) >= (sizeof_field(struct ima_iint_cache, measured_pcrs) * 8))
> +#define IMA_INVALID_PCR(a) (((a) < 0) ||				    \
> +	(a) >= (8 * min(sizeof_field(struct ima_iint_cache, measured_pcrs), \
> +			sizeof(ima_extended_pcrs_mask))))
>  
>  
>  extern struct lsm_blob_sizes ima_blob_sizes;
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 0cc1189446a8..6e8a7514d9f6 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -51,6 +51,11 @@ static DEFINE_MUTEX(ima_extend_list_mutex);
>   */
>  static bool ima_measurements_suspended;
>  
> +/*
> + * Set of PCRs ever extended by IMA.
> + */
> +unsigned long ima_extended_pcrs_mask;
> +
>  /* lookup up the digest value in the hash table, and return the entry */
>  static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
>  						       int pcr)
> @@ -144,15 +149,19 @@ unsigned long ima_get_binary_runtime_size(void)
>  
>  static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
>  {
> -	int result = 0;
> +	int result;
>  
>  	if (!ima_tpm_chip)
> -		return result;
> +		return 0;
>  
>  	result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
> -	if (result != 0)
> +	if (result != 0) {
>  		pr_err("Error Communicating to TPM chip, result: %d\n", result);
> -	return result;
> +		return result;
> +	}
> +
> +	ima_extended_pcrs_mask |= BIT(pcr);
> +	return 0;
>  }
>  
>  /*