Message-ID: <17ce8192-6dea-465d-bedd-0333e83c9792@126.com>
Date: Sat, 5 Jul 2025 14:59:23 +0800
From: Ge Yang <yangge1116@....com>
To: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com>,
 Jarkko Sakkinen <jarkko@...nel.org>
Cc: ardb@...nel.org, ilias.apalodimas@...aro.org, jgg@...pe.ca,
 linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org, liuzixing@...on.cn
Subject: Re: [PATCH] efi/tpm: Fix the issue where the CC platforms event log
 header can't be correctly identified



在 2025/7/5 6:27, Sathyanarayanan Kuppuswamy 写道:
> 
> On 7/3/25 7:53 PM, Ge Yang wrote:
>>
>>
>> 在 2025/7/4 9:50, Jarkko Sakkinen 写道:
>>> On Thu, Jul 03, 2025 at 10:38:37AM +0800, yangge1116@....com wrote:
>>>> From: Ge Yang <yangge1116@....com>
>>>>
>>>> Since commit d228814b1913 ("efi/libstub: Add get_event_log() support
>>>> for CC platforms") reuses TPM2 support code for the CC platforms, when
>>>> launching a TDX virtual machine with coco measurement enabled, the
>>>> following error log is generated:
>>>>
>>>> [Firmware Bug]: Failed to parse event in TPM Final Events Log
>>>>
>>>> Call Trace:
>>>> efi_config_parse_tables()
>>>>    efi_tpm_eventlog_init()
>>>>      tpm2_calc_event_log_size()
>>>>        __calc_tpm2_event_size()
>>>>
>>>> The pcr_idx value in the Intel TDX log header is 1, causing the
>>>> function __calc_tpm2_event_size() to fail to recognize the log header,
>>>> ultimately leading to the "Failed to parse event in TPM Final Events
>>>> Log" error.
>>>>
>>>> According to UEFI Spec 2.10 Section 38.4.1: For Tdx, TPM PCR 0 maps to
>>>> MRTD, so the log header uses TPM PCR 1. To successfully parse the TDX
>>>> event log header, the check for a pcr_idx value of 0 has been removed
>>>> here, and it appears that this will not affect other functionalities.
>>>
>>> I'm not familiar with the original change but with a quick check it did
>>> not change __calc_tpm2_event_size(). Your change is changing semantics
>>> to two types of callers:
>>>
>>> 1. Those that caused the bug.
>>> 2. Those that nothing to do with this bug.
>>>
>>> I'm not seeing anything explaining that your change is guaranteed not to
>>> have any consequences to "innocent" callers, which have no relation to
>>> the bug.
>>>
>>
>> Thank you for your response.
>>
>> According to Section 10.2.1, Table 6 (TCG_PCClientPCREvent Structure) 
>> in the TCG PC Client Platform Firmware Profile Specification, 
>> determining whether an event is an event log header does not require 
>> checking the pcrIndex field. The identification can be made based on 
>> other fields alone. Therefore, removing the pcrIndex check here is 
>> considered safe
>> for "innocent" callers.
>>
>> Reference Link: https://trustedcomputinggroup.org/wp-content/uploads/ 
>> TCG_PCClient_PFP_r1p05_v23_pub.pdf
> 
> 
> It looks like this check was originally added to handle a case which 
> does not align with the TCG spec. So
> removing it directly may have some impact to these older platform. Can 
> we make this conditional to
> CC platform?

Ok, thanks.

I originally intended to add a 
cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) check inside the 
__calc_tpm2_event_size() function to limit the fix to Confidential 
Computing (CC) platforms only. However, this function is also used 
during the EFI stub phase, where the cc_platform_has() function is not 
defined.

Call Trace:
efi_pe_entry()
   efi_stub_entry()
     efi_retrieve_eventlog()
       efi_retrieve_tcg2_eventlog()
         __calc_tpm2_event_size()

Therefore, I plan to modify __calc_tpm2_event_size() to accept an 
additional parameter indicating whether the event originates from a CC 
platform.

> 
> commit 7dfc06a0f25b593a9f51992f540c0f80a57f3629
> Author: Fabian Vogt <fvogt@...e.de>
> Date:   Mon Jun 15 09:16:36 2020 +0200
> 
>      efi/tpm: Verify event log header before parsing
> 
>      It is possible that the first event in the event log is not actually a
>      log header at all, but rather a normal event. This leads to the 
> cast in
>      __calc_tpm2_event_size being an invalid conversion, which means that
>      the values read are effectively garbage. Depending on the first 
> event's
>      contents, this leads either to apparently normal behaviour, a crash or
>      a freeze.
> 
>      While this behaviour of the firmware is not in accordance with the
>      TCG Client EFI Specification, this happens on a Dell Precision 5510
>      with the TPM enabled but hidden from the OS ("TPM On" disabled, state
>      otherwise untouched). The EFI firmware claims that the TPM is present
>      and active and that it supports the TCG 2.0 event log format.
> 
>      Fortunately, this can be worked around by simply checking the header
>      of the first event and the event log header signature itself.
> 
>      Commit b4f1874c6216 ("tpm: check event log version before reading 
> final
>      events") addressed a similar issue also found on Dell models.
> 
>      Fixes: 6b0326190205 ("efi: Attempt to get the TCG2 event log in the 
> boot stub")
>      Signed-off-by: Fabian Vogt <fvogt@...e.de>
>      Link: https://lore.kernel.org/r/1927248.evlx2EsYKh@linux-e202.suse.de
>      Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1165773
>      Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
> 
> 
>>>>
>>>> Link: https://uefi.org/specs/ 
>>>> UEFI/2.10/38_Confidential_Computing.html#intel-trust-domain-extension
>>>> Fixes: d228814b1913 ("efi/libstub: Add get_event_log() support for 
>>>> CC platforms")
>>>> Signed-off-by: Ge Yang <yangge1116@....com>
>>>> Cc: stable@...r.kernel.org
>>>> ---
>>>>   include/linux/tpm_eventlog.h | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/tpm_eventlog.h b/include/linux/ 
>>>> tpm_eventlog.h
>>>> index 891368e..05c0ae5 100644
>>>> --- a/include/linux/tpm_eventlog.h
>>>> +++ b/include/linux/tpm_eventlog.h
>>>> @@ -202,8 +202,7 @@ static __always_inline u32 
>>>> __calc_tpm2_event_size(struct tcg_pcr_event2_head *ev
>>>>       event_type = event->event_type;
>>>>         /* Verify that it's the log header */
>>>> -    if (event_header->pcr_idx != 0 ||
>>>> -        event_header->event_type != NO_ACTION ||
>>>> +    if (event_header->event_type != NO_ACTION ||
>>>>           memcmp(event_header->digest, zero_digest, 
>>>> sizeof(zero_digest))) {
>>>>           size = 0;
>>>>           goto out;
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>> BR, Jarkko
>>

Powered by blists - more mailing lists