Re: [RFC PATCH 0/6] efi: capsule: Image GUID usage cleanup

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Sughosh,

On Thu, Mar 24, 2022 at 06:08:55PM +0530, Sughosh Ganu wrote:
> 
> This series is cleaning up the usage of the image GUIDs that are used
> in capsule update and the EFI System Resource Table(ESRT).
> 
> Currently, there are two instances of the Firmware Management
> Protocol(FMP), one defined for updating the FIT images, and the other
> for updating raw images. The FMP code defines two GUID values, one for
> all FIT images, and one for raw images. Depending on the FMP instance
> used on a platform, the platform needs to use the corresponding image
> GUID value for all images on the platform, and also across platforms.
> 
> A few issues are being fixed through the patch series. One, that an
> image for a different platform can be flashed on another platform if
> both the platforms are using the same FMP instance. So, for e.g. a
> capsule generated for the Socionext DeveloperBox platform can be
> flashed on the ZynqMP platform, since both the platforms use the
> CONFIG_EFI_CAPSULE_FIRMWARE_RAW instance of the FMP. This can be
> corrected if each firmware image that can be updated through the
> capsule update mechanism has it's own unique image GUID.

Ok although the specification doesn't explicitly require the uniqueness
"across platforms".

> The second issue that this patch series fixes is the value of FwClass
> in the ESRT. With the current logic, all firmware image entries in the
> ESRT display the same GUID value -- either the FIT GUID or the raw
> GUID. This is not in compliance with the UEFI specification, as the
> specification requires all entries to have unique GUID values.

Well, this is *not* a problem of fit FMP driver, but a matter of how
ESRT is populated. Table 23-16 "ESRT and FMP Fields" says,
        The ImageTypeId GUID from the Firmware
        Management Protocol instance for a device is
        used as the Firmware Class GUID in the ESRT.
        Where there are multiple identical devices in
        the system, system firmware must create a
        mapping to ensure that the ESRT FwClass
        GUIDs are unique and consistent.
The second sentence suggests that UEFI implementation, i.e.
efi_esrt_populate(), may and should allow some *mapping*.

That said, I basically accept the requirement that you mention above.

> The third issue being fixed is the population of the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR array. The current code uses the dfu
> framework for populating the image descriptor array. However, there
> might be other images that are not to be updated through the capsule
> update mechanism also registered with the dfu framework. As a result
> of this, the ESRT will show up entries of images that are not to be
> targeted by the capsule update mechanism.
> 
> These issues are being fixed by defining a structure, efi_fw_images. A
> platform can then define image related information like the image GUID
> and image name. Every platform that uses capsule update mechanism
> needs to define fw_images array. This array will then be used to
> populate the image descriptor array, and also in determining if a
> particular capsule's payload can be used for updating an image on the
> platform.

When ESRT support patch was posted, I proposed that we should have
a kind of configuration table that defines all the firmware on the system
for ESRT, but this proposal was rejected.
Your efi_fw_images[] looks quite similar as what I had in mind.
Why not define efi_fw_images[] as ESRT itself.
(Of course, some fields in an entry can still be populated through
GetImageInfo API.)

> The first patch of this series adds the fw_images array in all
> platforms which are using UEFI capsule updates
> 
> The second patch of the series changes the logic for populating the
> image descriptor array, using the information from the fw_images array
> defined by the platform.

So a FIT image can still work as a single binary of firmware
under FIT FMP driver. Correct?

> The third patch of the series removes the test cases using the --raw
> and --fit parameters, removes test case for FIT images, and adds a
> test case for checking that the update happens only with the correct
> image GUID value in the capsule.

Your change in test_capsule_firmware.py tries to remove FIT FMP
driver test and it eventually hides the fact that FIT driver may get broken.

I expect that you should maintain FIT FMP driver properly and it be
tested as before.
# Moreover, you have not yet added capsule authentication support
to FIT FMP driver.

-Takahiro Akashi

> 
> The fourth patch of the series makes corresponding changes in the
> capsule update related documentation.
> 
> The fifth patch of the series removes the now unused FIT and raw image
> GUID values from the FMP module.
> 
> The sixth patch of the series removes the --raw and --fit command line
> parameters in the mkeficapsule utility.
> 
> 
> Sughosh Ganu (6):
>   capsule: Add Image GUIDs for platforms using capsule updates
>   capsule: FMP: Populate the image descriptor array from platform data
>   test: capsule: Modify the capsule tests to use GUID values for sandbox
>   doc: uefi: Update the capsule update related documentation
>   FMP: Remove GUIDs for FIT and raw images
>   mkeficapsule: Remove raw and FIT GUID types
> 
>  .../imx8mp_rsb3720a1/imx8mp_rsb3720a1.c       |  19 ++
>  .../imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c   |  18 ++
>  board/emulation/qemu-arm/qemu-arm.c           |  20 +++
>  board/kontron/pitx_imx8m/pitx_imx8m.c         |  15 +-
>  board/kontron/sl-mx8mm/sl-mx8mm.c             |  14 ++
>  board/kontron/sl28/sl28.c                     |  14 ++
>  board/sandbox/sandbox.c                       |  17 ++
>  board/socionext/developerbox/developerbox.c   |  23 +++
>  board/xilinx/common/board.h                   |  18 ++
>  board/xilinx/zynq/board.c                     |  18 ++
>  board/xilinx/zynqmp/zynqmp.c                  |  18 ++
>  configs/sandbox64_defconfig                   |   1 -
>  configs/sandbox_defconfig                     |   1 -
>  doc/develop/uefi/uefi.rst                     |  10 +-
>  include/configs/imx8mm-cl-iot-gate.h          |  10 ++
>  include/configs/imx8mp_rsb3720.h              |  10 ++
>  include/configs/kontron-sl-mx8mm.h            |   6 +
>  include/configs/kontron_pitx_imx8m.h          |   6 +
>  include/configs/kontron_sl28.h                |   6 +
>  include/configs/qemu-arm.h                    |  10 ++
>  include/configs/sandbox.h                     |  10 ++
>  include/configs/synquacer.h                   |  14 ++
>  include/efi_api.h                             |   8 -
>  include/efi_loader.h                          |  18 ++
>  lib/efi_loader/efi_firmware.c                 |  95 +++-------
>  test/py/tests/test_efi_capsule/conftest.py    |  20 +--
>  .../test_efi_capsule/test_capsule_firmware.py | 167 ++++++------------
>  tools/eficapsule.h                            |   8 -
>  tools/mkeficapsule.c                          |  26 +--
>  29 files changed, 384 insertions(+), 236 deletions(-)
> 
> -- 
> 2.25.1
> 
>