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

[Heinrich Schuchardt]

On 21.06.24 13:25, Ilias Apalodimas wrote:
> On Fri, 21 Jun 2024 at 14:01, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
>>
>> Hi Vincent,
>>
>> [...]
>>
>>>>>    $ 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.
>>
>> Ah yes, the current code is using it in clrsetbits_8, which inverts it
>> internally, so it's indeed correct.
>>
>>>
>>>>
>>>> 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.
>>
>> Yes, we need the conversions
>>
>>> Also, this is a 16b quantity, not 32b; we need to redefine tmp to uint16_t and
>>> use the be16 functions.
>>>
>>
>> Yep I already pointed that out earlier.
>>
>>>>          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.
>>
>> RFC4122 says that
>> "put name space ID in network byte order so it hashes the same no
>> matter what endian machine we're on "
>> the EFI spec says
>> "It should also be noted that TimeLow, TimeMid, TimeHighAndVersion
>> fields in the EFI are encoded as little endian. The following table
>> defines the format of an EFI GUID (128 bits)."
>>
>> Which is lovely....
>>
>
> But this brings up something that Heinrich also mentioned. Since the
> efi spec and RFC4122 interpret the endianess differently, how do you
> expect uuid -d to work?

The binary format does depend on endianness:

$ echo -n -e
"\\xF8\\x1D\\x4F\\xAE\\x7D\\xEC\\x51\\xD0\\xA7\\x65\\x00\\xA0\\xC9\\x1E\\x6B\\xF6"
\
 > | uuid -d -F BIN -
encode: STR:     f81d4fae-7dec-51d0-a765-00a0c91e6bf6
         SIV:     329800735698586931527096882168799849462
decode: variant: DCE 1.1, ISO/IEC 11578:1996
         version: 5 (name based, SHA-1)
         content: F8:1D:4F:AE:7D:EC:01:D0:27:65:00:A0:C9:1E:6B:F6
                  (not decipherable: truncated SHA-1 message digest only)

Best regards

Heinrich

>
> Thanks
> /Ilias
>> I'll send a patch with the changes
>>
>> Regards
>> /Ilias
>>>
>>> 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