Re: [RFC PATCH v3 2/4] security: ima: call ima_init() again at late_initcall_sync for defered TPM

[Yeoreum Yun <yeoreum.yun@arm.com>]

> From: Jonathan McDowell <noodles@meta.com>
>
> The Linux IMA (Integrity Measurement Architecture) subsystem used for
> secure boot, file integrity, or remote attestation cannot be a loadable
> module for few reasons listed below:
>
>  o Boot-Time Integrity: IMA’s main role is to measure and appraise files
>    before they are used. This includes measuring critical system files
>    during early boot (e.g., init, init scripts, login binaries). If IMA
>    were a module, it would be loaded too late to cover those.
>
>  o TPM Dependency: IMA integrates tightly with the TPM to record
>    measurements into PCRs. The TPM must be initialized early (ideally
>    before init_ima()), which aligns with IMA being built-in.
>
>  o Security Model: IMA is part of a Trusted Computing Base (TCB). Making
>    it a module would weaken the security model, as a potentially
>    compromised system could delay or tamper with its initialization.
>
> IMA must be built-in to ensure it starts measuring from the earliest
> possible point in boot which inturn implies TPM must be initialised and
> ready to use before IMA.
>
> Unfortunately some TPM drivers (such as Arm FF-A, or SPI attached TPM
> devices) are not reliably available during the initcall_late stage,
> resulting in a log error:
>
>   ima: No TPM chip found, activating TPM-bypass!
>
> and no measurements into the TPM by IMA. We can avoid this by doing IMA
> init in the initcall_late_sync stage, after the drivers have completed
> their init + registration.
>
> Rather than do this everywhere, and needlessly delay the initialisation
> of IMA when there is no need to do so, we continue to try to initialise
> at the earlier stage, only deferring to the later point if the TPM is
> not available yet.
>
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
>  security/integrity/ima/ima.h              |  3 +-
>  security/integrity/ima/ima_init.c         | 25 ++++++++-------
>  security/integrity/ima/ima_main.c         | 37 ++++++++++++++++++++---
>  security/integrity/ima/ima_template_lib.c |  3 +-
>  4 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 89ebe98ffc5e..b3677b403a5a 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -65,6 +65,7 @@ extern struct ima_algo_desc *ima_algo_array __ro_after_init;
>  extern int ima_appraise;
>  extern struct tpm_chip *ima_tpm_chip;
>  extern const char boot_aggregate_name[];
> +extern const char boot_aggregate_late_name[];
>
>  /* IMA event related data */
>  struct ima_event_data {
> @@ -257,7 +258,7 @@ static inline void ima_measure_kexec_event(const char *event_name) {}
>  extern bool ima_canonical_fmt;
>
>  /* Internal IMA function definitions */
> -int ima_init(void);
> +int ima_init_core(bool late);
>  int ima_fs_init(void);
>  int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>  			   const char *op, struct inode *inode,
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index a2f34f2d8ad7..5f335834a9bb 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -22,6 +22,7 @@
>
>  /* name for boot aggregate entry */
>  const char boot_aggregate_name[] = "boot_aggregate";
> +const char boot_aggregate_late_name[] = "boot_aggregate_late";
>  struct tpm_chip *ima_tpm_chip;
>
>  /* Add the boot aggregate to the IMA measurement list and extend
> @@ -39,17 +40,17 @@ struct tpm_chip *ima_tpm_chip;
>   * a different value.) Violations add a zero entry to the measurement
>   * list and extend the aggregate PCR value with ff...ff's.
>   */
> -static int __init ima_add_boot_aggregate(void)
> +static int __init ima_add_boot_aggregate(bool late)
>  {
>  	static const char op[] = "add_boot_aggregate";
>  	const char *audit_cause = "ENOMEM";
>  	struct ima_template_entry *entry;
>  	struct ima_iint_cache tmp_iint, *iint = &tmp_iint;
> -	struct ima_event_data event_data = { .iint = iint,
> -					     .filename = boot_aggregate_name };
> +	struct ima_event_data event_data = { .iint = iint };
>  	struct ima_max_digest_data hash;
>  	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
>  						struct ima_digest_data, hdr);
> +	const char *filename;
>  	int result = -ENOMEM;
>  	int violation = 0;
>
> @@ -59,6 +60,12 @@ static int __init ima_add_boot_aggregate(void)
>  	iint->ima_hash->algo = ima_hash_algo;
>  	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
>
> +	if (late)
> +		filename = boot_aggregate_late_name;
> +	else
> +		filename = boot_aggregate_name;
> +	event_data.filename = filename;
> +
>  	/*
>  	 * With TPM 2.0 hash agility, TPM chips could support multiple TPM
>  	 * PCR banks, allowing firmware to configure and enable different
> @@ -86,7 +93,7 @@ static int __init ima_add_boot_aggregate(void)
>  	}
>
>  	result = ima_store_template(entry, violation, NULL,
> -				    boot_aggregate_name,
> +				    filename,
>  				    CONFIG_IMA_MEASURE_PCR_IDX);
>  	if (result < 0) {
>  		ima_free_template_entry(entry);
> @@ -95,7 +102,7 @@ static int __init ima_add_boot_aggregate(void)
>  	}
>  	return 0;
>  err_out:
> -	integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, boot_aggregate_name, op,
> +	integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, filename, op,
>  			    audit_cause, result, 0);
>  	return result;
>  }
> @@ -115,14 +122,10 @@ void __init ima_load_x509(void)
>  }
>  #endif
>
> -int __init ima_init(void)
> +int __init ima_init_core(bool late)
>  {
>  	int rc;
>
> -	ima_tpm_chip = tpm_default_chip();
> -	if (!ima_tpm_chip)
> -		pr_info("No TPM chip found, activating TPM-bypass!\n");
> -
>  	rc = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
>  	if (rc)
>  		return rc;
> @@ -140,7 +143,7 @@ int __init ima_init(void)
>  	rc = ima_init_digests();
>  	if (rc != 0)
>  		return rc;
> -	rc = ima_add_boot_aggregate();	/* boot aggregate must be first entry */
> +	rc = ima_add_boot_aggregate(late);	/* boot aggregate must be first entry */
>  	if (rc != 0)
>  		return rc;
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 1d6229b156fb..0b93a286c0d3 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1237,7 +1237,7 @@ static int ima_kernel_module_request(char *kmod_name)
>
>  #endif /* CONFIG_INTEGRITY_ASYMMETRIC_KEYS */
>
> -static int __init init_ima(void)
> +static int __init init_ima(bool late)
>  {
>  	int error;
>
> @@ -1247,10 +1247,26 @@ static int __init init_ima(void)
>  		return 0;
>  	}
>
> +	/*
> +	 * If we found the TPM during our first attempt, or we know there's no
> +	 * TPM, nothing further to do
> +	 */
> +	if (late && (ima_tpm_chip || !IS_ENABLED(CONFIG_TCG_TPM)))
> +		return 0;
> +
> +	ima_tpm_chip = tpm_default_chip();
> +	if (!ima_tpm_chip && !late && IS_ENABLED(CONFIG_TCG_TPM)) {
> +		pr_debug("TPM not available, will try later\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	if (!ima_tpm_chip)
> +		pr_info("No TPM chip found, activating TPM-bypass!\n");
> +
>  	ima_appraise_parse_cmdline();
>  	ima_init_template_list();
>  	hash_setup(CONFIG_IMA_DEFAULT_HASH);
> -	error = ima_init();
> +	error = ima_init_core(late);
>
>  	if (error && strcmp(hash_algo_name[ima_hash_algo],
>  			    CONFIG_IMA_DEFAULT_HASH) != 0) {
> @@ -1258,7 +1274,7 @@ static int __init init_ima(void)
>  			hash_algo_name[ima_hash_algo], CONFIG_IMA_DEFAULT_HASH);
>  		hash_setup_done = 0;
>  		hash_setup(CONFIG_IMA_DEFAULT_HASH);
> -		error = ima_init();
> +		error = ima_init_core(late);
>  	}
>
>  	if (error)
> @@ -1274,6 +1290,16 @@ static int __init init_ima(void)
>  	return error;
>  }
>
> +static int __init init_ima_late(void)
> +{
> +	return init_ima(false);
> +}
> +
> +static int __init init_ima_late_sync(void)
> +{
> +	return init_ima(true);
> +}
> +
>  static struct security_hook_list ima_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(bprm_check_security, ima_bprm_check),
>  	LSM_HOOK_INIT(bprm_creds_for_exec, ima_bprm_creds_for_exec),
> @@ -1319,6 +1345,7 @@ DEFINE_LSM(ima) = {
>  	.init = init_ima_lsm,
>  	.order = LSM_ORDER_LAST,
>  	.blobs = &ima_blob_sizes,
> -	/* Start IMA after the TPM is available */
> -	.initcall_late = init_ima,
> +	/* Ensure we start IMA after the TPM is available */
> +	.initcall_late = init_ima_late,
> +	.initcall_late_sync = init_ima_late_sync,
>  };
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 0e627eac9c33..8a89236f926c 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -363,7 +363,8 @@ int ima_eventdigest_init(struct ima_event_data *event_data,
>  		goto out;
>  	}
>
> -	if ((const char *)event_data->filename == boot_aggregate_name) {
> +	if ((const char *)event_data->filename == boot_aggregate_name ||
> +	    (const char *)event_data->filename == boot_aggregate_late_name) {
>  		if (ima_tpm_chip) {
>  			hash.hdr.algo = HASH_ALGO_SHA1;
>  			result = ima_calc_boot_aggregate(hash_hdr);
> --
> 2.53.0
>

This looks good to me. Feel free to add:
Reviewed-by: Yeoreum Yun <yeoreum.yun@arm.com>

Thanks!

--
Sincerely,
Yeoreum Yun