Re: [PATCH] efi: tpm: Avoid READ_ONCE() for accessing the event log

From: Nathan Chancellor <nathan@kernel.org>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org, linux-efi@vger.kernel.org,
	will@kernel.org, catalin.marinas@arm.com, stable@vger.kernel.org,
	Peter Jones <pjones@redhat.com>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	Matthew Garrett <mjg59@srcf.ucam.org>
Subject: Re: [PATCH] efi: tpm: Avoid READ_ONCE() for accessing the event log
Date: Mon, 9 Jan 2023 10:48:47 -0700	[thread overview]
Message-ID: <Y7xTf1SEPTXiCoPM@dev-arch.thelio-3990X> (raw)
In-Reply-To: <20230109095948.2471205-1-ardb@kernel.org>

On Mon, Jan 09, 2023 at 10:59:48AM +0100, Ard Biesheuvel wrote:
> Nathan reports that recent kernels built with LTO will crash when doing
> EFI boot using Fedora's GRUB and SHIM. The culprit turns out to be a
> misaligned load from the TPM event log, which is annotated with
> READ_ONCE(), and under LTO, this gets translated into a LDAR instruction
> which does not tolerate misaligned accesses.
> 
> Interestingly, this does not happen when booting the same kernel
> straight from the UEFI shell, and so the fact that the event log may
> appear misaligned in memory may be caused by a bug in GRUB or SHIM.
> 
> However, using READ_ONCE() to access firmware tables is slightly unusual
> in any case, and here, we only need to ensure that 'event' is not
> dereferenced again after it gets unmapped, so a compiler barrier should
> be sufficient, and works around the reported issue.
> 
> Cc: <stable@vger.kernel.org>
> Cc: Peter Jones <pjones@redhat.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1782
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Based on the thread, I tested this patch without barrier() and my
machine boots up just fine now with an LTO kernel. Thanks a lot for the
analysis and fix!

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  include/linux/tpm_eventlog.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
> index 20c0ff54b7a0d313..0abcc85904cba874 100644
> --- a/include/linux/tpm_eventlog.h
> +++ b/include/linux/tpm_eventlog.h
> @@ -198,8 +198,10 @@ static __always_inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *ev
>  	 * The loop below will unmap these fields if the log is larger than
>  	 * one page, so save them here for reference:
>  	 */
> -	count = READ_ONCE(event->count);
> -	event_type = READ_ONCE(event->event_type);
> +	count = event->count;
> +	event_type = event->event_type;
> +
> +	barrier();
>  
>  	/* Verify that it's the log header */
>  	if (event_header->pcr_idx != 0 ||
> -- 
> 2.39.0
> 