Re: [PATCH] efi/tpm: Fix the issue where the CC platforms event log header can't be correctly identified

[Ge Yang <yangge1116@126.com>]

在 2025/7/4 23:13, Jarkko Sakkinen 写道:
> On Fri, Jul 04, 2025 at 10:53:54AM +0800, Ge Yang wrote:
>>
>>
>> 在 2025/7/4 9:50, Jarkko Sakkinen 写道:
>>> On Thu, Jul 03, 2025 at 10:38:37AM +0800, yangge1116@126.com wrote:
>>>> From: Ge Yang <yangge1116@126.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.
> 
> Thanks for digging that out. Can you add something to the commit
> message? That spec is common knowledge if you are "into the topic"
> in the first palace so something along the lines of this would be
> perfectly fine:
> 
> "The check can be safely removed, as ccording to table 6 at section
> 10.2.1 of TCG PC client specification the index field does not require
> fixing the PCR index to zero."
> 
Ok, thanks.

> But then: we still have that constraint there and we cannot predict the
> side-effects of removing a constraint, even if it is incorrectly defined
> constraint. For comparison, it's much less risky situation when adding
> additional constraints, as possible side-effects in the worst case
> scenarios can be even theoretically much lighter than in the opposite
> situation.
> 
> For this reasons it would be perhaps better to limit the fix for the
> CC only, and not change the semantics across the board.
> 
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.


> BR, Jarkko