[PATCH v3 0/7] efi: CapsuleUpdate: support for dynamic UUIDs

[Heinrich Schuchardt]

On 21.06.24 11:12, Vincent Stehlé wrote:
> On Wed, Jun 19, 2024 at 10:15:32PM +0300, Ilias Apalodimas wrote:
>> Allô Vincent,
>
> Hi Ilias :)
>
>>
>> Thanks for testing!
>>
>> On Wed, 19 Jun 2024 at 17:02, Vincent Stehlé <vincent.stehle at arm.com> wrote:
>>>
>>> On Fri, May 31, 2024 at 03:50:34PM +0200, Caleb Connolly wrote:
>>>> As more boards adopt support for the EFI CapsuleUpdate mechanism, there
>>>> is a growing issue of being able to target updates to them properly. The
>>>> current mechanism of hardcoding UUIDs for each board at compile time is
>>>> unsustainable, and maintaining lists of GUIDs is similarly cumbersome.
>>>>
>>>> In this series, I propose that we adopt v5 GUIDs, these are generated
>>>> by using a well-known salt GUID as well as board specific information
>>>> (like the model/revision), these are hashed together and the result is
>>>> truncated to form a new UUID.
>>>
>>> Dear Caleb,
>>>
>>> Thank you for working on this proposal, this looks very useful.
>>> Indeed, we found out during SystemReady certifications that it is easy for
>>> unique ids to be inadvertently re-used, making them thus non-unique.
>>>
>>> I have a doubt regarding the format of the generated UUIDs, which end up in the
>>> ESRT, though.
>>>
>>> Here is a quick experiment on the sandbox booting with a DTB using the efidebug
>>> command.
>>>
>>> With the patch series, rebased on the master branch:
>>>
>>>    $ make sandbox_defconfig
>>>    $ make
>>>    $ ./u-boot --default_fdt
>>>    ...
>>>    U-Boot 2024.07-rc4-00028-g1c96225b0b3 (Jun 19 2024 - 15:29:04 +0200)
>>>    ...
>>>    Model: sandbox
>>>    ...
>>>    Hit any key to stop autoboot:  0
>>>    => efidebug capsule esrt
>>>    ...
>>>    ========================================
>>>    ESRT: fw_resource_count=2
>>>    ESRT: fw_resource_count_max=2
>>>    ESRT: fw_resource_version=1
>>>    [entry 0]==============================
>>>    ESRT: fw_class=FD5DB83C-12F3-A46B-80A9-E3007C7FF56E
>>>    ...
>>>    [entry 1]==============================
>>>    ESRT: fw_class=935FE837-FAC8-4394-C008-737D8852C60D
>>>    ...
>>>    ========================================
>>>
>>>    $ uuid -d FD5DB83C-12F3-A46B-80A9-E3007C7FF56E
>>>    encode: STR:     fd5db83c-12f3-a46b-80a9-e3007c7ff56e
>>>            SIV:     336781303264349553179461347850802165102
>>>    decode: variant: DCE 1.1, ISO/IEC 11578:1996
>>>            version: 10 (unknown)
>>>            content: FD:5D:B8:3C:12:F3:04:6B:00:A9:E3:00:7C:7F:F5:6E
>>>                     (not decipherable: unknown UUID version)
>>>
>>> Version 10 does not look right.
>>
>> So, this seems to be an endianess problem.
>> Looking at RFC4122 only the space ID needs to be in BE.
>>
>>>
>>>    $ uuid -d 935FE837-FAC8-4394-C008-737D8852C60D
>>>    encode: STR:     935fe837-fac8-4394-c008-737d8852c60d
>>>            SIV:     195894493536133784175416063449172723213
>>>    decode: variant: reserved (Microsoft GUID)
>>>            version: 4 (random data based)
>>>            content: 93:5F:E8:37:FA:C8:03:94:00:08:73:7D:88:52:C6:0D
>>>                     (no semantics: random data only)
>>>
>>> A reserved Microsoft GUID variant does not look right.
>>
>> This seems like an existing bug. RFC4122 defines the MS reserved GUIDs
>> in the variant as
>> 1     1     0    Reserved, Microsoft Corporation backward
>> compatibility and the existing UUID_VARIANT_MASK is defined as 0xc0...
>
> I think the variant mask 0xc0 is correct:
>
> - The variant field is in the top three bits of the "clock seq high and
>    reserved" byte, but...
> - The variant we want is 1 0 x (do not care for bit 5, a.k.a. "Msb2").
>
> With the mask 0xc0 we can clear the top two bits as we set the top most bit just
> after anyway.
>
> ...the mask needs to be used correctly, though; see below.
>
>>
>> The patch below should work for you (on top of Calebs')
>>
>> diff --git a/include/uuid.h b/include/uuid.h
>> index b38b20d957ef..78ed5839d2d6 100644
>> --- a/include/uuid.h
>> +++ b/include/uuid.h
>> @@ -81,7 +81,7 @@ struct uuid {
>>   #define UUID_VERSION_SHIFT     12
>>   #define UUID_VERSION           0x4
>>
>> -#define UUID_VARIANT_MASK      0xc0
>> +#define UUID_VARIANT_MASK      0xb0
>>   #define UUID_VARIANT_SHIFT     7
>>   #define UUID_VARIANT           0x1
>>
>> diff --git a/lib/uuid.c b/lib/uuid.c
>> index 89911b06ccc0..73251eaa397e 100644
>> --- a/lib/uuid.c
>> +++ b/lib/uuid.c
>> @@ -411,9 +411,9 @@ void gen_uuid_v5(const struct uuid *namespace,
>> struct uuid *uuid, ...)
>>          memcpy(uuid, hash, sizeof(*uuid));
>>
>>          /* Configure variant/version bits */
>> -       tmp = be32_to_cpu(uuid->time_hi_and_version);
>> +       tmp = uuid->time_hi_and_version;
>
> Not caring for the endianness at all does not look right.
> Indeed, while this does work on my side in little-endian, this does not work on
> on big-endian simulators.
> Also, this is a 16b quantity, not 32b; we need to redefine tmp to uint16_t and
> use the be16 functions.
>
>>          tmp = (tmp & ~UUID_VERSION_MASK) | (5 << UUID_VERSION_SHIFT);
>> -       uuid->time_hi_and_version = cpu_to_be32(tmp);
>> +       uuid->time_hi_and_version = tmp;
>>
>>          uuid->clock_seq_hi_and_reserved &= UUID_VARIANT_MASK;
>
> We need to mask with ~UUID_VARIANT_MASK, I think.
>
>>          uuid->clock_seq_hi_and_reserved |= UUID_VARIANT << UUID_VARIANT_SHIFT;
>>
>> Can you give it a shot?
>
> This does indeed work on my little-endian machines, but not on big-endian
> simulators.
> For testing on big-endian, I suggest using only genguid as the sandbox will not
> help there:
>
>    $ make sandbox_defconfig
>    $ make tools-only
>    $ ./tools/genguid 2a5aa852-b856-4d97-baa9-5c5f4421551f \
>          -c "qcom,qrb4210-rb2" \
>          QUALCOMM-UBOOT
>
> ...and feed the resulting UUID to `uuid -d'.
> (The genguid command is the online help example.)
>
>> What I am afraid of is breaking existing use cases using a different
>> variant mask....
>> If that's the case we might need to keep the buggy existing
>> UUID_VARIANT_MASK and use the new one only on v5 and newer code
>
> I tried to debug further and I suspect that:
>
> - Operations on 8b clock_seq_hi_and_reserved might need further casts.
>
> - My understanding is that we are generating the v5 UUID as big-endian in
>    memory; if this is indeed the case, genguid should not print it with the GUID
>    byte order.

The current specification is in RFC 9562, 4.1, "Variant field"

"The variant field consists of a variable number of the most significant
bits of octet 8 of the UUID.

...

Specifically for UUIDs in this document, bits 64 and 65 of the UUID
(bits 0 and 1 of octet 8) MUST be set to 1 and 0 as specified in row 2
of Table 1."

This reference to byte 8 does not depend on endianness.

U-Boot's include/uuid.h has:

     /* This is structure is in big-endian */
     struct uuid {

The field time_hi_and_version needs to be stored in big-endian fashion.


tools/genguid uses UUID_STR_FORMAT_GUID which prints low-endian which is
typical in the EFI context but not understood by 'uuid -d'. Maybe we
should add a parameter for the output format.

Best regards

Heinrich

>
> For the moment I am unable to make the code work in all the following cases:
>
> - genguid on little-endian
> - genguid on big-endian
> - sandbox ESRT on little-endian
>
> I will let you and Caleb know if I make any progress.
>
> Best regards,
> Vincent.
>
>>
>> Thanks
>> /Ilias