Re: [PATCH v3] ima: Suspend PCR extends and log appends when rebooting

[Mimi Zohar <zohar@linux.ibm.com>]

On Mon, 2024-11-18 at 09:57 -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> 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 <tusharsu@linux.microsoft.com>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Thank you for updating the patch description and the comment below.  The patch
is now queued in next-integrity-testing.

Mimi

> ---

[...] 
> 						       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;
>  }