[PATCH 2/5] scripts/Makefile.lib: Embed capsule public key in platform's dtb

[Sughosh Ganu]

hi Simon,

On Wed, 16 Aug 2023 at 00:09, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 15 Aug 2023 at 10:26, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > The EFI capsule authentication logic in u-boot expects the public key
> > in the form of an EFI Signature List(ESL) to be provided as part of
> > the platform's dtb. Currently, the embedding of the ESL file into the
> > dtb needs to be done manually.
> >
> > Add a target for generating a dtsi file which contains the signature
> > node with the ESL file included as a property under the signature
> > node. Include the dtsi file in the dtb. This brings the embedding of
> > the ESL in the dtb into the U-Boot build flow.
> >
> > The path to the ESL file is specified through the
> > CONFIG_EFI_CAPSULE_ESL_FILE symbol.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > Changes since RFC series:
> > * Remove the default value of the config symbol.
> > * s/include_files/dtsi_include_list
> > * Add all the dtsi files being included as dependency for the dtb
> >   target.
> >
> >  lib/efi_loader/Kconfig             |  8 ++++++++
> >  lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
> >  scripts/Makefile.lib               | 18 +++++++++++++++++-
> >  3 files changed, 36 insertions(+), 1 deletion(-)
> >  create mode 100644 lib/efi_loader/capsule_esl.dtsi.in
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 9989e3f384..d20aaab6db 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -272,6 +272,14 @@ config EFI_CAPSULE_MAX
> >           Select the max capsule index value used for capsule report
> >           variables. This value is used to create CapsuleMax variable.
> >
> > +config EFI_CAPSULE_ESL_FILE
> > +       string "Path to the EFI Signature List File"
>
> Do we need this, or could we name it as we do with the .env file? It
> seems confusing to have to set this for each board - it might be
> better to have it in a defined location.

The reason I put this is because I thought this gave the user the
flexibility to provide the location and name of the ESL. But I suppose
that the board directory would be a good location to expect this file.
Then this file can have a name like capsule_pub_key,esl. Tom, what are
your thoughts on this?

>
> Another idea is that we could use binman to pull this in, e.g. with an
> option to insert the capsule key during the build. Then it can be
> anywhere on the binman path.

So, if I understand you right, I believe you are suggesting using
binman for this task? Which, just to make it clear, I pretty much
understand what would be needed to be done to get this working in
binman. A typical binman image for for this task would look like

&binman {
        u-boot-capsule-esl {
                u-boot-no-dtb {
                };
                fdt-esl {
                        u-boot-dtb {
                        };
                };
        };
};

The fdt-esl entry type would then call the fdt_add_pubkey tool to do
the needful. We also support capsule update for FIT images, and that
would mean adding some logic to call the fdt_add_pubkey for the
fdt_list. So implementing this is not a difficult thing at all.

But the main issue, or rather the only issue with this is that we are
getting an image with a different name, like u-boot-capsule-esl.bin as
the resulting image. But like I had mentioned earlier in our
discussions, we have platforms which boot the u-boot.bin binary. We
also have the ST boards which generate the fip image and use the
u-boot.dtb as the BL33_CFG. So generating a new image binary, or a new
DTB would break these boards. Not to mention that these platforms
would not be interested in using a different image just because they
are enabling some functionality on the platform.

>
> As you know I am not a fan of these opaque binaries when we have a
> nice self-describing format like devicetre. But we can worry about
> that problem another time.

I know, but this is what we have when we follow the UEFI
specification. It is not our design.

-sughosh

>
> > +       depends on EFI_CAPSULE_AUTHENTICATE
> > +       help
> > +         Provides the path to the EFI Signature List file which will
> > +         be embedded in the platform's device tree and used for
> > +         capsule authentication at the time of capsule update.
> > +
> >  config EFI_DEVICE_PATH_TO_TEXT
> >         bool "Device path to text protocol"
> >         default y
> > diff --git a/lib/efi_loader/capsule_esl.dtsi.in b/lib/efi_loader/capsule_esl.dtsi.in
> > new file mode 100644
> > index 0000000000..61a9f2b25e
> > --- /dev/null
> > +++ b/lib/efi_loader/capsule_esl.dtsi.in
> > @@ -0,0 +1,11 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/**
> > + * Devicetree file with the public key EFI Signature List(ESL)
> > + * node. This file is used to generate the dtsi file to be
> > + * included into the DTB.
> > +*/
> > +/ {
> > +       signature {
> > +               capsule-key = /incbin/("ESL_BIN_FILE");
> > +       };
> > +};
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 368b5a3e28..2e71f190bc 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -334,7 +334,23 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> >                 ; \
> >         sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
> >
> > -$(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> > +ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> > +quiet_cmd_capsule_esl_gen = CAPSULE_ESL_GEN $@
> > +cmd_capsule_esl_gen = \
> > +       $(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" $(capsule_esl_input_file) > $@)
> > +
> > +$(obj)/.capsule_esl.dtsi:
> > +       $(call cmd_capsule_esl_gen)
> > +
> > +capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in
> > +capsule_esl_dtsi = .capsule_esl.dtsi
> > +capsule_esl_path=$(abspath $(srctree)/$(subst $(quote),,$(CONFIG_EFI_CAPSULE_ESL_FILE)))
> > +dtsi_include_list += $(capsule_esl_dtsi)
> > +endif
> > +
> > +dtsi_include_list_deps = $(addprefix $(obj)/,$(subst $(quote),,$(dtsi_include_list)))
> > +
> > +$(obj)/%.dtb: $(src)/%.dts $(DTC) $(dtsi_include_list_deps) FORCE
> >         $(call if_changed_dep,dtc)
> >
> >  pre-tmp = $(subst $(comma),_,$(dot-target).pre.tmp)
> > --
> > 2.34.1
> >
>
> Regards,
> Simon