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

Fri Mar 25 11:41:29 CET 2022

Sughosh,

On Fri, Mar 25, 2022 at 03:14:20PM +0530, Sughosh Ganu wrote:
> hi Takahiro,
> 
> On Fri, 25 Mar 2022 at 10:23, AKASHI Takahiro
> <takahiro.akashi at linaro.org> wrote:
> >
> > 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".
> 
> Yes, but unless we have a single image running on multiple platforms,
> we do need different GUID values 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.)
> 
> Currently, with this patch series, that is what is happening. The
> efi_fw_images array gets read by the GetImageInfo function, and that
> information gets used for populating the ESRT. Btw, I also want to
> extend this structure as part of a separate task, where we have
> information related to the dfu framework as part of the structure.
> Then the capsule related dfu information can be populated from this
> structure, instead of the dfu_alt_info env variable. This is what
> Ilias has suggested. But I will take this up as a separate exercise.
> 
> >
> > > 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?
> 
> Yes, this will still work. Only change is that every platform will
> have a separate image GUID for the FIT image.

Ok.

> >
> > > 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.
> 
> Yes, I am aware of this. I was required to do this since we cannot
> have both instances of the FMP. I will move the FIT test into a
> separate script.

Instead, please add a *new* test script, or separate test "class", for your changes.

I think that you should mention remaining issues or TODO items in your cover letter.

> >
> > I expect that you should maintain FIT FMP driver properly and it be
> > tested as before.
> 
> Okay. Although, I wanted to check if you think we need to have support
> for updating the FIT image.

Yes, I think so.
FIT format has been long used, recognized as a standard on U-Boot
and well integrated with DFU framework.
That is why we can simply call fit_update() in SetImage API.
So having FIT FMP driver is a good migration path.
Moreover, treating binaries as a single capsule ensures that all the firmware
can and should be updated atomically and simultaneously.

-Takahiro Akashi

> The individual images can very much be
> updated through the raw FMP instance. And the raw images also map to
> the ESRT, which is not the case with the FIT image.
> 
> > # Moreover, you have not yet added capsule authentication support
> > to FIT FMP driver.
> 
> Yes, I have not used an FIT image in my testing up until now, although
> it should not be very difficult. I will keep this on my Todo list and
> will add support once this gets upstreamed.
> 
> -sughosh
> 
> >
> > -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
> > >
> > >