| Message ID | 20241011150522.2697216-1-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: Suspend PCR extends and log appends when rebooting | expand |
On 10/11/24 11:05 AM, Stefan Berger wrote: > To avoid the following types of error messages from the TPM driver, suspend > PCR extends once the 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. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> Some of the code is taken from Tushar's series: https://lore.kernel.org/linux-integrity/20240214153827.1087657-1-tusharsu@linux.microsoft.com/T/#m2d5f23959510ea2ada534febe03beff4a3f97ac7 See patch 6/8. Tushar's series is still needed for carrying the log across kexec properly since without it it can still happen that the state of the PCR 10 does not match with the IMA log if a new measurements is taken after the freezing of the log (currently at 'kexec load') and before the 'kexec exec'.
On Fri, Oct 11, 2024 at 11:40 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > Tushar's series is still needed ... A quick note that Tushar decided to leave Microsoft, but there is a new developer coming up to speed that will be picking up that work as a priority item.
On Fri, 2024-10-11 at 11:05 -0400, Stefan Berger wrote: > To avoid the following types of error messages from the TPM driver, suspend > PCR extends once the 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. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- No need to send a separate post about the patch. Any comments not part of the patch description belong here. > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_init.c | 16 ++++++++++++++++ > security/integrity/ima/ima_queue.c | 27 +++++++++++++++++++++++++++ > 3 files changed, 44 insertions(+) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index c51e24d24d1e..d3f46232b8b9 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -274,6 +274,7 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template); > int ima_restore_measurement_entry(struct ima_template_entry *entry); > int ima_restore_measurement_list(loff_t bufsize, void *buf); > int ima_measurements_show(struct seq_file *m, void *v); > +void ima_measurements_suspend(void); > unsigned long ima_get_binary_runtime_size(void); > int ima_init_template(void); > void ima_init_template_list(void); > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c > index 4e208239a40e..ac630d8d3049 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -16,6 +16,7 @@ > #include <linux/slab.h> > #include <linux/err.h> > #include <linux/ima.h> > +#include <linux/reboot.h> > #include <generated/utsrelease.h> > > #include "ima.h" > @@ -115,6 +116,19 @@ void __init ima_load_x509(void) > } > #endif > > +static int ima_reboot_notify(struct notifier_block *nb, > + unsigned long action, > + void *data) > +{ > + ima_measurements_suspend(); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block ima_reboot_notifier = { > + .notifier_call = ima_reboot_notify, > +}; > + > int __init ima_init(void) > { > int rc; > @@ -152,6 +166,8 @@ int __init ima_init(void) > > ima_init_key_queue(); > > + register_reboot_notifier(&ima_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..b8613b6c1a39 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -44,6 +44,12 @@ struct ima_h_table ima_htable = { > */ > static DEFINE_MUTEX(ima_extend_list_mutex); > > +/* > + * Used internally by the kernel to suspend-resume ima measurements. > + * Protected by ima_extend_list_mutex. > + */ Tushar's original patch incorrectly provided both a function to suspend and resume appending records to the IMA measurement list and extending the TPM PCR. This patch only suspends appending records to the IMA measurement list and extending the TPM PCR. Please replace the word "suspend-resume" in the comment above with just "suspend". > +static bool suspend_ima_measurements; > + > /* 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) > @@ -148,6 +154,13 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) > return result; > } > > +void ima_measurements_suspend(void) > +{ > + mutex_lock(&ima_extend_list_mutex); > + suspend_ima_measurements = true; > + mutex_unlock(&ima_extend_list_mutex); > +} > + > /* > * Add template entry to the measurement list and hash table, and > * extend the pcr. > @@ -176,6 +189,20 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, > } > } > > + /* > + * suspend_ima_measurements will be set if the system is > + * undergoing kexec soft boot to a new kernel. > + * suspending measurements in this short window ensures the > + * consistency of the IMA measurement list during copying > + * of the kexec buffer. > + */ Tushar's patch did not register a reboot notifier. Please update the above comment accordingly. > + if (suspend_ima_measurements) { > + 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) { thanks, Mimi
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index c51e24d24d1e..d3f46232b8b9 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -274,6 +274,7 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template); int ima_restore_measurement_entry(struct ima_template_entry *entry); int ima_restore_measurement_list(loff_t bufsize, void *buf); int ima_measurements_show(struct seq_file *m, void *v); +void ima_measurements_suspend(void); unsigned long ima_get_binary_runtime_size(void); int ima_init_template(void); void ima_init_template_list(void); diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 4e208239a40e..ac630d8d3049 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -16,6 +16,7 @@ #include <linux/slab.h> #include <linux/err.h> #include <linux/ima.h> +#include <linux/reboot.h> #include <generated/utsrelease.h> #include "ima.h" @@ -115,6 +116,19 @@ void __init ima_load_x509(void) } #endif +static int ima_reboot_notify(struct notifier_block *nb, + unsigned long action, + void *data) +{ + ima_measurements_suspend(); + + return NOTIFY_DONE; +} + +static struct notifier_block ima_reboot_notifier = { + .notifier_call = ima_reboot_notify, +}; + int __init ima_init(void) { int rc; @@ -152,6 +166,8 @@ int __init ima_init(void) ima_init_key_queue(); + register_reboot_notifier(&ima_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..b8613b6c1a39 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -44,6 +44,12 @@ struct ima_h_table ima_htable = { */ static DEFINE_MUTEX(ima_extend_list_mutex); +/* + * Used internally by the kernel to suspend-resume ima measurements. + * Protected by ima_extend_list_mutex. + */ +static bool suspend_ima_measurements; + /* 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) @@ -148,6 +154,13 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) return result; } +void ima_measurements_suspend(void) +{ + mutex_lock(&ima_extend_list_mutex); + suspend_ima_measurements = true; + mutex_unlock(&ima_extend_list_mutex); +} + /* * Add template entry to the measurement list and hash table, and * extend the pcr. @@ -176,6 +189,20 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, } } + /* + * suspend_ima_measurements will be set if the system is + * undergoing kexec soft boot to a new kernel. + * suspending measurements in this short window ensures the + * consistency of the IMA measurement list during copying + * of the kexec buffer. + */ + if (suspend_ima_measurements) { + 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) {