mbox series

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

Message ID 20240531-b4-dynamic-uuid-v3-0-ca4a4865db00@linaro.org
Headers show
Series efi: CapsuleUpdate: support for dynamic UUIDs | expand

Message

Caleb Connolly May 31, 2024, 1:50 p.m. UTC
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.

The well-known salt GUID can be specific to the architecture (SoC
vendor), or OEM. It is defined in the board defconfig so that vendors
can easily bring their own.

Specifically, the following fields are used to generate a GUID for a
particular fw_image:

* namespace salt
* board compatible (usually the first entry in the dt root compatible
  array).
* fw_image name (the string identifying the specific image, especially
  relevant for board that can update multiple images).

== Usage ==

Boards can integrate dynamic UUID support as follows:

1. Adjust Kconfig to depend on EFI_CAPSULE_DYNAMIC_UUIDS if
   EFI_HAVE_CAPSULE_SUPPORT.
2. Skip setting the fw_images image_type_id property.
3. Generate a UUID and set CONFIG_EFI_CAPSULE_NAMESPACE_UUID in your
   defconfig.

== Limitations ==

* Changing GUIDs

The primary limitation with this approach is that if any of the source
fields change, so will the GUID for the board. It is therefore pretty
important to ensure that GUID changes are caught during development.

* Supporting multiple boards with a single image

This now requires having an entry with the GUID for every board which
might lead to larger UpdateCapsule images.

== Tooling ==

This series introduces a new tool: genguid. This can be used to generate
the same GUIDs that the board would at runtime.

This series follows a related discussion started by Ilias:
https://lore.kernel.org/u-boot/CAC_iWjJNHa4gMF897MqYZNdbgjFG8K4kwGsTXWuy72WkYLizrw@mail.gmail.com/

---
Changes in v3:
- Add manpage for genguid
- Add dedicated CONFIG_TOOLS_GENGUID option
- Minor code fixes addressing v2 feedback
- Link to v2: https://lore.kernel.org/r/20240529-b4-dynamic-uuid-v2-0-c26f31057bbe@linaro.org

Changes in v2:
- Move namespace UUID to be defined in defconfig
- Add tests and tooling
- Only use the first board compatible to generate UUID.
- Link to v1: https://lore.kernel.org/r/20240426-b4-dynamic-uuid-v1-0-e8154e00ec44@linaro.org

---
Caleb Connolly (7):
      lib: uuid: add UUID v5 support
      efi: add a helper to generate dynamic UUIDs
      doc: uefi: document dynamic UUID generation
      sandbox: switch to dynamic UUIDs
      lib: uuid: supporting building as part of host tools
      tools: add genguid tool
      test: lib/uuid: add unit tests for dynamic UUIDs

 arch/Kconfig                                       |   1 +
 board/sandbox/sandbox.c                            |  16 ---
 configs/sandbox_defconfig                          |   1 +
 configs/sandbox_flattree_defconfig                 |   1 +
 doc/develop/uefi/uefi.rst                          |  31 +++++
 doc/genguid.1                                      |  52 +++++++
 include/sandbox_efi_capsule.h                      |   6 +-
 include/uuid.h                                     |  21 ++-
 lib/Kconfig                                        |   8 ++
 lib/efi_loader/Kconfig                             |  23 +++
 lib/efi_loader/efi_capsule.c                       |   1 +
 lib/efi_loader/efi_firmware.c                      |  66 +++++++++
 lib/uuid.c                                         |  81 +++++++++--
 test/lib/uuid.c                                    |  88 ++++++++++++
 .../test_efi_capsule/test_capsule_firmware_fit.py  |   2 +-
 .../test_efi_capsule/test_capsule_firmware_raw.py  |   8 +-
 .../test_capsule_firmware_signed_fit.py            |   2 +-
 .../test_capsule_firmware_signed_raw.py            |   4 +-
 test/py/tests/test_efi_capsule/version.dts         |   6 +-
 tools/Kconfig                                      |   7 +
 tools/Makefile                                     |   3 +
 tools/binman/etype/efi_capsule.py                  |   2 +-
 tools/binman/ftest.py                              |   2 +-
 tools/genguid.c                                    | 154 +++++++++++++++++++++
 24 files changed, 538 insertions(+), 48 deletions(-)
---
change-id: 20240422-b4-dynamic-uuid-1a5ab1486c27
base-commit: 2e682a4a406fc81ef32e05c28542cc8067f1e15f

// Caleb (they/them)

Comments

Heinrich Schuchardt June 5, 2024, 5:59 a.m. UTC | #1
On 5/31/24 15:50, 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.
>
> The well-known salt GUID can be specific to the architecture (SoC
> vendor), or OEM. It is defined in the board defconfig so that vendors
> can easily bring their own.
>
> Specifically, the following fields are used to generate a GUID for a
> particular fw_image:
>
> * namespace salt
> * board compatible (usually the first entry in the dt root compatible
>    array).
> * fw_image name (the string identifying the specific image, especially
>    relevant for board that can update multiple images).
>
> == Usage ==
>
> Boards can integrate dynamic UUID support as follows:
>
> 1. Adjust Kconfig to depend on EFI_CAPSULE_DYNAMIC_UUIDS if
>     EFI_HAVE_CAPSULE_SUPPORT.
> 2. Skip setting the fw_images image_type_id property.
> 3. Generate a UUID and set CONFIG_EFI_CAPSULE_NAMESPACE_UUID in your
>     defconfig.

Why should we have two alternatives?

If we have the dynamic UUIDs, please, get rid of static ones.

>
> == Limitations ==
>
> * Changing GUIDs
>
> The primary limitation with this approach is that if any of the source
> fields change, so will the GUID for the board. It is therefore pretty
> important to ensure that GUID changes are caught during development.
>
> * Supporting multiple boards with a single image
>
> This now requires having an entry with the GUID for every board which
> might lead to larger UpdateCapsule images.

Do we have a test case were a capsule contains images that shall not be
installed?

>
> == Tooling ==
>
> This series introduces a new tool: genguid. This can be used to generate
> the same GUIDs that the board would at runtime.

     the GUIDs that the board requires at runtime.

Best regards

Heinrich

>
> This series follows a related discussion started by Ilias:
> https://lore.kernel.org/u-boot/CAC_iWjJNHa4gMF897MqYZNdbgjFG8K4kwGsTXWuy72WkYLizrw@mail.gmail.com/
>
> ---
> Changes in v3:
> - Add manpage for genguid
> - Add dedicated CONFIG_TOOLS_GENGUID option
> - Minor code fixes addressing v2 feedback
> - Link to v2: https://lore.kernel.org/r/20240529-b4-dynamic-uuid-v2-0-c26f31057bbe@linaro.org
>
> Changes in v2:
> - Move namespace UUID to be defined in defconfig
> - Add tests and tooling
> - Only use the first board compatible to generate UUID.
> - Link to v1: https://lore.kernel.org/r/20240426-b4-dynamic-uuid-v1-0-e8154e00ec44@linaro.org
>
> ---
> Caleb Connolly (7):
>        lib: uuid: add UUID v5 support
>        efi: add a helper to generate dynamic UUIDs
>        doc: uefi: document dynamic UUID generation
>        sandbox: switch to dynamic UUIDs
>        lib: uuid: supporting building as part of host tools
>        tools: add genguid tool
>        test: lib/uuid: add unit tests for dynamic UUIDs
>
>   arch/Kconfig                                       |   1 +
>   board/sandbox/sandbox.c                            |  16 ---
>   configs/sandbox_defconfig                          |   1 +
>   configs/sandbox_flattree_defconfig                 |   1 +
>   doc/develop/uefi/uefi.rst                          |  31 +++++
>   doc/genguid.1                                      |  52 +++++++
>   include/sandbox_efi_capsule.h                      |   6 +-
>   include/uuid.h                                     |  21 ++-
>   lib/Kconfig                                        |   8 ++
>   lib/efi_loader/Kconfig                             |  23 +++
>   lib/efi_loader/efi_capsule.c                       |   1 +
>   lib/efi_loader/efi_firmware.c                      |  66 +++++++++
>   lib/uuid.c                                         |  81 +++++++++--
>   test/lib/uuid.c                                    |  88 ++++++++++++
>   .../test_efi_capsule/test_capsule_firmware_fit.py  |   2 +-
>   .../test_efi_capsule/test_capsule_firmware_raw.py  |   8 +-
>   .../test_capsule_firmware_signed_fit.py            |   2 +-
>   .../test_capsule_firmware_signed_raw.py            |   4 +-
>   test/py/tests/test_efi_capsule/version.dts         |   6 +-
>   tools/Kconfig                                      |   7 +
>   tools/Makefile                                     |   3 +
>   tools/binman/etype/efi_capsule.py                  |   2 +-
>   tools/binman/ftest.py                              |   2 +-
>   tools/genguid.c                                    | 154 +++++++++++++++++++++
>   24 files changed, 538 insertions(+), 48 deletions(-)
> ---
> change-id: 20240422-b4-dynamic-uuid-1a5ab1486c27
> base-commit: 2e682a4a406fc81ef32e05c28542cc8067f1e15f
>
> // Caleb (they/them)
>
Caleb Connolly June 5, 2024, 5:02 p.m. UTC | #2
On 05/06/2024 07:59, Heinrich Schuchardt wrote:
> On 5/31/24 15:50, 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.
>>
>> The well-known salt GUID can be specific to the architecture (SoC
>> vendor), or OEM. It is defined in the board defconfig so that vendors
>> can easily bring their own.
>>
>> Specifically, the following fields are used to generate a GUID for a
>> particular fw_image:
>>
>> * namespace salt
>> * board compatible (usually the first entry in the dt root compatible
>>    array).
>> * fw_image name (the string identifying the specific image, especially
>>    relevant for board that can update multiple images).
>>
>> == Usage ==
>>
>> Boards can integrate dynamic UUID support as follows:
>>
>> 1. Adjust Kconfig to depend on EFI_CAPSULE_DYNAMIC_UUIDS if
>>     EFI_HAVE_CAPSULE_SUPPORT.
>> 2. Skip setting the fw_images image_type_id property.
>> 3. Generate a UUID and set CONFIG_EFI_CAPSULE_NAMESPACE_UUID in your
>>     defconfig.
> 
> Why should we have two alternatives?
> 
> If we have the dynamic UUIDs, please, get rid of static ones.

I don't think this is possible to do without the risk of breaking some 
boards. Can we assume that ALL existing capsule update image GUIDs are 
safe to just scrap?
> 
>>
>> == Limitations ==
>>
>> * Changing GUIDs
>>
>> The primary limitation with this approach is that if any of the source
>> fields change, so will the GUID for the board. It is therefore pretty
>> important to ensure that GUID changes are caught during development.
>>
>> * Supporting multiple boards with a single image
>>
>> This now requires having an entry with the GUID for every board which
>> might lead to larger UpdateCapsule images.
> 
> Do we have a test case were a capsule contains images that shall not be
> installed?
> 
>>
>> == Tooling ==
>>
>> This series introduces a new tool: genguid. This can be used to generate
>> the same GUIDs that the board would at runtime.
> 
>      the GUIDs that the board requires at runtime.
> 
> Best regards
> 
> Heinrich
> 
>>
>> This series follows a related discussion started by Ilias:
>> https://lore.kernel.org/u-boot/CAC_iWjJNHa4gMF897MqYZNdbgjFG8K4kwGsTXWuy72WkYLizrw@mail.gmail.com/
>>
>> ---
>> Changes in v3:
>> - Add manpage for genguid
>> - Add dedicated CONFIG_TOOLS_GENGUID option
>> - Minor code fixes addressing v2 feedback
>> - Link to v2: 
>> https://lore.kernel.org/r/20240529-b4-dynamic-uuid-v2-0-c26f31057bbe@linaro.org
>>
>> Changes in v2:
>> - Move namespace UUID to be defined in defconfig
>> - Add tests and tooling
>> - Only use the first board compatible to generate UUID.
>> - Link to v1: 
>> https://lore.kernel.org/r/20240426-b4-dynamic-uuid-v1-0-e8154e00ec44@linaro.org
>>
>> ---
>> Caleb Connolly (7):
>>        lib: uuid: add UUID v5 support
>>        efi: add a helper to generate dynamic UUIDs
>>        doc: uefi: document dynamic UUID generation
>>        sandbox: switch to dynamic UUIDs
>>        lib: uuid: supporting building as part of host tools
>>        tools: add genguid tool
>>        test: lib/uuid: add unit tests for dynamic UUIDs
>>
>>   arch/Kconfig                                       |   1 +
>>   board/sandbox/sandbox.c                            |  16 ---
>>   configs/sandbox_defconfig                          |   1 +
>>   configs/sandbox_flattree_defconfig                 |   1 +
>>   doc/develop/uefi/uefi.rst                          |  31 +++++
>>   doc/genguid.1                                      |  52 +++++++
>>   include/sandbox_efi_capsule.h                      |   6 +-
>>   include/uuid.h                                     |  21 ++-
>>   lib/Kconfig                                        |   8 ++
>>   lib/efi_loader/Kconfig                             |  23 +++
>>   lib/efi_loader/efi_capsule.c                       |   1 +
>>   lib/efi_loader/efi_firmware.c                      |  66 +++++++++
>>   lib/uuid.c                                         |  81 +++++++++--
>>   test/lib/uuid.c                                    |  88 ++++++++++++
>>   .../test_efi_capsule/test_capsule_firmware_fit.py  |   2 +-
>>   .../test_efi_capsule/test_capsule_firmware_raw.py  |   8 +-
>>   .../test_capsule_firmware_signed_fit.py            |   2 +-
>>   .../test_capsule_firmware_signed_raw.py            |   4 +-
>>   test/py/tests/test_efi_capsule/version.dts         |   6 +-
>>   tools/Kconfig                                      |   7 +
>>   tools/Makefile                                     |   3 +
>>   tools/binman/etype/efi_capsule.py                  |   2 +-
>>   tools/binman/ftest.py                              |   2 +-
>>   tools/genguid.c                                    | 154 
>> +++++++++++++++++++++
>>   24 files changed, 538 insertions(+), 48 deletions(-)
>> ---
>> change-id: 20240422-b4-dynamic-uuid-1a5ab1486c27
>> base-commit: 2e682a4a406fc81ef32e05c28542cc8067f1e15f
>>
>> // Caleb (they/them)
>>
>
Vincent Stehlé June 19, 2024, 2:02 p.m. UTC | #3
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.

  $ 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.

With the master branch, the (hardcoded) GUIDs are ok:

  $ ./u-boot --default_fdt
  ...
  U-Boot 2024.07-rc4-00021-gfe2ce09a075 (Jun 19 2024 - 15:35:08 +0200)
  ...
  Model: sandbox
  ...
  => efidebug capsule esrt
  ...
  ========================================
  ESRT: fw_resource_count=2
  ESRT: fw_resource_count_max=2
  ESRT: fw_resource_version=1
  [entry 0]==============================
  ESRT: fw_class=09D7CF52-0720-4710-91D1-08469B7FE9C8
  ...
  [entry 1]==============================
  ESRT: fw_class=5A7021F5-FEF2-48B4-AABA-832E777418C0
  ...
  ========================================

  $ uuid -d 09D7CF52-0720-4710-91D1-08469B7FE9C8
  encode: STR:     09d7cf52-0720-4710-91d1-08469b7fe9c8
          SIV:     13083600744351929150374221048734280136
  decode: variant: DCE 1.1, ISO/IEC 11578:1996
          version: 4 (random data based)
          content: 09:D7:CF:52:07:20:07:10:11:D1:08:46:9B:7F:E9:C8
                   (no semantics: random data only)

  $ uuid -d 5A7021F5-FEF2-48B4-AABA-832E777418C0
  encode: STR:     5a7021f5-fef2-48b4-aaba-832e777418c0
          SIV:     120212745678117161641696128857923655872
  decode: variant: DCE 1.1, ISO/IEC 11578:1996
          version: 4 (random data based)
          content: 5A:70:21:F5:FE:F2:08:B4:2A:BA:83:2E:77:74:18:C0
                   (no semantics: random data only)

Also, this is what to expect for a v5 UUID [1]:

  $ uuid -d a8f6ae40-d8a7-58f0-be05-a22f94eca9ec
  encode: STR:     a8f6ae40-d8a7-58f0-be05-a22f94eca9ec
          SIV:     224591142595989943290477237735758014956
  decode: variant: DCE 1.1, ISO/IEC 11578:1996
          version: 5 (name based, SHA-1)
          content: A8:F6:AE:40:D8:A7:08:F0:3E:05:A2:2F:94:EC:A9:EC
                   (not decipherable: truncated SHA-1 message digest only)

Best regards,
Vincent.

[1] https://uuid.ramsey.dev/en/stable/rfc4122/version5.html

> 
> The well-known salt GUID can be specific to the architecture (SoC
> vendor), or OEM. It is defined in the board defconfig so that vendors
> can easily bring their own.
> 
> Specifically, the following fields are used to generate a GUID for a
> particular fw_image:
> 
> * namespace salt
> * board compatible (usually the first entry in the dt root compatible
>   array).
> * fw_image name (the string identifying the specific image, especially
>   relevant for board that can update multiple images).
> 
> == Usage ==
> 
> Boards can integrate dynamic UUID support as follows:
> 
> 1. Adjust Kconfig to depend on EFI_CAPSULE_DYNAMIC_UUIDS if
>    EFI_HAVE_CAPSULE_SUPPORT.
> 2. Skip setting the fw_images image_type_id property.
> 3. Generate a UUID and set CONFIG_EFI_CAPSULE_NAMESPACE_UUID in your
>    defconfig.
> 
> == Limitations ==
> 
> * Changing GUIDs
> 
> The primary limitation with this approach is that if any of the source
> fields change, so will the GUID for the board. It is therefore pretty
> important to ensure that GUID changes are caught during development.
> 
> * Supporting multiple boards with a single image
> 
> This now requires having an entry with the GUID for every board which
> might lead to larger UpdateCapsule images.
> 
> == Tooling ==
> 
> This series introduces a new tool: genguid. This can be used to generate
> the same GUIDs that the board would at runtime.
> 
> This series follows a related discussion started by Ilias:
> https://lore.kernel.org/u-boot/CAC_iWjJNHa4gMF897MqYZNdbgjFG8K4kwGsTXWuy72WkYLizrw@mail.gmail.com/
> 
> ---
> Changes in v3:
> - Add manpage for genguid
> - Add dedicated CONFIG_TOOLS_GENGUID option
> - Minor code fixes addressing v2 feedback
> - Link to v2: https://lore.kernel.org/r/20240529-b4-dynamic-uuid-v2-0-c26f31057bbe@linaro.org
> 
> Changes in v2:
> - Move namespace UUID to be defined in defconfig
> - Add tests and tooling
> - Only use the first board compatible to generate UUID.
> - Link to v1: https://lore.kernel.org/r/20240426-b4-dynamic-uuid-v1-0-e8154e00ec44@linaro.org
> 
> ---
> Caleb Connolly (7):
>       lib: uuid: add UUID v5 support
>       efi: add a helper to generate dynamic UUIDs
>       doc: uefi: document dynamic UUID generation
>       sandbox: switch to dynamic UUIDs
>       lib: uuid: supporting building as part of host tools
>       tools: add genguid tool
>       test: lib/uuid: add unit tests for dynamic UUIDs
> 
>  arch/Kconfig                                       |   1 +
>  board/sandbox/sandbox.c                            |  16 ---
>  configs/sandbox_defconfig                          |   1 +
>  configs/sandbox_flattree_defconfig                 |   1 +
>  doc/develop/uefi/uefi.rst                          |  31 +++++
>  doc/genguid.1                                      |  52 +++++++
>  include/sandbox_efi_capsule.h                      |   6 +-
>  include/uuid.h                                     |  21 ++-
>  lib/Kconfig                                        |   8 ++
>  lib/efi_loader/Kconfig                             |  23 +++
>  lib/efi_loader/efi_capsule.c                       |   1 +
>  lib/efi_loader/efi_firmware.c                      |  66 +++++++++
>  lib/uuid.c                                         |  81 +++++++++--
>  test/lib/uuid.c                                    |  88 ++++++++++++
>  .../test_efi_capsule/test_capsule_firmware_fit.py  |   2 +-
>  .../test_efi_capsule/test_capsule_firmware_raw.py  |   8 +-
>  .../test_capsule_firmware_signed_fit.py            |   2 +-
>  .../test_capsule_firmware_signed_raw.py            |   4 +-
>  test/py/tests/test_efi_capsule/version.dts         |   6 +-
>  tools/Kconfig                                      |   7 +
>  tools/Makefile                                     |   3 +
>  tools/binman/etype/efi_capsule.py                  |   2 +-
>  tools/binman/ftest.py                              |   2 +-
>  tools/genguid.c                                    | 154 +++++++++++++++++++++
>  24 files changed, 538 insertions(+), 48 deletions(-)
> ---
> change-id: 20240422-b4-dynamic-uuid-1a5ab1486c27
> base-commit: 2e682a4a406fc81ef32e05c28542cc8067f1e15f
> 
> // Caleb (they/them)
>
Ilias Apalodimas June 19, 2024, 7:15 p.m. UTC | #4
Allô Vincent,

Thanks for testing!

On Wed, 19 Jun 2024 at 17:02, Vincent Stehlé <vincent.stehle@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...

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;
        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;
        uuid->clock_seq_hi_and_reserved |= UUID_VARIANT << UUID_VARIANT_SHIFT;

Can you give it a shot?
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

Thanks
/Ilias


>
> With the master branch, the (hardcoded) GUIDs are ok:
>
>   $ ./u-boot --default_fdt
>   ...
>   U-Boot 2024.07-rc4-00021-gfe2ce09a075 (Jun 19 2024 - 15:35:08 +0200)
>   ...
>   Model: sandbox
>   ...
>   => efidebug capsule esrt
>   ...
>   ========================================
>   ESRT: fw_resource_count=2
>   ESRT: fw_resource_count_max=2
>   ESRT: fw_resource_version=1
>   [entry 0]==============================
>   ESRT: fw_class=09D7CF52-0720-4710-91D1-08469B7FE9C8
>   ...
>   [entry 1]==============================
>   ESRT: fw_class=5A7021F5-FEF2-48B4-AABA-832E777418C0
>   ...
>   ========================================
>
>   $ uuid -d 09D7CF52-0720-4710-91D1-08469B7FE9C8
>   encode: STR:     09d7cf52-0720-4710-91d1-08469b7fe9c8
>           SIV:     13083600744351929150374221048734280136
>   decode: variant: DCE 1.1, ISO/IEC 11578:1996
>           version: 4 (random data based)
>           content: 09:D7:CF:52:07:20:07:10:11:D1:08:46:9B:7F:E9:C8
>                    (no semantics: random data only)
>
>   $ uuid -d 5A7021F5-FEF2-48B4-AABA-832E777418C0
>   encode: STR:     5a7021f5-fef2-48b4-aaba-832e777418c0
>           SIV:     120212745678117161641696128857923655872
>   decode: variant: DCE 1.1, ISO/IEC 11578:1996
>           version: 4 (random data based)
>           content: 5A:70:21:F5:FE:F2:08:B4:2A:BA:83:2E:77:74:18:C0
>                    (no semantics: random data only)
>
> Also, this is what to expect for a v5 UUID [1]:
>
>   $ uuid -d a8f6ae40-d8a7-58f0-be05-a22f94eca9ec
>   encode: STR:     a8f6ae40-d8a7-58f0-be05-a22f94eca9ec
>           SIV:     224591142595989943290477237735758014956
>   decode: variant: DCE 1.1, ISO/IEC 11578:1996
>           version: 5 (name based, SHA-1)
>           content: A8:F6:AE:40:D8:A7:08:F0:3E:05:A2:2F:94:EC:A9:EC
>                    (not decipherable: truncated SHA-1 message digest only)
>
> Best regards,
> Vincent.
>
> [1] https://uuid.ramsey.dev/en/stable/rfc4122/version5.html
>
> >
> > The well-known salt GUID can be specific to the architecture (SoC
> > vendor), or OEM. It is defined in the board defconfig so that vendors
> > can easily bring their own.
> >
> > Specifically, the following fields are used to generate a GUID for a
> > particular fw_image:
> >
> > * namespace salt
> > * board compatible (usually the first entry in the dt root compatible
> >   array).
> > * fw_image name (the string identifying the specific image, especially
> >   relevant for board that can update multiple images).
> >
> > == Usage ==
> >
> > Boards can integrate dynamic UUID support as follows:
> >
> > 1. Adjust Kconfig to depend on EFI_CAPSULE_DYNAMIC_UUIDS if
> >    EFI_HAVE_CAPSULE_SUPPORT.
> > 2. Skip setting the fw_images image_type_id property.
> > 3. Generate a UUID and set CONFIG_EFI_CAPSULE_NAMESPACE_UUID in your
> >    defconfig.
> >
> > == Limitations ==
> >
> > * Changing GUIDs
> >
> > The primary limitation with this approach is that if any of the source
> > fields change, so will the GUID for the board. It is therefore pretty
> > important to ensure that GUID changes are caught during development.
> >
> > * Supporting multiple boards with a single image
> >
> > This now requires having an entry with the GUID for every board which
> > might lead to larger UpdateCapsule images.
> >
> > == Tooling ==
> >
> > This series introduces a new tool: genguid. This can be used to generate
> > the same GUIDs that the board would at runtime.
> >
> > This series follows a related discussion started by Ilias:
> > https://lore.kernel.org/u-boot/CAC_iWjJNHa4gMF897MqYZNdbgjFG8K4kwGsTXWuy72WkYLizrw@mail.gmail.com/
> >
> > ---
> > Changes in v3:
> > - Add manpage for genguid
> > - Add dedicated CONFIG_TOOLS_GENGUID option
> > - Minor code fixes addressing v2 feedback
> > - Link to v2: https://lore.kernel.org/r/20240529-b4-dynamic-uuid-v2-0-c26f31057bbe@linaro.org
> >
> > Changes in v2:
> > - Move namespace UUID to be defined in defconfig
> > - Add tests and tooling
> > - Only use the first board compatible to generate UUID.
> > - Link to v1: https://lore.kernel.org/r/20240426-b4-dynamic-uuid-v1-0-e8154e00ec44@linaro.org
> >
> > ---
> > Caleb Connolly (7):
> >       lib: uuid: add UUID v5 support
> >       efi: add a helper to generate dynamic UUIDs
> >       doc: uefi: document dynamic UUID generation
> >       sandbox: switch to dynamic UUIDs
> >       lib: uuid: supporting building as part of host tools
> >       tools: add genguid tool
> >       test: lib/uuid: add unit tests for dynamic UUIDs
> >
> >  arch/Kconfig                                       |   1 +
> >  board/sandbox/sandbox.c                            |  16 ---
> >  configs/sandbox_defconfig                          |   1 +
> >  configs/sandbox_flattree_defconfig                 |   1 +
> >  doc/develop/uefi/uefi.rst                          |  31 +++++
> >  doc/genguid.1                                      |  52 +++++++
> >  include/sandbox_efi_capsule.h                      |   6 +-
> >  include/uuid.h                                     |  21 ++-
> >  lib/Kconfig                                        |   8 ++
> >  lib/efi_loader/Kconfig                             |  23 +++
> >  lib/efi_loader/efi_capsule.c                       |   1 +
> >  lib/efi_loader/efi_firmware.c                      |  66 +++++++++
> >  lib/uuid.c                                         |  81 +++++++++--
> >  test/lib/uuid.c                                    |  88 ++++++++++++
> >  .../test_efi_capsule/test_capsule_firmware_fit.py  |   2 +-
> >  .../test_efi_capsule/test_capsule_firmware_raw.py  |   8 +-
> >  .../test_capsule_firmware_signed_fit.py            |   2 +-
> >  .../test_capsule_firmware_signed_raw.py            |   4 +-
> >  test/py/tests/test_efi_capsule/version.dts         |   6 +-
> >  tools/Kconfig                                      |   7 +
> >  tools/Makefile                                     |   3 +
> >  tools/binman/etype/efi_capsule.py                  |   2 +-
> >  tools/binman/ftest.py                              |   2 +-
> >  tools/genguid.c                                    | 154 +++++++++++++++++++++
> >  24 files changed, 538 insertions(+), 48 deletions(-)
> > ---
> > change-id: 20240422-b4-dynamic-uuid-1a5ab1486c27
> > base-commit: 2e682a4a406fc81ef32e05c28542cc8067f1e15f
> >
> > // Caleb (they/them)
> >
Vincent Stehlé June 21, 2024, 9:12 a.m. UTC | #5
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@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
Heinrich Schuchardt June 21, 2024, 11 a.m. UTC | #6
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@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
Ilias Apalodimas June 21, 2024, 11:01 a.m. UTC | #7
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....

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
Ilias Apalodimas June 21, 2024, 11:25 a.m. UTC | #8
On Fri, 21 Jun 2024 at 14:01, Ilias Apalodimas
<ilias.apalodimas@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
Heinrich Schuchardt June 21, 2024, 1:16 p.m. UTC | #9
On 21.06.24 13:25, Ilias Apalodimas wrote:
> On Fri, 21 Jun 2024 at 14:01, Ilias Apalodimas
> <ilias.apalodimas@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
Vincent Stehlé June 21, 2024, 5:06 p.m. UTC | #10
On Fri, Jun 21, 2024 at 01:00:51PM +0200, Heinrich Schuchardt wrote:
(..)
> 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.

Hi Heinrich,

Agreed, variant is not concerned by the 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.

Thanks! I thought this structure was used to hold either a big-endian UUID or a
little-endian GUID, but now you have convinced me.

This confirms that the generation of the dynamic GUID is missing something:

    gen_uuid_v5(&namespace,
                (struct uuid *)&fw_array[i].image_type_id,
                compatible, strlen(compatible),
                fw_array[i].fw_name, u16_strsize(fw_array[i].fw_name)
                    - sizeof(uint16_t),
                NULL);

It is not possible to cast the little-endian efi_guid_t .image_type_id as the
big-endian struct uuid output of gen_uuid_v5() like this; we need to convert the
three time fields from big to little endianness.

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

My understanding is that there is a single universal string format for both
UUIDs and GUIDs, which uuid -d understands, and which has no notion of
endianness. (Only the structures in memory have an endianness.)
This means we do not need an output format parameter.

genguid is calling the new gen_uuid_v5() function, which outputs a big-endian
struct uuid. Therefore, genguid should print it with 'STD format, not 'GUID.

Best regards,
Vincent.

> 
> Best regards
> 
> Heinrich
Caleb Connolly June 21, 2024, 5:11 p.m. UTC | #11
On Fri, 21 Jun 2024, 19:06 Vincent Stehlé, <vincent.stehle@arm.com> wrote:

> On Fri, Jun 21, 2024 at 01:00:51PM +0200, Heinrich Schuchardt wrote:
> (..)
> > 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.
>
> Hi Heinrich,
>
> Agreed, variant is not concerned by the 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.
>
> Thanks! I thought this structure was used to hold either a big-endian UUID
> or a
> little-endian GUID, but now you have convinced me.
>
> This confirms that the generation of the dynamic GUID is missing something:
>
>     gen_uuid_v5(&namespace,
>                 (struct uuid *)&fw_array[i].image_type_id,
>                 compatible, strlen(compatible),
>                 fw_array[i].fw_name, u16_strsize(fw_array[i].fw_name)
>                     - sizeof(uint16_t),
>                 NULL);
>
> It is not possible to cast the little-endian efi_guid_t .image_type_id as
> the
> big-endian struct uuid output of gen_uuid_v5() like this; we need to
> convert the
> three time fields from big to little endianness.
>

I'm inclined to disagree, the comment above struct uuid in include/uuid.h
state clearly that the format in memory is always big endian, but that a
GUID when printed has some fields converted to little endian.


> >
> >
> > 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.
>
> My understanding is that there is a single universal string format for both
> UUIDs and GUIDs, which uuid -d understands, and which has no notion of
> endianness. (Only the structures in memory have an endianness.)
> This means we do not need an output format parameter.
>
> genguid is calling the new gen_uuid_v5() function, which outputs a
> big-endian
> struct uuid. Therefore, genguid should print it with 'STD format, not
> 'GUID.
>
> Best regards,
> Vincent.
>
> >
> > Best regards
> >
> > Heinrich
>
Vincent Stehlé June 24, 2024, 9:14 a.m. UTC | #12
On Fri, Jun 21, 2024 at 07:11:28PM +0200, Caleb Connolly wrote:
> On Fri, 21 Jun 2024, 19:06 Vincent Stehlé, <vincent.stehle@arm.com> wrote:
> 
> > On Fri, Jun 21, 2024 at 01:00:51PM +0200, Heinrich Schuchardt wrote:
> > (..)
> > > 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.
> >
> > Hi Heinrich,
> >
> > Agreed, variant is not concerned by the 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.
> >
> > Thanks! I thought this structure was used to hold either a big-endian UUID
> > or a
> > little-endian GUID, but now you have convinced me.
> >
> > This confirms that the generation of the dynamic GUID is missing something:
> >
> >     gen_uuid_v5(&namespace,
> >                 (struct uuid *)&fw_array[i].image_type_id,
> >                 compatible, strlen(compatible),
> >                 fw_array[i].fw_name, u16_strsize(fw_array[i].fw_name)
> >                     - sizeof(uint16_t),
> >                 NULL);
> >
> > It is not possible to cast the little-endian efi_guid_t .image_type_id as
> > the
> > big-endian struct uuid output of gen_uuid_v5() like this; we need to
> > convert the
> > three time fields from big to little endianness.
> >
> 
> I'm inclined to disagree, the comment above struct uuid in include/uuid.h
> state clearly that the format in memory is always big endian, but that a
> GUID when printed has some fields converted to little endian.

Hi Caleb,

I read the comments above struct uuid differently: the GUID has some fields
little-endian when stored in memory (and can thus not be stored in a struct
uuid after Heinrich's comments).

This is consistent with how it is done in U-Boot in various locations; for
example, the EFI_GUID() macro stores a GUID with its time fields little-endian
in memory. Similarly, the callers of uuid_str_to_bin() or uuid_bin_to_str() with
format UUID_STR_FORMAT_GUID have indeed a little-endian GUID in memory (most
often an efi_guid_t). This is also consistent with the UEFI specification's
16-byte buffer format. [1]

When you have a big-endian UUID in memory, the version field is stored in byte
6, which is consistent with the RFC 9562. [2]
If you convert this big-endian UUID with uuid_bin_to_str() and
UUID_STR_FORMAT_GUID as in genguid, the "time high and version" field's bytes
will be printed with byte 7 first and then byte 6, as per guid_char_order[].

This is in contradiction with the RFC, which shows that the version field ("M")
should be printed first.

If you print the UUID with format 'STD, you will indeed have byte 6 containing
the version field printed first as it should (uuid_char_order[]).

This is confirmed by "uuid -d".

Best regards,
Vincent.

[1] https://uefi.org/specs/UEFI/2.10/Apx_A_GUID_and_Time_Formats.html
[2] https://www.rfc-editor.org/rfc/rfc9562

> 
> 
> > >
> > >
> > > 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.
> >
> > My understanding is that there is a single universal string format for both
> > UUIDs and GUIDs, which uuid -d understands, and which has no notion of
> > endianness. (Only the structures in memory have an endianness.)
> > This means we do not need an output format parameter.
> >
> > genguid is calling the new gen_uuid_v5() function, which outputs a
> > big-endian
> > struct uuid. Therefore, genguid should print it with 'STD format, not
> > 'GUID.
> >
> > Best regards,
> > Vincent.
> >
> > >
> > > Best regards
> > >
> > > Heinrich
> >
Caleb Connolly July 2, 2024, 1:49 p.m. UTC | #13
Hi Vincent,

On 27/06/2024 11:55, Vincent Stehlé wrote:
> Here are the changes that I would like to suggest for the "efi:
> CapsuleUpdate: support for dynamic UUIDs" v3 patch series:
> 
> - Convert from big-endian UUID to little-endian GUID in
>    efi_capsule_update_info_gen_ids().
> 
> - Fix tmp size and masking in gen_uuid_v5().
> 
> - Use UUID_STR_FORMAT_STD in all places where we are dealing with a
>    big-endian UUID.
> 
> - Update all GUIDs constants in the code and in the tests accordingly. This
>    gets rid of the following broken UUIDs:
> 
>      5af91295-5a99-f62b-80d7-e9574de87170
>      8ee418dc-7e00-e156-80a7-274fbbc05ba8
>      935fe837-fac8-4394-c008-737d8852c60d
>      fd5db83c-12f3-a46b-80a9-e3007c7ff56e
>      ffd97379-0956-fa94-c003-8bfcf5cc097b
> 
> - Also, a few minor modifications here and there.

Thanks, this was really helpful for prepping v4. I decided to go with a 
slightly different approach and just make the the v5 generator produce a 
little endian GUID rather than a BE UUID.

V4 is here: 
https://lore.kernel.org/u-boot/20240702-b4-dynamic-uuid-v4-0-a00c82d1f504@linaro.org

Kind regards,
> 
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Caleb Connolly <caleb.connolly@linaro.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Mario Six <mario.six@gdsys.cc>
> Cc: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> Cc: Richard Hughes <hughsient@gmail.com>
> ---
>   include/sandbox_efi_capsule.h                      |  6 +++---
>   lib/efi_loader/efi_firmware.c                      | 14 +++++++++++---
>   lib/uuid.c                                         |  8 ++++----
>   test/lib/uuid.c                                    | 12 ++++++------
>   .../test_efi_capsule/test_capsule_firmware_fit.py  |  4 ++--
>   .../test_efi_capsule/test_capsule_firmware_raw.py  |  8 ++++----
>   .../test_capsule_firmware_signed_fit.py            |  2 +-
>   .../test_capsule_firmware_signed_raw.py            |  4 ++--
>   test/py/tests/test_efi_capsule/version.dts         |  6 +++---
>   tools/.gitignore                                   |  1 +
>   tools/binman/etype/efi_capsule.py                  |  2 +-
>   tools/binman/ftest.py                              |  2 +-
>   tools/genguid.c                                    |  7 +++----
>   13 files changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/include/sandbox_efi_capsule.h b/include/sandbox_efi_capsule.h
> index 25ac496ea24..6f0de5a1e25 100644
> --- a/include/sandbox_efi_capsule.h
> +++ b/include/sandbox_efi_capsule.h
> @@ -6,9 +6,9 @@
>   #if !defined(_SANDBOX_EFI_CAPSULE_H_)
>   #define _SANDBOX_EFI_CAPSULE_H_
>   
> -#define SANDBOX_UBOOT_IMAGE_GUID	"fd5db83c-12f3-a46b-80a9-e3007c7ff56e"
> -#define SANDBOX_UBOOT_ENV_IMAGE_GUID	"935fe837-fac8-4394-c008-737d8852c60d"
> -#define SANDBOX_FIT_IMAGE_GUID		"ffd97379-0956-fa94-c003-8bfcf5cc097b"
> +#define SANDBOX_UBOOT_IMAGE_GUID	"50980990-5af9-5522-86e2-8f05f4d7313c"
> +#define SANDBOX_UBOOT_ENV_IMAGE_GUID	"3554b655-b9f0-5240-ace2-6f34c2f7fcca"
> +#define SANDBOX_FIT_IMAGE_GUID		"8b38adc7-df0c-5769-8b89-c090ca3d07a7"
>   #define SANDBOX_INCORRECT_GUID		"058b7d83-50d5-4c47-a195-60d86ad341c4"
>   
>   #define UBOOT_FIT_IMAGE			"u-boot_bin_env.itb"
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index a8dafe4f01a..f0d0c3fa972 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -258,7 +258,7 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_
>   static efi_status_t efi_capsule_update_info_gen_ids(void)
>   {
>   	int ret, i;
> -	struct uuid namespace;
> +	struct uuid namespace, type;
>   	const char *compatible; /* Full array including null bytes */
>   	struct efi_fw_image *fw_array;
>   
> @@ -269,7 +269,7 @@ static efi_status_t efi_capsule_update_info_gen_ids(void)
>   		return EFI_SUCCESS;
>   
>   	ret = uuid_str_to_bin(CONFIG_EFI_CAPSULE_NAMESPACE_UUID,
> -			(unsigned char *)&namespace, UUID_STR_FORMAT_GUID);
> +			(unsigned char *)&namespace, UUID_STR_FORMAT_STD);
>   	if (ret) {
>   		log_debug("%s: CONFIG_EFI_CAPSULE_NAMESPACE_UUID is invalid: %d\n", __func__, ret);
>   		return EFI_UNSUPPORTED;
> @@ -289,12 +289,20 @@ static efi_status_t efi_capsule_update_info_gen_ids(void)
>   
>   	for (i = 0; i < update_info.num_images; i++) {
>   		gen_uuid_v5(&namespace,
> -			    (struct uuid *)&fw_array[i].image_type_id,
> +			    &type,
>   			    compatible, strlen(compatible),
>   			    fw_array[i].fw_name, u16_strsize(fw_array[i].fw_name)
>   				- sizeof(uint16_t),
>   			    NULL);
>   
> +		/* Convert to little-endian GUID. */
> +		fw_array[i].image_type_id = (efi_guid_t)EFI_GUID(
> +			be32_to_cpu(type.time_low), be16_to_cpu(type.time_mid),
> +			be16_to_cpu(type.time_hi_and_version),
> +			type.clock_seq_hi_and_reserved, type.clock_seq_low,
> +			type.node[0], type.node[1], type.node[2], type.node[3],
> +			type.node[4], type.node[5]);
> +
>   		log_debug("Image %ls UUID %pUs\n", fw_array[i].fw_name,
>   			  &fw_array[i].image_type_id);
>   	}
> diff --git a/lib/uuid.c b/lib/uuid.c
> index 89911b06ccc..a8c3a504090 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -391,7 +391,7 @@ void gen_uuid_v5(const struct uuid *namespace, struct uuid *uuid, ...)
>   	va_list args;
>   	const uint8_t *data;
>   	uint8_t hash[SHA1_SUM_LEN];
> -	uint32_t tmp;
> +	uint16_t tmp;
>   
>   	sha1_starts(&ctx);
>   	/* Hash the namespace UUID as salt */
> @@ -411,11 +411,11 @@ 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 = be16_to_cpu(uuid->time_hi_and_version);
>   	tmp = (tmp & ~UUID_VERSION_MASK) | (5 << UUID_VERSION_SHIFT);
> -	uuid->time_hi_and_version = cpu_to_be32(tmp);
> +	uuid->time_hi_and_version = cpu_to_be16(tmp);
>   
> -	uuid->clock_seq_hi_and_reserved &= UUID_VARIANT_MASK;
> +	uuid->clock_seq_hi_and_reserved &= ~UUID_VARIANT_MASK;
>   	uuid->clock_seq_hi_and_reserved |= UUID_VARIANT << UUID_VARIANT_SHIFT;
>   }
>   #endif
> diff --git a/test/lib/uuid.c b/test/lib/uuid.c
> index 0bcb332e534..b5940fa855c 100644
> --- a/test/lib/uuid.c
> +++ b/test/lib/uuid.c
> @@ -60,7 +60,7 @@ static int lib_test_dynamic_uuid_case(struct unit_test_state *uts,
>   	int j;
>   
>   	ut_assertok(uuid_str_to_bin(data->namespace, (unsigned char *)&namespace,
> -				    UUID_STR_FORMAT_GUID));
> +				    UUID_STR_FORMAT_STD));
>   
>   	for (j = 0; data->images[j]; j++) {
>   		const char *expected_uuid = data->expected_uuids[j];
> @@ -72,7 +72,7 @@ static int lib_test_dynamic_uuid_case(struct unit_test_state *uts,
>   			    data->compatible, strlen(data->compatible),
>   			    image, u16_strsize(image) - sizeof(uint16_t),
>   			    NULL);
> -		uuid_bin_to_str((unsigned char *)&uuid, uuid_str, UUID_STR_FORMAT_GUID);
> +		uuid_bin_to_str((unsigned char *)&uuid, uuid_str, UUID_STR_FORMAT_STD);
>   
>   		ut_asserteq_str(expected_uuid, uuid_str);
>   	}
> @@ -94,9 +94,9 @@ static int lib_test_dynamic_uuid(struct unit_test_state *uts)
>   				NULL,
>   			},
>   			.expected_uuids = {
> -				"fd5db83c-12f3-a46b-80a9-e3007c7ff56e",
> -				"935fe837-fac8-4394-c008-737d8852c60d",
> -				"ffd97379-0956-fa94-c003-8bfcf5cc097b",
> +				"50980990-5af9-5522-86e2-8f05f4d7313c",
> +				"3554b655-b9f0-5240-ace2-6f34c2f7fcca",
> +				"8b38adc7-df0c-5769-8b89-c090ca3d07a7",
>   				NULL,
>   			}
>   		},
> @@ -108,7 +108,7 @@ static int lib_test_dynamic_uuid(struct unit_test_state *uts)
>   				NULL,
>   			},
>   			.expected_uuids = {
> -				"8ee418dc-7e00-e156-80a7-274fbbc05ba8",
> +				"14c399c8-4e16-5ba4-b720-44426d3a0bb9",
>   				NULL,
>   			}
>   		},
> diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
> index 746da460208..9701acebbe3 100644
> --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
> +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_fit.py
> @@ -146,8 +146,8 @@ class TestEfiCapsuleFirmwareFit():
>                   verify_content(u_boot_console, '100000', 'u-boot:Old')
>                   verify_content(u_boot_console, '150000', 'u-boot-env:Old')
>               else:
> -                # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT.
> -                assert '5AF91295-5A99-F62B-80D7-E9574DE87170' in ''.join(output)
> +                # ensure that SANDBOX_FIT_IMAGE_GUID is in the ESRT.
> +                assert '8B38ADC7-DF0C-5769-8B89-C090CA3D07A7' in ''.join(output)
>                   assert 'ESRT: fw_version=5' in ''.join(output)
>                   assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
>   
> diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py
> index 1866b808657..cedb3a43591 100644
> --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py
> +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_raw.py
> @@ -134,10 +134,10 @@ class TestEfiCapsuleFirmwareRaw:
>                   'efidebug capsule esrt'])
>   
>               # ensure that SANDBOX_UBOOT_ENV_IMAGE_GUID is in the ESRT.
> -            assert '935FE837-FAC8-4394-C008-737D8852C60D' in ''.join(output)
> +            assert '3554B655-B9F0-5240-ACE2-6F34C2F7FCCA' in ''.join(output)
>   
>               # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT.
> -            assert 'FD5DB83C-12F3-A46B-80A9-E3007C7FF56E' in ''.join(output)
> +            assert '50980990-5AF9-5522-86E2-8F05F4D7313C' in ''.join(output)
>   
>               check_file_removed(u_boot_console, disk_img, capsule_files)
>   
> @@ -188,12 +188,12 @@ class TestEfiCapsuleFirmwareRaw:
>                   verify_content(u_boot_console, '150000', 'u-boot-env:Old')
>               else:
>                   # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT.
> -                assert 'FD5DB83C-12F3-A46B-80A9-E3007C7FF56E' in ''.join(output)
> +                assert '50980990-5AF9-5522-86E2-8F05F4D7313C' in ''.join(output)
>                   assert 'ESRT: fw_version=5' in ''.join(output)
>                   assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
>   
>                   # ensure that SANDBOX_UBOOT_ENV_IMAGE_GUID is in the ESRT.
> -                assert '935FE837-FAC8-4394-C008-737D8852C60D' in ''.join(output)
> +                assert '3554B655-B9F0-5240-ACE2-6F34C2F7FCCA' in ''.join(output)
>                   assert 'ESRT: fw_version=10' in ''.join(output)
>                   assert 'ESRT: lowest_supported_fw_version=7' in ''.join(output)
>   
> diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
> index a4e0a3bc73f..10eb8281457 100644
> --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
> +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_fit.py
> @@ -157,7 +157,7 @@ class TestEfiCapsuleFirmwareSignedFit():
>                   'efidebug capsule esrt'])
>   
>               # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT.
> -            assert 'FD5DB83C-12F3-A46B-80A9-E3007C7FF56E' in ''.join(output)
> +            assert '8B38ADC7-DF0C-5769-8B89-C090CA3D07A7' in ''.join(output)
>               assert 'ESRT: fw_version=5' in ''.join(output)
>               assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
>   
> diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
> index 260c7186063..01e5f3b3405 100644
> --- a/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
> +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware_signed_raw.py
> @@ -151,12 +151,12 @@ class TestEfiCapsuleFirmwareSignedRaw():
>                   'efidebug capsule esrt'])
>   
>               # ensure that SANDBOX_UBOOT_IMAGE_GUID is in the ESRT.
> -            assert 'FD5DB83C-12F3-A46B-80A9-E3007C7FF56E' in ''.join(output)
> +            assert '50980990-5AF9-5522-86E2-8F05F4D7313C' in ''.join(output)
>               assert 'ESRT: fw_version=5' in ''.join(output)
>               assert 'ESRT: lowest_supported_fw_version=3' in ''.join(output)
>   
>               # ensure that SANDBOX_UBOOT_ENV_IMAGE_GUID is in the ESRT.
> -            assert '935FE837-FAC8-4394-C008-737D8852C60D' in ''.join(output)
> +            assert '3554B655-B9F0-5240-ACE2-6F34C2F7FCCA' in ''.join(output)
>               assert 'ESRT: fw_version=10' in ''.join(output)
>               assert 'ESRT: lowest_supported_fw_version=7' in ''.join(output)
>   
> diff --git a/test/py/tests/test_efi_capsule/version.dts b/test/py/tests/test_efi_capsule/version.dts
> index 3f0698bf728..c447a3d8199 100644
> --- a/test/py/tests/test_efi_capsule/version.dts
> +++ b/test/py/tests/test_efi_capsule/version.dts
> @@ -8,17 +8,17 @@
>   		image1 {
>   			lowest-supported-version = <3>;
>   			image-index = <1>;
> -			image-type-id = "FD5DB83C-12F3-A46B-80A9-E3007C7FF56E";
> +			image-type-id = "50980990-5AF9-5522-86E2-8F05F4D7313C";
>   		};
>   		image2 {
>   			lowest-supported-version = <7>;
>   			image-index = <2>;
> -			image-type-id = "935FE837-FAC8-4394-C008-737D8852C60D";
> +			image-type-id = "3554B655-B9F0-5240-ACE2-6F34C2F7FCCA";
>   		};
>   		image3 {
>   			lowest-supported-version = <3>;
>   			image-index = <1>;
> -			image-type-id = "FFD97379-0956-FA94-C003-8BFCF5CC097B";
> +			image-type-id = "8B38ADC7-DF0C-5769-8B89-C090CA3D07A7";
>   		};
>   	};
>   };
> diff --git a/tools/.gitignore b/tools/.gitignore
> index 0108c567309..6b7d7b89c39 100644
> --- a/tools/.gitignore
> +++ b/tools/.gitignore
> @@ -15,6 +15,7 @@
>   /gdb/gdbsend
>   /gen_eth_addr
>   /gen_ethaddr_crc
> +/genguid
>   /ifdtool
>   /ifwitool
>   /img2srec
> diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py
> index da1f9b0a381..f9f4fda5f71 100644
> --- a/tools/binman/etype/efi_capsule.py
> +++ b/tools/binman/etype/efi_capsule.py
> @@ -24,7 +24,7 @@ def get_binman_test_guid(type_str):
>           The actual GUID value (str)
>       """
>       TYPE_TO_GUID = {
> -        'binman-test' : 'fd5db83c-12f3-a46b-80a9-e3007c7ff56e'
> +        'binman-test' : '50980990-5af9-5522-86e2-8f05f4d7313c'
>       }
>   
>       return TYPE_TO_GUID[type_str]
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index dc602b95ecd..5610afc26de 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -124,7 +124,7 @@ TEE_ADDR = 0x5678
>   # Firmware Management Protocol(FMP) GUID
>   FW_MGMT_GUID = '6dcbd5ed-e82d-4c44-bda1-7194199ad92a'
>   # Image GUID specified in the DTS
> -CAPSULE_IMAGE_GUID = 'fd5db83c-12f3-a46b-80a9-e3007c7ff56e'
> +CAPSULE_IMAGE_GUID = '50980990-5af9-5522-86e2-8f05f4d7313c'
>   # Windows cert GUID
>   WIN_CERT_TYPE_EFI_GUID = '4aafd29d-68df-49ee-8aa9-347d375665a7'
>   # Empty capsule GUIDs
> diff --git a/tools/genguid.c b/tools/genguid.c
> index e71bc1d48f9..1e365399721 100644
> --- a/tools/genguid.c
> +++ b/tools/genguid.c
> @@ -15,7 +15,6 @@
>   #include <uuid.h>
>   
>   static struct option options[] = {
> -	{"dtb", required_argument, NULL, 'd'},
>   	{"compat", required_argument, NULL, 'c'},
>   	{"help", no_argument, NULL, 'h'},
>   	{"verbose", no_argument, NULL, 'v'},
> @@ -99,7 +98,7 @@ int main(int argc, char **argv)
>   		return 1;
>   	}
>   
> -	if (uuid_str_to_bin(namespace_str, (unsigned char *)&namespace, UUID_STR_FORMAT_GUID)) {
> +	if (uuid_str_to_bin(namespace_str, (unsigned char *)&namespace, UUID_STR_FORMAT_STD)) {
>   		fprintf(stderr, "ERROR: Check that your UUID is formatted correctly.\n");
>   		exit(EXIT_FAILURE);
>   	}
> @@ -116,7 +115,7 @@ int main(int argc, char **argv)
>   
>   	if (debug) {
>   		fprintf(stderr, "GUID:         ");
> -		uuid_bin_to_str((uint8_t *)&namespace, uuid_str, UUID_STR_FORMAT_GUID);
> +		uuid_bin_to_str((uint8_t *)&namespace, uuid_str, UUID_STR_FORMAT_STD);
>   		fprintf(stderr, "%s\n", uuid_str);
>   		fprintf(stderr, "Compatible:  \"%s\"\n", compatible);
>   		fprintf(stderr, "Images:      ");
> @@ -134,7 +133,7 @@ int main(int argc, char **argv)
>   			    images_u16[i], u16_strsize(images_u16[i]) - sizeof(uint16_t),
>   			    NULL);
>   
> -		uuid_bin_to_str((uint8_t *)&image_type_id, uuid_str, UUID_STR_FORMAT_GUID);
> +		uuid_bin_to_str((uint8_t *)&image_type_id, uuid_str, UUID_STR_FORMAT_STD);
>   		image_uuids[i] = strdup(uuid_str);
>   	}
>