[PATCH] efi_loader: FMP cleanups
AKASHI Takahiro
takahiro.akashi at linaro.org
Tue Jun 15 06:44:58 CEST 2021
On Tue, Jun 15, 2021 at 06:55:50AM +0300, Ilias Apalodimas wrote:
> Akashi-san,
>
> On Tue, Jun 15, 2021 at 10:51:01AM +0900, AKASHI Takahiro wrote:
> > Ilias,
> >
> > In this patch, you are trying to address a couple of independent
> > issues in a single commit.
> > Please split.
> > (Heinrich doesn't like that.)
Any comment?
> > On Mon, Jun 14, 2021 at 06:10:14PM +0300, Ilias Apalodimas wrote:
> > > Right now we allow both of the FMPs (RAW and FIT based) to be installed at
> > > the same time. Moreover we only install those if a CapsuleUpdate is
> > > requested. Since we now have an ESRT table, it makes more sense to
> > > unconditionally install the FMP, so any userspace applications (e.g fwupd)
> > > can make use of them and trigger an update.
> > >
> > > While at it clean up the FMP installation as well. Chapter 23 of the EFI
> > > spec (rev 2.9) says:
> > > "A specific updatable hardware firmware store must be represented by
> > > exactly one FMP instance".
> > > This is not the case for us, since both of our FMP protocols can be
> > > installed at the same time and are controlled by a single 'dfu_alt_info'
> > > env variable.
> > > So make the config option a choice and allow the user to install one
> > > of them at any given time.
> >
> > I'd like to say nak in some respects:
> > - Although I do understand the UEFI requirement that you mentioned above,
> > FIT and RAW FMP drivers can handle *different* firmware even though
> > they share the same dfu_alt_info.
>
> How ?
One idea that I can imagine is that we may be able to utilize
"update_image_index", which is currently not used effectively,
in order to specify which firmware in dfu_alt_info be handled
by either FIT FMP or RAW FMP.
> > We should not impose unnecessary restriction if we manage to have some
> > workaround to meet the requirement.
>
> It's not the updating part only. It's that the .get_image_info also relies on
> the same env variable.
The above idea can and should be applied to GetImageInfo implementation
at the same time.
> Specifically in the fwupd case on an RPI4 with the
> dfu set at 'dfu_alt_info=u-boot.bin fat 0 1;' although 2 ERSTs entries were
> populated only one was reported. Probably because this really does give you
> 2 ways of updating the same flash.
>
> > (I still believe that the firmware definition for ESRT should exist
> > elsewhere other than in FMP themselves.)
>
> That's a whole different story, and if we have that, then .get_image_info
> should change as well instead of using the DFU information.
I don't think so as I mentioned above.
> Because right
> now we enabled security (or think we have), while allowing users to set an env
> variable which is not authenticated, and completely change what the
> firmware reports (or updates).
This is the point that I mentioned earlier in our private chat,
and it's a "whole different" story in this context.
> We can always add a huge warning saying
> something along the lines of "If you really care this should come with a
> CONFIG_ENV_IS_NOWHERE and a boot timeout set to -1".
>
> The spec is pretty clear and we allow users to *break* it with a config
> option. Arguably this is fine, since the code continues to work fine and
> you can perform the updates, but in essence RAW and FITs are used to update
> the same medium right now. You can't have a capsule with half it's contents
> describing something RAW and the other half being a FIT. You have a FIT based
> capsule or a RAW based capsule.
See above.
> > - We should allow users to add their own FMP drivers and so not call
> > [arch_]efi_load_capsule_drivers() unconditionally
> > even if you don't like "__weak" attribute.
>
> I am fine with the __weak attribute. On the other hand I consider the
> current code the defacto way users would use to update their firmware. That's
> why I removed the __weak attribute. If a hardware vendor was to update
> their special PCI option ROM or a flash that lives on the secure world they
> should install their FMPs on a different handle and leave the current code
> as is.
And we should provide an option that disables these existing handle.
> > - Selecting only one of FIT and RAW FMPs in sandbox*_defconfig will
> > leave some test cases in pytest skipped.
>
> Yea that's unfortunate, but maybe we can just add an extra config on the
> sandbox?
Please add another patch that is missing.
-Takahiro Akashi
> Thanks
> /Ilias
>
> >
> > -Takahiro Akashi
> >
> > > The overall changes show up in fwupd
> > >
> > > pre-patch:
> > > fwupdmgr get-devices
> > > No detected devices
> > >
> > > post-patch (with FIT FMP installed):
> > > fwupdmgr get-devices
> > > WARNING: Required efivarfs filesystem was not found
> > > See https://github.com/fwupd/fwupd/wiki/PluginFlag:efivar-not-mounted for more information.
> > > Unknown Product
> > > │
> > > └─Unknown Firmware:
> > > Device ID: 605080e08f71dabb86d0781c28f7d923edabf7d6
> > > Current version: 0
> > > Vendor: DMI:U-Boot
> > > Update Error: Not updatable as efivarfs was not found
> > > GUIDs: ae13ff2d-9ad4-4e25-9ac8-6d80b3b22147
> > > 230c8b18-8d9b-53ec-838b-6cfc0383493a ← main-system-firmware
> > > 1a1da7d4-0a24-51b5-8a1a-1e3274328094 ← UEFI\RES_{AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147}
> > > Device Flags: • Internal device
> > > • System requires external power source
> > > • Needs a reboot after installation
> > > • Device is usable for the duration of the update
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > ---
> > > configs/sandbox64_defconfig | 1 -
> > > configs/sandbox_defconfig | 1 -
> > > configs/xilinx_zynqmp_virt_defconfig | 1 -
> > > include/efi_loader.h | 1 +
> > > lib/efi_loader/Kconfig | 48 +++++++++++++++-------------
> > > lib/efi_loader/efi_capsule.c | 22 ++++---------
> > > lib/efi_loader/efi_setup.c | 4 +++
> > > 7 files changed, 37 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> > > index 9a373bab6fe3..af18b6c7826e 100644
> > > --- a/configs/sandbox64_defconfig
> > > +++ b/configs/sandbox64_defconfig
> > > @@ -233,7 +233,6 @@ CONFIG_LZ4=y
> > > CONFIG_ERRNO_STR=y
> > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > CONFIG_EFI_CAPSULE_ON_DISK=y
> > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > CONFIG_EFI_SECURE_BOOT=y
> > > CONFIG_TEST_FDTDEC=y
> > > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > > index bdbf714e2bd9..24313fdfa53d 100644
> > > --- a/configs/sandbox_defconfig
> > > +++ b/configs/sandbox_defconfig
> > > @@ -280,7 +280,6 @@ CONFIG_LZ4=y
> > > CONFIG_ERRNO_STR=y
> > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > CONFIG_EFI_CAPSULE_ON_DISK=y
> > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > CONFIG_EFI_SECURE_BOOT=y
> > > CONFIG_TEST_FDTDEC=y
> > > diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> > > index e939b04ef6a5..0c2d1a70a5a1 100644
> > > --- a/configs/xilinx_zynqmp_virt_defconfig
> > > +++ b/configs/xilinx_zynqmp_virt_defconfig
> > > @@ -188,5 +188,4 @@ CONFIG_EFI_SET_TIME=y
> > > CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > CONFIG_EFI_CAPSULE_ON_DISK=y
> > > CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y
> > > -CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 0a9c82a257e1..b81180cfda8b 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -972,4 +972,5 @@ efi_status_t efi_esrt_register(void);
> > > * - error code otherwise.
> > > */
> > > efi_status_t efi_esrt_populate(void);
> > > +efi_status_t efi_load_capsule_drivers(void);
> > > #endif /* _EFI_LOADER_H */
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index 6242caceb7f9..da6f5faf5adb 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -161,6 +161,31 @@ config EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > Select this option if you want to enable capsule-based
> > > firmware update using Firmware Management Protocol.
> > >
> > > +choice EFI_CAPSULE_TYPE
> > > + prompt "Capsule type (RAW/FIT)"
> > > + depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > +
> > > +config EFI_CAPSULE_FIRMWARE_FIT
> > > + bool "FMP driver for FIT images"
> > > + depends on FIT
> > > + select UPDATE_FIT
> > > + select DFU
> > > + select EFI_CAPSULE_FIRMWARE
> > > + help
> > > + Select this option if you want to enable firmware management protocol
> > > + driver for FIT image
> > > +
> > > +config EFI_CAPSULE_FIRMWARE_RAW
> > > + bool "FMP driver for raw images"
> > > + select DFU_WRITE_ALT
> > > + select DFU
> > > + select EFI_CAPSULE_FIRMWARE
> > > + help
> > > + Select this option if you want to enable firmware management protocol
> > > + driver for raw image
> > > +
> > > +endchoice
> > > +
> > > config EFI_CAPSULE_AUTHENTICATE
> > > bool "Update Capsule authentication"
> > > depends on EFI_CAPSULE_FIRMWARE
> > > @@ -181,29 +206,6 @@ config EFI_CAPSULE_AUTHENTICATE
> > > Select this option if you want to enable capsule
> > > authentication
> > >
> > > -config EFI_CAPSULE_FIRMWARE_FIT
> > > - bool "FMP driver for FIT image"
> > > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > - depends on FIT
> > > - select UPDATE_FIT
> > > - select DFU
> > > - select EFI_CAPSULE_FIRMWARE
> > > - default n
> > > - help
> > > - Select this option if you want to enable firmware management protocol
> > > - driver for FIT image
> > > -
> > > -config EFI_CAPSULE_FIRMWARE_RAW
> > > - bool "FMP driver for raw image"
> > > - depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
> > > - select DFU
> > > - select DFU_WRITE_ALT
> > > - select EFI_CAPSULE_FIRMWARE
> > > - default n
> > > - help
> > > - Select this option if you want to enable firmware management protocol
> > > - driver for raw image
> > > -
> > > config EFI_DEVICE_PATH_TO_TEXT
> > > bool "Device path to text protocol"
> > > default y
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 9ead0d2c7816..3b4214a8d4cc 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -918,31 +918,27 @@ static void efi_capsule_scan_done(void)
> > > }
> > >
> > > /**
> > > - * arch_efi_load_capsule_drivers - initialize capsule drivers
> > > + * efi_load_capsule_drivers - initialize capsule drivers
> > > *
> > > - * Architecture or board specific initialization routine
> > > + * Generic FMP drivers backed by DFU
> > > *
> > > * Return: status code
> > > */
> > > -efi_status_t __weak arch_efi_load_capsule_drivers(void)
> > > +efi_status_t efi_load_capsule_drivers(void)
> > > {
> > > - __maybe_unused efi_handle_t handle;
> > > + __maybe_unused efi_handle_t handle = NULL;
> > > efi_status_t ret = EFI_SUCCESS;
> > >
> > > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> > > - handle = NULL;
> > > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT))
> > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > > &handle, &efi_guid_firmware_management_protocol,
> > > &efi_fmp_fit, NULL));
> > > - }
> > >
> > > - if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> > > - handle = NULL;
> > > + if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW))
> > > ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> > > - &efi_root,
> > > + &handle,
> > > &efi_guid_firmware_management_protocol,
> > > &efi_fmp_raw, NULL));
> > > - }
> > >
> > > return ret;
> > > }
> > > @@ -975,10 +971,6 @@ efi_status_t efi_launch_capsules(void)
> > >
> > > index = get_last_capsule();
> > >
> > > - /* Load capsule drivers */
> > > - ret = arch_efi_load_capsule_drivers();
> > > - if (ret != EFI_SUCCESS)
> > > - return ret;
> > >
> > > /*
> > > * Find capsules on disk.
> > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > index 3c5cf9a4357e..0106efdc161b 100644
> > > --- a/lib/efi_loader/efi_setup.c
> > > +++ b/lib/efi_loader/efi_setup.c
> > > @@ -254,6 +254,10 @@ efi_status_t efi_init_obj_list(void)
> > > if (ret != EFI_SUCCESS)
> > > goto out;
> > >
> > > + ret = efi_load_capsule_drivers();
> > > + if (ret != EFI_SUCCESS)
> > > + goto out;
> > > +
> > > #if defined(CONFIG_LCD) || defined(CONFIG_DM_VIDEO)
> > > ret = efi_gop_register();
> > > if (ret != EFI_SUCCESS)
> > > --
> > > 2.32.0.rc0
> > >
More information about the U-Boot
mailing list