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