Re: [PATCH v1] efi_loader: Remove authentication header in efi variable data

[Heinrich Schuchardt]

Am 25. Juli 2025 19:11:57 MESZ schrieb Aswin Murugan <aswin.murugan at 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.murugan at 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;
>>>   }
>>