[PATCH v7 04/11] capsule: authenticate: Add capsule public key in platform's dtb

[Sughosh Ganu]

hi Simon,

On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
> On Sun, 6 Aug 2023 at 11:25, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Sun, Aug 06, 2023 at 04:48:11PM +0530, Sughosh Ganu wrote:
> > > On Sun, 6 Aug 2023 at 03:42, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Sat, Aug 05, 2023 at 09:03:53AM -0600, Simon Glass wrote:
> > > > > Hi Sughosh,
> > > > >
> > > > > On Sat, 5 Aug 2023 at 05:35, 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 signature node in the u-boot dtsi file and include the public
> > > > > > key through the capsule-key property. This file is per architecture,
> > > > > > and is currently being added for sandbox and arm architectures. It
> > > > > > will have to be added for other architectures which need to enable
> > > > > > capsule authentication support.
> > > > > >
> > > > > > 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 V6:
> > > > > > * Populate the CONFIG_EFI_CAPSULE_ESL_FILE symbol for sandbox and
> > > > > >   sandbox_flattree which enable capsule authentication.
> > > > > >
> > > > > > Note:
> > > > > > Simon Glass had asked me to rid of the CONFIG_EFI_HAVE_CAPSULE_SUPPORT
> > > > > > ifdef used in the sandbox' u-boot.dtsi file. However, that results in
> > > > > > the sandbox_vpl test failing in CI. Hence that check has been kept.
> > > > > >
> > > > > >
> > > > > >  arch/arm/dts/u-boot.dtsi           | 14 ++++++++++++++
> > > > > >  arch/sandbox/dts/u-boot.dtsi       | 17 +++++++++++++++++
> > > >
> > > > We've already had some go-round as to why this basically identical file
> > > > can't be in like lib/efi_loader/ or something, yes?
> > >
> > > Yes, these need to be under the arch/$(ARCH)/dts/ directory for the
> > > dtc to include them as part of the platform's dts.
> >
> > That logic is just in scripts/ somewhere and can be extended.  We can
> > add flags to specific files when needed.
> >
> > > > > >  configs/sandbox_defconfig          |  1 +
> > > > > >  configs/sandbox_flattree_defconfig |  1 +
> > > > > >  lib/efi_loader/Kconfig             |  9 +++++++++
> > > > > >  5 files changed, 42 insertions(+)
> > > > > >  create mode 100644 arch/arm/dts/u-boot.dtsi
> > > > > >  create mode 100644 arch/sandbox/dts/u-boot.dtsi
> > > > > >
> > > > > > diff --git a/arch/arm/dts/u-boot.dtsi b/arch/arm/dts/u-boot.dtsi
> > > > > > new file mode 100644
> > > > > > index 0000000000..4f31da4521
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm/dts/u-boot.dtsi
> > > > > > @@ -0,0 +1,14 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/**
> > > > > > + * Devicetree file with miscellaneous nodes that will be included
> > > > > > + * at build time into the DTB. Currently being used for including
> > > > > > + * capsule related information.
> > > > > > + */
> > > > > > +
> > > > > > +#ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> > > > > > +/ {
> > > > > > +       signature {
> > > > > > +               capsule-key = /incbin/(CONFIG_EFI_CAPSULE_ESL_FILE);
> > > > > > +       };
> > > > > > +};
> > > > > > +#endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
> > > > > > diff --git a/arch/sandbox/dts/u-boot.dtsi b/arch/sandbox/dts/u-boot.dtsi
> > > > > > new file mode 100644
> > > > > > index 0000000000..60bd004937
> > > > > > --- /dev/null
> > > > > > +++ b/arch/sandbox/dts/u-boot.dtsi
> > > > > > @@ -0,0 +1,17 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + * Devicetree file with miscellaneous nodes that will be included
> > > > > > + * at build time into the DTB. Currently being used for including
> > > > > > + * capsule related information.
> > > > > > + *
> > > > > > + */
> > > > > > +
> > > > > > +#ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT
> > > > > > +/ {
> > > > > > +#ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> > > > > > +       signature {
> > > > > > +               capsule-key = /incbin/(CONFIG_EFI_CAPSULE_ESL_FILE);
> > > > > > +       };
> > > > > > +#endif
> > > > > > +};
> > > > > > +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
> > > >
> > > > And why are these two different?
> > >
> > > For the sandbox's u-boot.dtsi, we need CONFIG_EFI_CAPSULE_AUTHENTICATE
> > > so that the ESL file is looked for only when capsule authentication is
> > > enabled. The outer CONFIG_EFI_HAVE_CAPSULE_SUPPORT is to not include
> > > stuff from this file unless capsule support is enabled. Simon had
> > > asked me to rid of the outer ifdef, but the sandbox_vpl test fails
> > > without it.
> >
> > Sounds like this needs more re-working all around then, as we should
> > only have one of these fragments, and it probably shouldn't be included
> > when not needed.
>
> Another option is to include the actual file (or even the data!) in
> the capsule-key property for sandbox, rather than the CONFIG.

Will this not mean that the dtsi file will have to be included
explicitly for all the files which want to include the public key? I
think it will be easier if the dtsi file can get included
automatically, similar to u-boot.dtsi. I will check if we can put a
dtsi file under lib/efi_loader/ and get that included automatically
with capsule auth enabled, similar to how u-boot.dtsi gets included.

-sughosh