On 3/7/24 7:41 AM, Ard Biesheuvel wrote:
> On Thu, 7 Mar 2024 at 16:36, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
>>
>> On 3/7/24 3:30 AM, Ard Biesheuvel wrote:
>>> On Thu, 7 Mar 2024 at 12:13, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>>>> On Thu, 7 Mar 2024 at 12:08, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>>>>> Hi Muhammad,
>>>>>
>>>>> Thanks for the report.
>>>>>
>>>>> On Thu, 7 Mar 2024 at 12:02, Muhammad Usama Anjum
>>>>> <usama.anjum@xxxxxxxxxxxxx> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> The recent patch:
>>>>>> 276805fb9c305: efi/libstub: Add get_event_log() support for CC platforms
>>>>>> has introduced
>>>>>> #define EFI_CC_EVENT_LOG_FORMAT_TCG_2 0x00000002
>>>>>>
>>>>>> But EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 has the same numerical value:
>>>>>> #define EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 0x2
>>>>>>
>>>>>> Thus there is dead code in efi_retrieve_tcg2_eventlog() i.e, multiple if
>>>>>> conditions with (version == 2) I'm unable to decide on what is wrong and
>>>>>> what is right here. Please have a look.
>>>>>>
>>>>> Why is this a problem? The compiler will recognize this and simplify
>>>>> the conditional. The code as written is semantically correct, the fact
>>>>> that the symbolic constants resolve to the same numerical value is
>>>>> just an implementation detail.
>>>> Ah hold on. I see what you mean now:
>>>>
>>>> if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
>>>> final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
>>>> + else if (version == EFI_CC_EVENT_LOG_FORMAT_TCG_2)
>>>> + final_events_table = get_efi_config_table(LINUX_EFI_CC_FINAL_LOG_GUID);
>>>>
>>>> Yes, that is broken.
>>> Could we fix it like this perhaps?
>>>
>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>> @@ -75,8 +75,7 @@
>>> *
>>> * CC Event log also uses TCG2 format, handle it same as TPM2.
>>> */
>>> if (version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2) {
>>> /*
>>> * The TCG2 log format has variable length entries,
>>> * and the information to decode the hash algorithms
>>> @@ -109,10 +108,11 @@
>>> * Figure out whether any events have already been logged to the
>>> * final events structure, and if so how much space they take up
>>> */
>>> if (version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2)
>>> final_events_table =
>>> get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID) ?:
>>> get_efi_config_table(LINUX_EFI_CC_FINAL_LOG_GUID);
>> Looks good to me. If there is a concern that a TPM config table may exists
>> even in CC case, we can get the table pointer under protocol check and pass
>> it as argument to this function.
>>
> Yeah, that makes sense. I'll fold this into the patch too.
>
> I did realize, though, that we are still missing some pieces to allow
> the kernel to make use of this. Only the TCG2 final events log is
> accessed by the kernel proper, and AFAICT, this all happens in the
> context of a char driver attached to a TPM device.
>
> So I'd like to understand if there are any plans to follow up?
>
>
There is a discussion in the following thread about convergence between
RTMR and TPM for eventlog extension and access to firmware tables. I think
we will have follow up patches once we decide on the correct direction.
https://lore.kernel.org/lkml/b20a4dc2-2794-46a9-b3d5-41e06ce22e34@xxxxxxxxx/#t
>
>
>
>> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
>> index c35f99f259c1..2ef1e4cd6bb2 100644
>> --- a/drivers/firmware/efi/libstub/tpm.c
>> +++ b/drivers/firmware/efi/libstub/tpm.c
>> @@ -49,12 +49,12 @@ void efi_enable_reset_attack_mitigation(void)
>>
>> static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_location,
>> efi_physical_addr_t log_last_entry,
>> - efi_bool_t truncated)
>> + efi_bool_t truncated,
>> + struct efi_tcg2_final_events_table *final_events_table)
>> {
>> efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID;
>> efi_status_t status;
>> struct linux_efi_tpm_eventlog *log_tbl = NULL;
>> - struct efi_tcg2_final_events_table *final_events_table = NULL;
>> unsigned long first_entry_addr, last_entry_addr;
>> size_t log_size, last_entry_size;
>> int final_events_size = 0;
>> @@ -109,10 +109,6 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
>> * Figure out whether any events have already been logged to the
>> * final events structure, and if so how much space they take up
>> */
>> - if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
>> - final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
>> - else if (version == EFI_CC_EVENT_LOG_FORMAT_TCG_2)
>> - final_events_table = get_efi_config_table(LINUX_EFI_CC_FINAL_LOG_GUID);
>> if (final_events_table && final_events_table->nr_events) {
>> struct tcg_pcr_event2_head *header;
>> int offset;
>> @@ -152,6 +148,7 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
>>
>> void efi_retrieve_eventlog(void)
>> {
>> + struct efi_tcg2_final_events_table *final_events_table = NULL;
>> efi_physical_addr_t log_location = 0, log_last_entry = 0;
>> efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
>> int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
>> @@ -170,6 +167,8 @@ void efi_retrieve_eventlog(void)
>> &log_location, &log_last_entry,
>> &truncated);
>> }
>> + if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
>> + final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
>> } else {
>> efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>> efi_cc_protocol_t *cc = NULL;
>> @@ -179,6 +178,7 @@ void efi_retrieve_eventlog(void)
>> return;
>>
>> version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
>> + final_events_table = get_efi_config_table(LINUX_EFI_CC_FINAL_LOG_GUID);
>> status = efi_call_proto(cc, get_event_log, version, &log_location,
>> &log_last_entry, &truncated);
>> }
>> @@ -187,5 +187,5 @@ void efi_retrieve_eventlog(void)
>> return;
>>
>> efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>> - truncated);
>> + truncated, final_events_table);
>> }
>>
>>
>>
>>
>>
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
