| Message ID | 20241112165206.756351-1-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] ima: Suspend PCR extends and log appends when rebooting | expand |
On Tue Nov 12, 2024 at 6:52 PM EET, Stefan Berger wrote: > 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 > > 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 in > 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 <tusharsu@linux.microsoft.com> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > --- > v2: > - followed Mimi's suggestions > > --- > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_init.c | 2 ++ > security/integrity/ima/ima_queue.c | 43 ++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 3c323ca213d4..3f1a82b7cd71 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..9b3c9587313f 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -16,6 +16,7 @@ > */ > > #include <linux/rculist.h> > +#include <linux/reboot.h> > #include <linux/slab.h> > #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) > @@ -176,6 +183,17 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, > } > } > > + /* > + * ima_measurements_suspended will be set before the TPM subsystem has > + * been shut down. > + */ > + if (ima_measurements_suspended) { > + audit_cause = "measurements_suspended"; > + audit_info = 0; > + result = -ENODEV; > + goto out; > + } > + > result = ima_add_digest_entry(entry, > !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)); > if (result < 0) { > @@ -211,6 +229,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; Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
On Tue, 2024-11-12 at 11:52 -0500, Stefan Berger wrote: > 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 > > 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 in > 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 <tusharsu@linux.microsoft.com> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> Thanks, Stefan. The patch looks good. Based on the updated patch description, I'm wondering if we should be testing the "system_state" instead of registering a reboot notifier? > > --- > v2: > - followed Mimi's suggestions > > --- > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_init.c | 2 ++ > security/integrity/ima/ima_queue.c | 43 ++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 3c323ca213d4..3f1a82b7cd71 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..9b3c9587313f 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -16,6 +16,7 @@ > */ > > #include <linux/rculist.h> > +#include <linux/reboot.h> > #include <linux/slab.h> > #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) > @@ -176,6 +183,17 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, > } > } > > + /* > + * ima_measurements_suspended will be set before the TPM subsystem has > + * been shut down. > + */ The comment should indicate that the system itself is being shut down/rebooted as well. Mimi > + if (ima_measurements_suspended) { > + audit_cause = "measurements_suspended"; > + audit_info = 0; > + result = -ENODEV; > + goto out; > + } > + > result = ima_add_digest_entry(entry, > !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)); > if (result < 0) { > @@ -211,6 +229,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;
On 11/12/24 6:42 PM, Mimi Zohar wrote: > On Tue, 2024-11-12 at 11:52 -0500, Stefan Berger wrote: >> 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 >> >> 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 in >> 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 <tusharsu@linux.microsoft.com> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > Thanks, Stefan. The patch looks good. Based on the updated patch description, > I'm wondering if we should be testing the "system_state" instead of registering > a reboot notifier? That's a possibility and would definitely be less code. I don't see why not...
On 11/12/24 9:28 PM, Stefan Berger wrote: > > > On 11/12/24 6:42 PM, Mimi Zohar wrote: >> On Tue, 2024-11-12 at 11:52 -0500, Stefan Berger wrote: >>> 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 >>> >>> 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 in >>> 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 <tusharsu@linux.microsoft.com> >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> >> Thanks, Stefan. The patch looks good. Based on the updated patch >> description, >> I'm wondering if we should be testing the "system_state" instead of >> registering >> a reboot notifier? > > That's a possibility and would definitely be less code. I don't see why > not... > ... the missing synchronization with the mutex speaks against it. If we don't have it we could try to use the TPM subsystem after it's been shut down.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 3c323ca213d4..3f1a82b7cd71 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..9b3c9587313f 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -16,6 +16,7 @@ */ #include <linux/rculist.h> +#include <linux/reboot.h> #include <linux/slab.h> #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) @@ -176,6 +183,17 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, } } + /* + * ima_measurements_suspended will be set before the TPM subsystem has + * been shut down. + */ + if (ima_measurements_suspended) { + audit_cause = "measurements_suspended"; + audit_info = 0; + result = -ENODEV; + goto out; + } + result = ima_add_digest_entry(entry, !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)); if (result < 0) { @@ -211,6 +229,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;