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

[Ilias Apalodimas]

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?

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