diff mbox series

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

Message ID 20230805113458.1430239-5-sughosh.ganu@linaro.org
State New
Headers show
Series Integrate EFI capsule tasks into u-boot's build flow | expand

Commit Message

Sughosh Ganu Aug. 5, 2023, 11:34 a.m. UTC 
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@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 +++++++++++++++++
 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

Comments

Simon Glass Aug. 5, 2023, 3:03 p.m. UTC | #1 
Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@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@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 +++++++++++++++++
>  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 */
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index b6c4f735f2..779af4abc8 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -341,6 +341,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
>  CONFIG_EFI_CAPSULE_ON_DISK=y
>  CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
>  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"

Can we avoid the path here, and just use e.g. good.esl ?

Perhaps this could be fixed up later, e.g. by adding the board
directory as an include dir when building the DT?

>  CONFIG_EFI_SECURE_BOOT=y
>  CONFIG_TEST_FDTDEC=y
>  CONFIG_UNIT_TEST=y
> diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
> index 8aa295686d..0ca2e4a5ae 100644
> --- a/configs/sandbox_flattree_defconfig
> +++ b/configs/sandbox_flattree_defconfig
> @@ -227,6 +227,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
>  CONFIG_EFI_CAPSULE_ON_DISK=y
>  CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
>  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
>  CONFIG_UNIT_TEST=y
>  CONFIG_UT_TIME=y
>  CONFIG_UT_DM=y
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index a22e47616f..0d559ff3a1 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -235,6 +235,15 @@ 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"
> +       default ""
> +       depends on EFI_CAPSULE_AUTHENTICATE
> +       help
> +         Provides the absolute path to the EFI Signature List file which

It isn't really an absolute path as it doesn't start with /

You really can't/shouldn't use absolute paths in a U-Boot build.

> +         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
> --
> 2.34.1
>

Regards,
Simon
Sughosh Ganu Aug. 5, 2023, 5:54 p.m. UTC | #2 
hi Simon,

On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@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@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 +++++++++++++++++
> >  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 */
> > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > index b6c4f735f2..779af4abc8 100644
> > --- a/configs/sandbox_defconfig
> > +++ b/configs/sandbox_defconfig
> > @@ -341,6 +341,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> >  CONFIG_EFI_CAPSULE_ON_DISK=y
> >  CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> >  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> > +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
>
> Can we avoid the path here, and just use e.g. good.esl ?

Unfortunately no. I believe the way incbin works in dts is that it
looks for the binary to be included in the same directory as the dts.
Which is why I have to point it to the location of the esl relative to
the dts.

>
> Perhaps this could be fixed up later, e.g. by adding the board
> directory as an include dir when building the DT?

Again, this is not how incbin seems to work in dts. I tried putting
the esl in one of the existing include directory locations, but it
does not pick the file from those dirs. It works with the assumption
that the bin file is to be in the same dir as the dts.

>
> >  CONFIG_EFI_SECURE_BOOT=y
> >  CONFIG_TEST_FDTDEC=y
> >  CONFIG_UNIT_TEST=y
> > diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
> > index 8aa295686d..0ca2e4a5ae 100644
> > --- a/configs/sandbox_flattree_defconfig
> > +++ b/configs/sandbox_flattree_defconfig
> > @@ -227,6 +227,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> >  CONFIG_EFI_CAPSULE_ON_DISK=y
> >  CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
> >  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> > +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
> >  CONFIG_UNIT_TEST=y
> >  CONFIG_UT_TIME=y
> >  CONFIG_UT_DM=y
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index a22e47616f..0d559ff3a1 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -235,6 +235,15 @@ 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"
> > +       default ""
> > +       depends on EFI_CAPSULE_AUTHENTICATE
> > +       help
> > +         Provides the absolute path to the EFI Signature List file which
>
> It isn't really an absolute path as it doesn't start with /

Will change

>
> You really can't/shouldn't use absolute paths in a U-Boot build.

Okay

-sughosh

>
> > +         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
> > --
> > 2.34.1
> >
>
> Regards,
> Simon
Simon Glass Aug. 5, 2023, 6:36 p.m. UTC | #3 
Hi Sughosh,

On Sat, 5 Aug 2023 at 11:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@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@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 +++++++++++++++++
> > >  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 */
> > > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > > index b6c4f735f2..779af4abc8 100644
> > > --- a/configs/sandbox_defconfig
> > > +++ b/configs/sandbox_defconfig
> > > @@ -341,6 +341,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > >  CONFIG_EFI_CAPSULE_ON_DISK=y
> > >  CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > >  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> > > +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
> >
> > Can we avoid the path here, and just use e.g. good.esl ?
>
> Unfortunately no. I believe the way incbin works in dts is that it
> looks for the binary to be included in the same directory as the dts.
> Which is why I have to point it to the location of the esl relative to
> the dts.
>
> >
> > Perhaps this could be fixed up later, e.g. by adding the board
> > directory as an include dir when building the DT?
>
> Again, this is not how incbin seems to work in dts. I tried putting
> the esl in one of the existing include directory locations, but it
> does not pick the file from those dirs. It works with the assumption
> that the bin file is to be in the same dir as the dts.

Yes but you can change that. Try adding to the cmd_dtc rule in Makefile.lib:

   -i $(srctree)/board/$(BOARDDIR) \

[..]

Regards,
Simon
Sughosh Ganu Aug. 5, 2023, 6:46 p.m. UTC | #4 
hi Simon,

On Sun, 6 Aug 2023 at 00:06, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 11:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@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@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 +++++++++++++++++
> > > >  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 */
> > > > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > > > index b6c4f735f2..779af4abc8 100644
> > > > --- a/configs/sandbox_defconfig
> > > > +++ b/configs/sandbox_defconfig
> > > > @@ -341,6 +341,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > >  CONFIG_EFI_CAPSULE_ON_DISK=y
> > > >  CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > >  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> > > > +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
> > >
> > > Can we avoid the path here, and just use e.g. good.esl ?
> >
> > Unfortunately no. I believe the way incbin works in dts is that it
> > looks for the binary to be included in the same directory as the dts.
> > Which is why I have to point it to the location of the esl relative to
> > the dts.
> >
> > >
> > > Perhaps this could be fixed up later, e.g. by adding the board
> > > directory as an include dir when building the DT?
> >
> > Again, this is not how incbin seems to work in dts. I tried putting
> > the esl in one of the existing include directory locations, but it
> > does not pick the file from those dirs. It works with the assumption
> > that the bin file is to be in the same dir as the dts.
>
> Yes but you can change that. Try adding to the cmd_dtc rule in Makefile.lib:
>
>    -i $(srctree)/board/$(BOARDDIR) \

We already have the

-I$(srctree)/include

and I had tried putting the esl under the include directory, but it
was not found.

-sughosh


>
> [..]
>
> Regards,
> Simon
Simon Glass Aug. 5, 2023, 7:05 p.m. UTC | #5 
Hi Sughosh,

On Sat, 5 Aug 2023 at 12:47, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 6 Aug 2023 at 00:06, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sat, 5 Aug 2023 at 11:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@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@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 +++++++++++++++++
> > > > >  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 */
> > > > > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > > > > index b6c4f735f2..779af4abc8 100644
> > > > > --- a/configs/sandbox_defconfig
> > > > > +++ b/configs/sandbox_defconfig
> > > > > @@ -341,6 +341,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > > >  CONFIG_EFI_CAPSULE_ON_DISK=y
> > > > >  CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > > >  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> > > > > +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
> > > >
> > > > Can we avoid the path here, and just use e.g. good.esl ?
> > >
> > > Unfortunately no. I believe the way incbin works in dts is that it
> > > looks for the binary to be included in the same directory as the dts.
> > > Which is why I have to point it to the location of the esl relative to
> > > the dts.
> > >
> > > >
> > > > Perhaps this could be fixed up later, e.g. by adding the board
> > > > directory as an include dir when building the DT?
> > >
> > > Again, this is not how incbin seems to work in dts. I tried putting
> > > the esl in one of the existing include directory locations, but it
> > > does not pick the file from those dirs. It works with the assumption
> > > that the bin file is to be in the same dir as the dts.
> >
> > Yes but you can change that. Try adding to the cmd_dtc rule in Makefile.lib:
> >
> >    -i $(srctree)/board/$(BOARDDIR) \
>
> We already have the
>
> -I$(srctree)/include
>
> and I had tried putting the esl under the include directory, but it
> was not found.

I think the board directory is better, though. It isn't really an include.

Regards,
Simon
Sughosh Ganu Aug. 5, 2023, 7:24 p.m. UTC | #6 
hi Simon,

On Sun, 6 Aug 2023 at 00:35, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sat, 5 Aug 2023 at 12:47, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sun, 6 Aug 2023 at 00:06, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sat, 5 Aug 2023 at 11:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Sat, 5 Aug 2023 at 20:34, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.ganu@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@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 +++++++++++++++++
> > > > > >  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 */
> > > > > > diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> > > > > > index b6c4f735f2..779af4abc8 100644
> > > > > > --- a/configs/sandbox_defconfig
> > > > > > +++ b/configs/sandbox_defconfig
> > > > > > @@ -341,6 +341,7 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
> > > > > >  CONFIG_EFI_CAPSULE_ON_DISK=y
> > > > > >  CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > > > > >  CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> > > > > > +CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
> > > > >
> > > > > Can we avoid the path here, and just use e.g. good.esl ?
> > > >
> > > > Unfortunately no. I believe the way incbin works in dts is that it
> > > > looks for the binary to be included in the same directory as the dts.
> > > > Which is why I have to point it to the location of the esl relative to
> > > > the dts.
> > > >
> > > > >
> > > > > Perhaps this could be fixed up later, e.g. by adding the board
> > > > > directory as an include dir when building the DT?
> > > >
> > > > Again, this is not how incbin seems to work in dts. I tried putting
> > > > the esl in one of the existing include directory locations, but it
> > > > does not pick the file from those dirs. It works with the assumption
> > > > that the bin file is to be in the same dir as the dts.
> > >
> > > Yes but you can change that. Try adding to the cmd_dtc rule in Makefile.lib:
> > >
> > >    -i $(srctree)/board/$(BOARDDIR) \
> >
> > We already have the
> >
> > -I$(srctree)/include
> >
> > and I had tried putting the esl under the include directory, but it
> > was not found.
>
> I think the board directory is better, though. It isn't really an include.

What I was trying to say is that putting the esl file under an include
directory does not work, in that the dtc is not able to locate the esl
file from the include directories. The incbin file has to be in a
directory relative to that of the dts file.

I just put the SIGNER.esl under the include/ directory to check if dtc
is able to locate the file from an include directory, and it does not.

-sughosh

>
> Regards,
> Simon
Tom Rini Aug. 5, 2023, 10:12 p.m. UTC | #7 
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@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@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?

> >  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?

> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index a22e47616f..0d559ff3a1 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -235,6 +235,15 @@ 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"
> > +       default ""

No default.

> > +       depends on EFI_CAPSULE_AUTHENTICATE
> > +       help
> > +         Provides the absolute path to the EFI Signature List file which
> 
> It isn't really an absolute path as it doesn't start with /
> 
> You really can't/shouldn't use absolute paths in a U-Boot build.

You can't as it will break reproducible builds (or make them harder than
required).
Sughosh Ganu Aug. 6, 2023, 11:18 a.m. UTC | #8 
On Sun, 6 Aug 2023 at 03:42, Tom Rini <trini@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@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@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.

>
> > >  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.

>
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index a22e47616f..0d559ff3a1 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -235,6 +235,15 @@ 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"
> > > +       default ""
>
> No default.

Okay

>
> > > +       depends on EFI_CAPSULE_AUTHENTICATE
> > > +       help
> > > +         Provides the absolute path to the EFI Signature List file which
> >
> > It isn't really an absolute path as it doesn't start with /
> >
> > You really can't/shouldn't use absolute paths in a U-Boot build.
>
> You can't as it will break reproducible builds (or make them harder than
> required).

Okay

-sughosh
Simon Glass Aug. 6, 2023, 3 p.m. UTC | #9 
Hi Sughosh,

On Sun, 6 Aug 2023 at 05:18, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Sun, 6 Aug 2023 at 03:42, Tom Rini <trini@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@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@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.
>
> >
> > > >  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.

Did you look at why? From what I can see, it is because you are adding
the multiple-images tag in binman but not updating VPL. This patch
seems to fix it:

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/sandbox/dts/sandbox_vpl.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/sandbox/dts/sandbox_vpl.dtsi
b/arch/sandbox/dts/sandbox_vpl.dtsi
index c7dc00a8d2d..83e7657d5db 100644
--- a/arch/sandbox/dts/sandbox_vpl.dtsi
+++ b/arch/sandbox/dts/sandbox_vpl.dtsi
@@ -4,6 +4,11 @@
  */

 &binman {
+ image: image {
+ };
+};
+
+&image {
  u-boot-tpl-elf {
  no-expanded;
  };

--
2.34.1

I think it makes sense to generate the capsule definition for all
sandbox builds so you can drop the #ifdefs. Once the tests are tidied
up, it won't have that much impact...if it annoys us we can drop it
later. The EFI_CAPSULE_ESL_FILE Kconfig needs to be present always (no
'depends on'), default to "", otherwise you need an annoying #ifdef in
the .dtsi file.

Regards,
Simon
Tom Rini Aug. 6, 2023, 5:25 p.m. UTC | #10 
On Sun, Aug 06, 2023 at 04:48:11PM +0530, Sughosh Ganu wrote:
> On Sun, 6 Aug 2023 at 03:42, Tom Rini <trini@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@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@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.
Simon Glass Aug. 6, 2023, 6:46 p.m. UTC | #11 
Hi,

On Sun, 6 Aug 2023 at 11:25, Tom Rini <trini@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@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@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@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.

Regards,
Simon
Sughosh Ganu Aug. 6, 2023, 7:34 p.m. UTC | #12 
hi Simon,

On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Sun, 6 Aug 2023 at 11:25, Tom Rini <trini@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@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@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@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
Simon Glass Aug. 7, 2023, 1:33 a.m. UTC | #13 
Hi Sughosh,

On Sun, 6 Aug 2023 at 13:34, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 7 Aug 2023 at 00:16, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Sun, 6 Aug 2023 at 11:25, Tom Rini <trini@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@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@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@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
diff mbox series

Patch

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 */
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index b6c4f735f2..779af4abc8 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -341,6 +341,7 @@  CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
 CONFIG_EFI_CAPSULE_ON_DISK=y
 CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
 CONFIG_EFI_CAPSULE_AUTHENTICATE=y
+CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
 CONFIG_EFI_SECURE_BOOT=y
 CONFIG_TEST_FDTDEC=y
 CONFIG_UNIT_TEST=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index 8aa295686d..0ca2e4a5ae 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -227,6 +227,7 @@  CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
 CONFIG_EFI_CAPSULE_ON_DISK=y
 CONFIG_EFI_CAPSULE_FIRMWARE_FIT=y
 CONFIG_EFI_CAPSULE_AUTHENTICATE=y
+CONFIG_EFI_CAPSULE_ESL_FILE="../../../board/sandbox/SIGNER.esl"
 CONFIG_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index a22e47616f..0d559ff3a1 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -235,6 +235,15 @@  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"
+	default ""
+	depends on EFI_CAPSULE_AUTHENTICATE
+	help
+	  Provides the absolute 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