Re: [PATCH] ima: debugging late_initcall_sync measurements
Yeoreum Yun <[email protected]>
| Newsgroups | org.kernel.vger.linux-integrity,dev.linux.lists.kvmarm,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-security-module |
|---|---|
| Message-ID | <[email protected]> |
Hi Mimi, > On Wed, 2026-05-06 at 06:54 +0100, Yeoreum Yun wrote: > > Hi Mimi, > > > > > On Sun, 2026-05-03 at 07:36 -0400, Mimi Zohar wrote: > > > > On Fri, 2026-05-01 at 12:52 -0400, David Safford wrote: > > > > > On Thu, Apr 30, 2026 at 5:43 PM Mimi Zohar <[email protected]> wrote: > > > > > > > > > > > > On Thu, 2026-04-30 at 10:48 +0100, Yeoreum Yun wrote: > > > > > > > With above change I confirmed there is no meaurement log > > > > > > > between boot_aggregate and boot_aggregate_late except "kernel_version" > > > > > > > But this is ignorable since this UTS measurement is done in > > > > > > > "ima_init_core() (old: ima_init())" and it is part of ima initialisation. > > > > > > > > > > > > > > 1. ima_policy=tcb > > > > > > > > > > > > > > # cat /sys/kernel/security/ima/ascii_runtime_measurements > > > > > > > 10 0adefe762c149c7cec19da62f0da1297fcfbffff ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 boot_aggregate > > > > > > > 10 4e5d73ebadfd8f850cb93ce4de755ba148a9a7d5 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 boot_aggregate_late > > > > > > > 10 7c23cc970eceec906f7a41bc2fbde770d7092209 ima-ng sha256:72ade6ae3d35cfe5ede7a77b1c0ed1d1782a899445fdcb219c0e994a084a70d5 /bin/busybox > > > > > snip > > > > > > > > > > > > > > 2. ima_policy=critical_data > > > > > > > > > > > > > > # cat /sys/kernel/security/ima/ascii_runtime_measurements > > > > > > > 10 0adefe762c149c7cec19da62f0da1297fcfbffff ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 boot_aggregate > > > > > > > 10 49ab61dd97ea2f759edcb6c6a3387ac67f0aa576 ima-buf sha256:0c907aab3261194f16b0c2a422a82f145bc9b9ecb8fdb633fa43e3e5379f0af2 kernel_version 372e312e302d7263312b // Ignorable since it's generated by ima_init(_core)(). > > > > > > > 10 4e5d73ebadfd8f850cb93ce4de755ba148a9a7d5 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 boot_aggregate_late > > > > > > > > > > > > > > Therefore, init_ima() could move into late_initcall_sync like v1 did: > > > > > > > - https://lore.kernel.org/all/[email protected]/ > > > > > > > > > > > > Thanks, Yeoreum. It's a bit premature to claim it's "safe" to move the > > > > > > initcall. Hopefully others will respond. > > > > > > > > > > > > Mimi > > > > > > > > > > I have also run with this patch on a number of bare metal and virtual machines, > > > > > running everything from default Fedora 44 to a version with everything turned on > > > > > (uefi secure boot, UKI with sdboot stub measurements, IMA measurement > > > > > and appraisal enabled, > > > > > all systemd measurements on, and systemd using the TPM for root > > > > > partition decryption.) > > > > > I too see only the kernel_version event between the normal and late > > > > > calls, if ima_policy=critical_data. > > > > > > > > Thanks, Dave! Were all the systems you tested x86_64? The next step would be > > > > to test on different arch's (e.g. Z, Power). > > > > > > On both Z and PowerVM, there are ~30 measurements between boot_aggregate and > > > boot_aggregate_late. For example, on PowerVM: > > > > > > # grep -n boot_aggregate > > > /sys/kernel/security/integrity/ima/ascii_runtime_measurements > > > > > > 1:10 f60a05d7354fb34aabc02965216abd3428ea52bb ima-sig > > > sha256:9887dd089ee19a6517bca10580b02c1bb9aa6cd86c157b6ead8a1c0403f348d5 > > > boot_aggregate > > > 31:10 e2592b0d61da6300d3db447b143897a9792231ea ima-sig > > > sha256:9887dd089ee19a6517bca10580b02c1bb9aa6cd86c157b6ead8a1c0403f348d5 > > > boot_aggregate_late > > > > > > It would be interesting to the results from a Raspberry Pi 5 as well, > > > with/without a TPM. > > > > Honestly, I find this result hard to accept. > > > > This effectively means that there is code invoking IMA measurement during late_initcall(). > > It also implies that if, in the future, a late_initcall is added that performs > > an IMA measurement before IMA initialization has occurred accoding to order by linker, > > that measurement could be missed. > > Exactly. The results are simply from booting with the builtin "tcb" and > "critical_data" policies. > > $ sudo grubby --args="ima_policy=\"tcb|critical_data\"" --update-kernel > /boot/vmlinuz-${SUFFIX} Thanks. but I still wonder what meaasurements there are between boot_aggregate and boot_aggregate_late. Might be there would be key measurements if it takes more than 5 mins before generating boot_aggregate_late but this seems rare. If you don't mind, would you share the contents of the log between boot_aggregate and boot_aggregate_late? since I only get a kernel_version in my environment. And I think we can collect missing measurements before ima_init_core() into another separate list than ima_measurements in ima_add_template_entry() and we can extend them after boot_aggreagate log generation at ima_init_core() then ima initialisation could be moved to late_initcall_sync like (just for a test and share concept): -------&<------- diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 9a1117112fb2..737c1ac8ad77 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -67,6 +67,7 @@ extern int ima_appraise; extern struct tpm_chip *ima_tpm_chip; extern const char boot_aggregate_name[]; extern const char boot_aggregate_late_name[]; +extern bool ima_extend_on; /* IMA event related data */ struct ima_event_data { @@ -107,6 +108,7 @@ struct ima_template_desc { :...skipping... diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 9a1117112fb2..737c1ac8ad77 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -67,6 +67,7 @@ extern int ima_appraise; extern struct tpm_chip *ima_tpm_chip; extern const char boot_aggregate_name[]; extern const char boot_aggregate_late_name[]; +extern bool ima_extend_on; /* IMA event related data */ struct ima_event_data { @@ -107,6 +108,7 @@ struct ima_template_desc { struct ima_template_entry { int pcr; + int violation; struct tpm_digest *digests; struct ima_template_desc *template_desc; /* template descriptor */ u32 template_data_len; @@ -317,6 +319,7 @@ unsigned long ima_get_binary_runtime_size(void); int ima_init_template(void); void ima_init_template_list(void); int __init ima_init_digests(void); +int __init ima_extend_deferred(void); void __init ima_init_reboot_notifier(void); int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, void *lsm_data); diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index edd063b99685..f6a2b53c1dcb 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -149,10 +149,22 @@ int __init ima_init_core(bool late) rc = ima_init_digests(); if (rc != 0) return rc; + + ima_extend_on = true; + rc = ima_add_boot_aggregate(late); /* boot aggregate must be first entry */ if (rc != 0) return rc; + /* This is just for a test. */ + if (!late) + ima_extend_on = false; + else { + rc = ima_extend_deferred(); + if (rc != 0) + return rc; + } + ima_init_policy(); rc = ima_fs_init(); diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 319522450854..81f2ee070fee 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -22,10 +22,13 @@ #define AUDIT_CAUSE_LEN_MAX 32 +bool ima_extend_on; + /* pre-allocated array of tpm_digest structures to extend a PCR */ static struct tpm_digest *digests; LIST_HEAD(ima_measurements); /* list of all measurements */ +static LIST_HEAD(ima_extend_deferred_head); #ifdef CONFIG_IMA_KEXEC static unsigned long binary_runtime_size; #else @@ -91,6 +94,7 @@ static int get_binary_runtime_size(struct ima_template_entry *entry) return size; } + /* ima_add_template_entry helper function: * - Add template entry to the measurement list and hash table, for * all entries except those carried across kexec. @@ -98,7 +102,8 @@ static int get_binary_runtime_size(struct ima_template_entry *entry) * (Called with ima_extend_list_mutex held.) */ static int ima_add_digest_entry(struct ima_template_entry *entry, - bool update_htable) + bool update_htable, + struct list_head *measurements_list) { struct ima_queue_entry *qe; unsigned int key; @@ -111,7 +116,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry, qe->entry = entry; INIT_LIST_HEAD(&qe->later); - list_add_tail_rcu(&qe->later, &ima_measurements); + list_add_tail_rcu(&qe->later, measurements_list); atomic_long_inc(&ima_htable.len); if (update_htable) { @@ -173,6 +178,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, char tpm_audit_cause[AUDIT_CAUSE_LEN_MAX]; int audit_info = 1; int result = 0, tpmresult = 0; + struct list_head *measurements_list; mutex_lock(&ima_extend_list_mutex); @@ -195,15 +201,21 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, } } + + entry->violation = violation; + measurements_list = (ima_extend_on) ? &ima_measurements : + &ima_extend_deferred_head; + result = ima_add_digest_entry(entry, - !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)); + !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE), + measurements_list); if (result < 0) { audit_cause = "ENOMEM"; audit_info = 0; goto out; } - if (violation) /* invalidate pcr */ + if (violation) /* invalidate pcr */ digests_arg = digests; tpmresult = ima_pcr_extend(digests_arg, entry->pcr); @@ -225,7 +237,7 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry) int result = 0; mutex_lock(&ima_extend_list_mutex); - result = ima_add_digest_entry(entry, 0); + result = ima_add_digest_entry(entry, 0, &ima_measurements); mutex_unlock(&ima_extend_list_mutex); return result; } @@ -288,3 +300,23 @@ int __init ima_init_digests(void) return 0; } + +int __init ima_extend_deferred(void) +{ + guard(mutex)(&ima_extend_list_mutex); + struct ima_queue_entry *qe; + struct tpm_digest *digests_arg; + int ret = 0; + + list_for_each_entry(qe, &ima_extend_deferred_head, later) { + digests_arg = (qe->entry->violation) ? digests : qe->entry->digests; + ret = ima_pcr_extend(digests_arg, qe->entry->pcr); + if (ret) + /* TODO: audit? */ + break; + } + + list_splice_tail_init(&ima_extend_deferred_head, &ima_measurements); + + return ret; +} -- Sincerely, Yeoreum Yun