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.
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
