Am 25. Juli 2025 19:11:57 MESZ schrieb Aswin Murugan
<aswin.muru...@oss.qualcomm.com>:
>
>On 7/25/2025 6:56 PM, Heinrich Schuchardt wrote:
>> On 25.07.25 14:36, Aswin Murugan wrote:
>>> efi_get_variable_mem() returned auth header along with ESL data for
>>> authenticated variables like PK, KEK, and DB. This caused issues in
>>> later stages where the auth header was misinterpreted as ESL data,
>>
>> Which later stages are you relating to?
>> In the current implementation of efi_signature_store()
>> https://source.denx.de/u-boot/u-boot/-/blob/master/lib/efi_loader/efi_signature.c#L809,
>> the db variable is retrieved using efi_get_var() and passed directly to
>> efi_build_signature_store(). However, for authenticated variables, the data
>> returned by efi_get_var() includes the authentication header.
>> efi_build_signature_store() expects the data pointer to begin at the start
>> of the EFI Signature List, not the authentication header. This mismatch leads
>> to a parsing error, as the header is misinterpreted as part of the signature
>> list, resulting in an invalid signature list format.
>>
>> How can the error be reproduced?
>> Created the ubootefi.var through tools/efivar.py and enabled
>> EFI_VARIABLES_PRESEED for secure boot,
>> during image authentication, wrong siglist format error was observed.
>>
>>> leading to parsing failures, the efi_get_variable_mem() is expected
>>> to return esl data alone.
>>> Added a check to remove the auth header if the variable has the
>>> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS attribute. Now,
>>> only the ESL data is returned from the function.
>>
>> Please, have a look at chapter 8.2.1 GetVariable in the UEFI spec.
>>
>> When the attribute EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS is set, the
>> Data buffer shall be interpreted as follows:
>> // NOTE: “||” indicates concatenation.
>> // Example: EFI_VARIABLE_AUTHENTICATION_3_TIMESTAMP_TYPE
>> EFI_VARIABLE_AUTHENTICATION_3 || EFI_TIME ||
>> EFI_VARIABLE_AUTHENTICATION_3_CERT_ID || Data
>> // Example: EFI_VARIABLE_AUTHENTICATION_3_NONCE_TYPE
>> EFI_VARIABLE_AUTHENTICATION_3 || EFI_VARIABLE_AUTHENTICATION_3_NONCE ||
>> EFI_VARIABLE_AUTHENTICATION_3_CERT_ID || Data
>>
> Isn't removing the authentication header violating the EFI specification?
> I belive that stripping the auth header during variable read might not
> violate the spec.
How could you return a different value from GetVariable() than required by the
spec and call it compliant?
Please, adjust the function consuming the value.
Best regards
Heinrich
>>
>> Is tests/test_efi_secboot/test_authvar.py still passing with your patch?
>> Yes, it passed
>>>
>>> Signed-off-by: Aswin Murugan <aswin.muru...@oss.qualcomm.com>
>>> ---
>>> lib/efi_loader/efi_var_mem.c | 42 +++++++++++++++++++++++++++++-------
>>> 1 file changed, 34 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
>>> index 31180df9e3a..f73daf8bdc9 100644
>>> --- a/lib/efi_loader/efi_var_mem.c
>>> +++ b/lib/efi_loader/efi_var_mem.c
>>> @@ -309,9 +309,12 @@ efi_get_variable_mem(const u16 *variable_name, const
>>> efi_guid_t *vendor,
>>> u32 *attributes, efi_uintn_t *data_size, void *data,
>>> u64 *timep, u32 mask)
>>> {
>>> - efi_uintn_t old_size;
>>> + efi_uintn_t old_size, var_data_size, auth_size;
>>> struct efi_var_entry *var;
>>> u16 *pdata;
>>> + void *var_data;
>>> + struct efi_time *timestamp;
>>> + struct win_certificate_uefi_guid *cert;
>>
>> Please, move variable definitions to the innermost code block where they are
>> used.
>>
>> Best regards
>>
>> Heinrich
>>
>>> if (!variable_name || !vendor || !data_size)
>>> return EFI_INVALID_PARAMETER;
>>> @@ -335,19 +338,42 @@ efi_get_variable_mem(const u16 *variable_name, const
>>> efi_guid_t *vendor,
>>> if (!u16_strcmp(variable_name, vtf))
>>> return efi_var_collect_mem(data, data_size,
>>> EFI_VARIABLE_NON_VOLATILE);
>>> + for (pdata = var->name; *pdata; ++pdata)
>>> + ;
>>> + ++pdata;
>>> +
>>> + var_data = (void *)pdata;
>>> + var_data_size = var->length;
>>> +
>>> + /* Handle EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS */
>>> + if (var->attr & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) {
>>> + if (var_data_size > sizeof(struct efi_time)) {
>>> + timestamp = (struct efi_time *)var_data;
>>> + cert = (struct win_certificate_uefi_guid *)(timestamp + 1);
>>> + auth_size = sizeof(struct efi_time) + cert->hdr.dwLength;
>>> + if (var_data_size > auth_size) {
>>> + var_data = (u8 *)var_data + auth_size;
>>> + var_data_size -= auth_size;
>>> + }
>>> + }
>>> + }
>>> + /*
>>> + * EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS type is old & not
>>> + * expected for auth variables
>>> + */
>>> + else if (var->attr & EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS)
>>> + return EFI_INVALID_PARAMETER;
>>> +
>>> old_size = *data_size;
>>> - *data_size = var->length;
>>> - if (old_size < var->length)
>>> + *data_size = var_data_size;
>>> +
>>> + if (old_size < var_data_size)
>>> return EFI_BUFFER_TOO_SMALL;
>>> if (!data)
>>> return EFI_INVALID_PARAMETER;
>>> - for (pdata = var->name; *pdata; ++pdata)
>>> - ;
>>> - ++pdata;
>>> -
>>> - efi_memcpy_runtime(data, pdata, var->length);
>>> + efi_memcpy_runtime(data, var_data, var_data_size);
>>> return EFI_SUCCESS;
>>> }
>>
