linux-kernel - Re: [PATCH v2] ima: Suspend PCR extends and log appends when rebooting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a616939fa13b9e01b9cb6be68246152772944a76.camel@linux.ibm.com>
Date: Tue, 12 Nov 2024 18:42:37 -0500
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Stefan Berger <stefanb@...ux.ibm.com>, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, roberto.sassu@...wei.com,
        Tushar Sugandhi
	 <tusharsu@...ux.microsoft.com>
Subject: Re: [PATCH v2] ima: Suspend PCR extends and log appends when
 rebooting

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@...ux.microsoft.com>
> Signed-off-by: Stefan Berger <stefanb@...ux.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;