[PATCH v5 03/12] capsule: authenticate: Add capsule public key in platform's dtb

[Sughosh Ganu]

hi Simon,

On Wed, 2 Aug 2023 at 18:22, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 1 Aug 2023 at 11:35, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Wed, 26 Jul 2023 at 19:56, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Wed, 26 Jul 2023 at 02:57, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Wed, 26 Jul 2023 at 04:22, Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Tue, 25 Jul 2023 at 02:58, 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 V4:
> > > > > > * Fix multi line comment format.
> > > > > > * Drop additional blank line.
> > > > > > * Remove the check for CONFIG_EFI_HAVE_CAPSULE_SUPPORT from arm's
> > > > > >   u-boot.dtsi.
> > > > > > * Wrap the help text in the EFI_CAPSULE_ESL_FILE config at 72 chars.
> > > > > >
> > > >
> > > > <snip>
> > > >
> > > > > > 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 */
> > > > >
> > > > > You missed my comment there. You should not need the outer #ifdef, but
> > > > > if you do, please combine them into one #if
> > > >
> > > > I did not miss your comment. The reason I have kept both the ifdefs is
> > > > that we need to include stuff which is needed only when
> > > > CONFIG_EFI_CAPSULE_AUTHENTICATE is enabled, and then the rest of the
> > > > stuff is needed only when CONFIG_EFI_HAVE_CAPSULE_SUPPORT is enabled.
> > > > Not having both the ifdefs would result in build failures. In the
> > > > u-boot.dtsi included for the arm arch, I am using a single ifdef,
> > > > since we are including only the signature node in that file.
> > >
> > > Well having
> > >
> > > / {
> > > };
> > >
> > > is harmless in all cases, I believe. So you should not need the outer one?
> >
> > Sorry, I missed out this comment earlier. So this would not be an
> > empty node but contain the capsule generation nodes. This would result
> > in capsules getting generated for the sandbox_vpl and sandbox_noinst
> > variants which do not enable the capsule functionality.
>
> If that is all it is then I think it is fine to leave them in. Those
> builds won't care anyway.

Fair enough. Will drop the outer ifdef.

-sughosh

>
> Regards,
> Simon