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

[Simon Glass]

Hi Sughosh,

On Sun, 6 Aug 2023 at 13:34, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> 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.

Well I feel that the CONFIG is a bit silly in a way, at least for
sandbox, since we know the file being used. It just makes it harder.

Regards,
Simon