From 254ef9541d68bd9d75296b2487ec97d4d6d40d57 Mon Sep 17 00:00:00 2001 From: Stefan Berger Date: Mon, 18 Nov 2024 09:57:31 -0500 Subject: ima: Suspend PCR extends and log appends when rebooting To avoid the following types of error messages due to a failure by the TPM driver to use the TPM, suspend TPM PCR extensions and the appending of entries to the IMA log once IMA's reboot notifier has been called. This avoids trying to use the TPM after the TPM subsystem has been shut down. [111707.685315][ T1] ima: Error Communicating to TPM chip, result: -19 [111707.685960][ T1] ima: Error Communicating to TPM chip, result: -19 Synchronization with the ima_extend_list_mutex to set ima_measurements_suspended ensures that the TPM subsystem is not shut down when IMA holds the mutex while appending to the log and extending the PCR. The alternative of reading the system_state variable would not provide this guarantee. This error could be observed on a ppc64 machine running SuSE Linux where processes are still accessing files after devices have been shut down. Suspending the IMA log and PCR extensions shortly before reboot does not seem to open a significant measurement gap since neither TPM quoting would work for attestation nor that new log entries could be written to anywhere after devices have been shut down. However, there's a time window between the invocation of the reboot notifier and the shutdown of devices. This includes all subsequently invoked reboot notifiers as well as kernel_restart_prepare() where __usermodehelper_disable() waits for all running_helpers to exit. During this time window IMA could now miss log entries even though attestation would still work. The reboot of the system shortly after may make this small gap insignificant. Signed-off-by: Tushar Sugandhi Signed-off-by: Stefan Berger Reviewed-by: Jarkko Sakkinen Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_init.c | 2 ++ security/integrity/ima/ima_queue.c | 44 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) (limited to 'security') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index c0d3b716d11f..24d09ea91b87 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -278,6 +278,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); +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 4e208239a40e..a2f34f2d8ad7 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -152,6 +152,8 @@ int __init ima_init(void) ima_init_key_queue(); + ima_init_reboot_notifier(); + ima_measure_critical_data("kernel_info", "kernel_version", UTS_RELEASE, strlen(UTS_RELEASE), false, NULL, 0); diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 532da87ce519..83d53824aa98 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -16,6 +16,7 @@ */ #include +#include #include #include "ima.h" @@ -44,6 +45,12 @@ struct ima_h_table ima_htable = { */ static DEFINE_MUTEX(ima_extend_list_mutex); +/* + * Used internally by the kernel to suspend measurements. + * Protected by ima_extend_list_mutex. + */ +static bool ima_measurements_suspended; + /* 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) @@ -168,6 +175,18 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, int result = 0, tpmresult = 0; mutex_lock(&ima_extend_list_mutex); + + /* + * Avoid appending to the measurement log when the TPM subsystem has + * been shut down while preparing for system reboot. + */ + if (ima_measurements_suspended) { + audit_cause = "measurements_suspended"; + audit_info = 0; + result = -ENODEV; + goto out; + } + if (!violation && !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)) { if (ima_lookup_digest_entry(digest, entry->pcr)) { audit_cause = "hash_exists"; @@ -211,6 +230,31 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry) return result; } +static void ima_measurements_suspend(void) +{ + mutex_lock(&ima_extend_list_mutex); + ima_measurements_suspended = true; + mutex_unlock(&ima_extend_list_mutex); +} + +static int ima_reboot_notifier(struct notifier_block *nb, + unsigned long action, + void *data) +{ + ima_measurements_suspend(); + + return NOTIFY_DONE; +} + +static struct notifier_block ima_reboot_nb = { + .notifier_call = ima_reboot_notifier, +}; + +void __init ima_init_reboot_notifier(void) +{ + register_reboot_notifier(&ima_reboot_nb); +} + int __init ima_init_digests(void) { u16 digest_size; -- cgit v1.2.3 From 68af44a71975688b881ea524e2526bb7c7ad0e9a Mon Sep 17 00:00:00 2001 From: Breno Leitao Date: Thu, 21 Nov 2024 01:57:12 -0800 Subject: ima: kexec: silence RCU list traversal warning The ima_measurements list is append-only and doesn't require rcu_read_lock() protection. However, lockdep issues a warning when traversing RCU lists without the read lock: security/integrity/ima/ima_kexec.c:40 RCU-list traversed in non-reader section!! Fix this by using the variant of list_for_each_entry_rcu() with the last argument set to true. This tells the RCU subsystem that traversing this append-only list without the read lock is intentional and safe. This change silences the lockdep warning while maintaining the correct semantics for the append-only list traversal. Signed-off-by: Breno Leitao Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_kexec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 52e00332defe..9d45f4d26f73 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -37,7 +37,8 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, memset(&khdr, 0, sizeof(khdr)); khdr.version = 1; - list_for_each_entry_rcu(qe, &ima_measurements, later) { + /* This is an append-only list, no need to hold the RCU read lock */ + list_for_each_entry_rcu(qe, &ima_measurements, later, true) { if (file.count < file.size) { khdr.count++; ima_measurements_show(&file, qe); -- cgit v1.2.3 From 7eef7c8bac9a31f12ae19369582bc25971bc8fe1 Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Thu, 26 Dec 2024 21:27:42 -0500 Subject: ima: limit the builtin 'tcb' dont_measure tmpfs policy rule With a custom policy similar to the builtin IMA 'tcb' policy [1], arch specific policy, and a kexec boot command line measurement policy rule, the kexec boot command line is not measured due to the dont_measure tmpfs rule. Limit the builtin 'tcb' dont_measure tmpfs policy rule to just the "func=FILE_CHECK" hook. Depending on the end users security threat model, a custom policy might not even include this dont_measure tmpfs rule. Note: as a result of this policy rule change, other measurements might also be included in the IMA-measurement list that previously weren't included. [1] https://ima-doc.readthedocs.io/en/latest/ima-policy.html#ima-tcb Reviewed-by: Petr Vorel Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 21a8e54c383f..23bbe2c405f0 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -148,7 +148,8 @@ static struct ima_rule_entry dont_measure_rules[] __ro_after_init = { {.action = DONT_MEASURE, .fsmagic = PROC_SUPER_MAGIC, .flags = IMA_FSMAGIC}, {.action = DONT_MEASURE, .fsmagic = SYSFS_MAGIC, .flags = IMA_FSMAGIC}, {.action = DONT_MEASURE, .fsmagic = DEBUGFS_MAGIC, .flags = IMA_FSMAGIC}, - {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC}, + {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .func = FILE_CHECK, + .flags = IMA_FSMAGIC | IMA_FUNC}, {.action = DONT_MEASURE, .fsmagic = DEVPTS_SUPER_MAGIC, .flags = IMA_FSMAGIC}, {.action = DONT_MEASURE, .fsmagic = BINFMTFS_MAGIC, .flags = IMA_FSMAGIC}, {.action = DONT_MEASURE, .fsmagic = SECURITYFS_MAGIC, .flags = IMA_FSMAGIC}, -- cgit v1.2.3 From 4785ed362a24d4f37ee0eb4403f587fee886f8da Mon Sep 17 00:00:00 2001 From: Mimi Zohar Date: Fri, 27 Dec 2024 08:28:32 -0500 Subject: ima: ignore suffixed policy rule comments Lines beginning with '#' in the IMA policy are comments and are ignored. Instead of placing the rule and comment on separate lines, allow the comment to be suffixed to the IMA policy rule. Reviewed-by: Petr Vorel Reviewed-by: Jarkko Sakkinen Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 23bbe2c405f0..128fab897930 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -1432,7 +1432,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int token; unsigned long lnum; - if (result < 0) + if (result < 0 || *p == '#') /* ignore suffixed comment */ break; if ((*p == '\0') || (*p == ' ') || (*p == '\t')) continue; -- cgit v1.2.3