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

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

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

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

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

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

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

Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu sughosh.ganu@linaro.org wrote:

Add a bintool for generating EFI capsules. This calls the mkeficapsule tool which generates the capsules.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6:

• Split the changes for mkeficapsule btool into a separate patch, as suggested by Simon Glass.
• Use the word commandline consistently, as suggested by Simon Glass.

tools/binman/btool/mkeficapsule.py | 101 +++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 tools/binman/btool/mkeficapsule.py

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/tools/binman/btool/mkeficapsule.py b/tools/binman/btool/mkeficapsule.py new file mode 100644 index 0000000000..61179747ff --- /dev/null +++ b/tools/binman/btool/mkeficapsule.py @@ -0,0 +1,101 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright 2023 Linaro Limited +# +"""Bintool implementation for mkeficapsule tool

+mkeficapsule is a tool used for generating EFI capsules.

+The following are the commandline options to be provided +to the tool +Usage: mkeficapsule [options] <image blob> <output file> +Options:

•   -g, --guid <guid string>    guid for image blob type
  

•   -i, --index <index>         update image index
  

•   -I, --instance <instance>   update hardware instance
  

•   -v, --fw-version <version>  firmware version
  

•   -p, --private-key <privkey file>  private key file
  

•   -c, --certificate <cert file>     signer's certificate file
  

•   -m, --monotonic-count <count>     monotonic count
  

•   -d, --dump_sig              dump signature (*.p7)
  

•   -A, --fw-accept  firmware accept capsule, requires GUID, no image blob
  

•   -R, --fw-revert  firmware revert capsule, takes no GUID, no image blob
  

•   -o, --capoemflag Capsule OEM Flag, an integer between 0x0000 and 0xffff
  

•   -h, --help                  print a help message
  

+"""

+from binman import bintool

+class Bintoolmkeficapsule(bintool.Bintool):

• """Handles the 'mkeficapsule' tool
• This bintool is used for generating the EFI capsules. The
• capsule generation parameters can either be specified through
• commandline, or through a config file.
• """
• def __init__(self, name):

•    super().__init__(name, 'mkeficapsule tool for generating capsules')
  

• def generate_capsule(self, image_index, image_guid, hardware_instance,

•                     payload, output_fname, priv_key, pub_key,
  

•                     monotonic_count=0, version=0, oemflags=0):
  

•    """Generate a capsule through commandline-provided parameters
  

•    Args:
  

•        image_index (int): Unique number for identifying payload image
  

•        image_guid (str): GUID used for identifying the image
  

I wonder what we can do about this, so that we don't have to speak in GUIDs? Is there a registry somewhere of what all these things are? It would be nice if you could provide a string like 'u-boot-sandbox' and the capsule tool would know what that means.

•        hardware_instance (int): Optional unique hardware instance of
  

•        a device in the system. 0 if not being used
  

•        payload (str): Path to the input payload image
  

•        output_fname (str): Path to the output capsule file
  

•        priv_key (str): Path to the private key
  

•        pub_key(str): Path to the public key
  

•        monotonic_count (int): Count used when signing an image
  

•        version (int): Image version (Optional)
  

•        oemflags (int): Optional 16 bit OEM flags
  

•    Returns:
  

•        str: Tool output
  

•    """
  

•    args = [
  

•        f'--index={image_index}',
  

•        f'--guid={image_guid}',
  

•        f'--instance={hardware_instance}'
  

•    ]
  

•    if version:
  

•        args += [f'--fw-version={version}']
  

•    if oemflags:
  

•        args += [f'--capoemflag={oemflags}']
  

•    if priv_key and pub_key:
  

•        args += [
  

•            f'--monotonic-count={monotonic_count}',
  

•            f'--private-key={priv_key}',
  

•            f'--certificate={pub_key}'
  

•        ]
  

It almost seems worth adding two methods in this class, one to build with keys and one to not. Anyway, we can leave it for now.

•    args += [
  

•        payload,
  

•        output_fname
  

•    ]
  

•    return self.run_cmd(*args)
  

• def fetch(self, method):

•    """Fetch handler for mkeficapsule
  

•    This builds the tool from source
  

•    Returns:
  

•        tuple:
  

•            str: Filename of fetched file to copy to a suitable directory
  

•            str: Name of temp directory to remove, or None
  

•    """
  

•    if method != bintool.FETCH_BUILD:
  

•        return None
  

•    cmd = ['tools-only_defconfig', 'tools']
  

•    result = self.build_from_git(
  

•        'https://source.denx.de/u-boot/u-boot.git',
  

•        cmd,
  

•        'tools/mkeficapsule')
  

•    return result
  

-- 2.34.1

Regards, Simon

Hi Sughosh,

On Sat, 5 Aug 2023 at 09:03, Simon Glass sjg@chromium.org wrote:

Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

Add support in binman for generating EFI capsules. The capsule parameters can be specified through the capsule binman entry. Also add test cases in binman for testing capsule generation.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6:

• Add macros for the GUID strings in sandbox_efi_capsule.h
• Highlight that the private and public keys are mandatory for capsule signing.
• Add a URL link to the UEFI spec, as used in the rst files.
• Use local vars for private and public keys in BuildSectionData()
• Use local vars for input payload and capsule filenames in BuildSectionData().
• Drop the ProcessContents() and SetImagePos() as the superclass functions suffice.
• Use GUID macro names in the capsule test dts files.
• Rename efi_capsule_payload.bin to capsule_input.bin.

include/sandbox_efi_capsule.h | 14 ++

Please move this file to a later patch - see below.

Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

...

tools/binman/entries.rst | 62 ++++++++ tools/binman/etype/efi_capsule.py | 143 ++++++++++++++++++ tools/binman/ftest.py | 121 +++++++++++++++ tools/binman/test/307_capsule.dts | 23 +++ tools/binman/test/308_capsule_signed.dts | 25 +++ tools/binman/test/309_capsule_version.dts | 24 +++ tools/binman/test/310_capsule_signed_ver.dts | 26 ++++ tools/binman/test/311_capsule_oemflags.dts | 24 +++ tools/binman/test/312_capsule_missing_key.dts | 24 +++ .../binman/test/313_capsule_missing_index.dts | 22 +++ .../binman/test/314_capsule_missing_guid.dts | 19 +++ .../test/315_capsule_missing_payload.dts | 19 +++ 13 files changed, 546 insertions(+) create mode 100644 include/sandbox_efi_capsule.h create mode 100644 tools/binman/etype/efi_capsule.py create mode 100644 tools/binman/test/307_capsule.dts create mode 100644 tools/binman/test/308_capsule_signed.dts create mode 100644 tools/binman/test/309_capsule_version.dts create mode 100644 tools/binman/test/310_capsule_signed_ver.dts create mode 100644 tools/binman/test/311_capsule_oemflags.dts create mode 100644 tools/binman/test/312_capsule_missing_key.dts create mode 100644 tools/binman/test/313_capsule_missing_index.dts create mode 100644 tools/binman/test/314_capsule_missing_guid.dts create mode 100644 tools/binman/test/315_capsule_missing_payload.dts

[..]

...

diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts new file mode 100644 index 0000000000..86552ff83f --- /dev/null +++ b/tools/binman/test/307_capsule.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+

+/dts-v1/;

+#include <sandbox_efi_capsule.h>

We can't include sandbox files in binman! I Does it actually matter what the GUID value is? Can they all be the same? For now I think it is best to have

#define BINMAN_TEST_GUID "xxx"

repeated at the top of each .dts file. Hopefully at some point we can figure out a sensible way to provide a decoder ring for this mess, so we can use actually names in the binman definition instead of GUIDs.

Oh, having thought about this a bit more, there is a much better way.

In the entry type, allow the image_guid to be a string, like 'binman-test' or 'sandbox-test'. Then binman can have a conversion function to figure out the GUID to pass to the tool, e.g. in efi_capsule.py:

TYPE_TO_GUID = { 'binman-test': '09123987987f98712', 'sandbox-test':, '98123987123123987123987', }

def get_guid(type_str): 'Get the GUID to use for a particular purpose""" return TYPE_TO_GUID.get(type_str, type_str)

Then you can use these same strings in the sandbox tests, without needing to include anything.

...

+/ {

•   #address-cells = <1>;
  

•   #size-cells = <1>;
  

•   binman {
  

•           efi-capsule {
  

•                   image-index = <0x1>;
  

•                   /* Image GUID for testing capsule update */
  

•                   image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
  

•                   hardware-instance = <0x0>;
  

•                   blob {
  

•                           filename = "capsule_input.bin";
  

•                   };
  

•           };
  

•   };
  

+};

Regards, Simon

hi Simon,

On Sat, 5 Aug 2023 at 22:50, Simon Glass sjg@chromium.org wrote:

Hi Sughosh,

On Sat, 5 Aug 2023 at 09:03, Simon Glass sjg@chromium.org wrote:

...

Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

Add support in binman for generating EFI capsules. The capsule parameters can be specified through the capsule binman entry. Also add test cases in binman for testing capsule generation.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6:

• Add macros for the GUID strings in sandbox_efi_capsule.h
• Highlight that the private and public keys are mandatory for capsule signing.
• Add a URL link to the UEFI spec, as used in the rst files.
• Use local vars for private and public keys in BuildSectionData()
• Use local vars for input payload and capsule filenames in BuildSectionData().
• Drop the ProcessContents() and SetImagePos() as the superclass functions suffice.
• Use GUID macro names in the capsule test dts files.
• Rename efi_capsule_payload.bin to capsule_input.bin.

include/sandbox_efi_capsule.h | 14 ++

Please move this file to a later patch - see below.

Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

...

tools/binman/entries.rst | 62 ++++++++ tools/binman/etype/efi_capsule.py | 143 ++++++++++++++++++ tools/binman/ftest.py | 121 +++++++++++++++ tools/binman/test/307_capsule.dts | 23 +++ tools/binman/test/308_capsule_signed.dts | 25 +++ tools/binman/test/309_capsule_version.dts | 24 +++ tools/binman/test/310_capsule_signed_ver.dts | 26 ++++ tools/binman/test/311_capsule_oemflags.dts | 24 +++ tools/binman/test/312_capsule_missing_key.dts | 24 +++ .../binman/test/313_capsule_missing_index.dts | 22 +++ .../binman/test/314_capsule_missing_guid.dts | 19 +++ .../test/315_capsule_missing_payload.dts | 19 +++ 13 files changed, 546 insertions(+) create mode 100644 include/sandbox_efi_capsule.h create mode 100644 tools/binman/etype/efi_capsule.py create mode 100644 tools/binman/test/307_capsule.dts create mode 100644 tools/binman/test/308_capsule_signed.dts create mode 100644 tools/binman/test/309_capsule_version.dts create mode 100644 tools/binman/test/310_capsule_signed_ver.dts create mode 100644 tools/binman/test/311_capsule_oemflags.dts create mode 100644 tools/binman/test/312_capsule_missing_key.dts create mode 100644 tools/binman/test/313_capsule_missing_index.dts create mode 100644 tools/binman/test/314_capsule_missing_guid.dts create mode 100644 tools/binman/test/315_capsule_missing_payload.dts

[..]

...

...

diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts new file mode 100644 index 0000000000..86552ff83f --- /dev/null +++ b/tools/binman/test/307_capsule.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+

+/dts-v1/;

+#include <sandbox_efi_capsule.h>

We can't include sandbox files in binman! I Does it actually matter what the GUID value is? Can they all be the same? For now I think it is best to have

#define BINMAN_TEST_GUID "xxx"

repeated at the top of each .dts file. Hopefully at some point we can figure out a sensible way to provide a decoder ring for this mess, so we can use actually names in the binman definition instead of GUIDs.

Oh, having thought about this a bit more, there is a much better way.

In the entry type, allow the image_guid to be a string, like 'binman-test' or 'sandbox-test'. Then binman can have a conversion function to figure out the GUID to pass to the tool, e.g. in efi_capsule.py:

TYPE_TO_GUID = { 'binman-test': '09123987987f98712', 'sandbox-test':, '98123987123123987123987', }

def get_guid(type_str): 'Get the GUID to use for a particular purpose""" return TYPE_TO_GUID.get(type_str, type_str)

Then you can use these same strings in the sandbox tests, without needing to include anything.

While we can use this method for both sandbox and binman tests, I would prefer using the actual GUID strings for the sandbox tests. This will serve as an illustrative example to the user for how to pass the image GUID value for generating the capsule. For the binman tests, I can use this logic to avoid including the header file.

-sughosh

...

...

+/ {

•   #address-cells = <1>;
  

•   #size-cells = <1>;
  

•   binman {
  

•           efi-capsule {
  

•                   image-index = <0x1>;
  

•                   /* Image GUID for testing capsule update */
  

•                   image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
  

•                   hardware-instance = <0x0>;
  

•                   blob {
  

•                           filename = "capsule_input.bin";
  

•                   };
  

•           };
  

•   };
  

+};

Regards, Simon

hi Simon,

On Mon, 7 Aug 2023 at 00:16, Simon Glass sjg@chromium.org wrote:

Hi Sughosh,

On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

hi Simon,

On Sat, 5 Aug 2023 at 22:50, Simon Glass sjg@chromium.org wrote:

...

Hi Sughosh,

On Sat, 5 Aug 2023 at 09:03, Simon Glass sjg@chromium.org wrote:

...

Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

Add support in binman for generating EFI capsules. The capsule parameters can be specified through the capsule binman entry. Also add test cases in binman for testing capsule generation.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6:

• Add macros for the GUID strings in sandbox_efi_capsule.h
• Highlight that the private and public keys are mandatory for capsule signing.
• Add a URL link to the UEFI spec, as used in the rst files.
• Use local vars for private and public keys in BuildSectionData()
• Use local vars for input payload and capsule filenames in BuildSectionData().
• Drop the ProcessContents() and SetImagePos() as the superclass functions suffice.
• Use GUID macro names in the capsule test dts files.
• Rename efi_capsule_payload.bin to capsule_input.bin.

include/sandbox_efi_capsule.h | 14 ++

Please move this file to a later patch - see below.

Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

...

tools/binman/entries.rst | 62 ++++++++ tools/binman/etype/efi_capsule.py | 143 ++++++++++++++++++ tools/binman/ftest.py | 121 +++++++++++++++ tools/binman/test/307_capsule.dts | 23 +++ tools/binman/test/308_capsule_signed.dts | 25 +++ tools/binman/test/309_capsule_version.dts | 24 +++ tools/binman/test/310_capsule_signed_ver.dts | 26 ++++ tools/binman/test/311_capsule_oemflags.dts | 24 +++ tools/binman/test/312_capsule_missing_key.dts | 24 +++ .../binman/test/313_capsule_missing_index.dts | 22 +++ .../binman/test/314_capsule_missing_guid.dts | 19 +++ .../test/315_capsule_missing_payload.dts | 19 +++ 13 files changed, 546 insertions(+) create mode 100644 include/sandbox_efi_capsule.h create mode 100644 tools/binman/etype/efi_capsule.py create mode 100644 tools/binman/test/307_capsule.dts create mode 100644 tools/binman/test/308_capsule_signed.dts create mode 100644 tools/binman/test/309_capsule_version.dts create mode 100644 tools/binman/test/310_capsule_signed_ver.dts create mode 100644 tools/binman/test/311_capsule_oemflags.dts create mode 100644 tools/binman/test/312_capsule_missing_key.dts create mode 100644 tools/binman/test/313_capsule_missing_index.dts create mode 100644 tools/binman/test/314_capsule_missing_guid.dts create mode 100644 tools/binman/test/315_capsule_missing_payload.dts

[..]

...

...

diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts new file mode 100644 index 0000000000..86552ff83f --- /dev/null +++ b/tools/binman/test/307_capsule.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+

+/dts-v1/;

+#include <sandbox_efi_capsule.h>

We can't include sandbox files in binman! I Does it actually matter what the GUID value is? Can they all be the same? For now I think it is best to have

#define BINMAN_TEST_GUID "xxx"

repeated at the top of each .dts file. Hopefully at some point we can figure out a sensible way to provide a decoder ring for this mess, so we can use actually names in the binman definition instead of GUIDs.

Oh, having thought about this a bit more, there is a much better way.

In the entry type, allow the image_guid to be a string, like 'binman-test' or 'sandbox-test'. Then binman can have a conversion function to figure out the GUID to pass to the tool, e.g. in efi_capsule.py:

TYPE_TO_GUID = { 'binman-test': '09123987987f98712', 'sandbox-test':, '98123987123123987123987', }

def get_guid(type_str): 'Get the GUID to use for a particular purpose""" return TYPE_TO_GUID.get(type_str, type_str)

Then you can use these same strings in the sandbox tests, without needing to include anything.

While we can use this method for both sandbox and binman tests, I would prefer using the actual GUID strings for the sandbox tests. This will serve as an illustrative example to the user for how to pass the image GUID value for generating the capsule. For the binman tests, I can use this logic to avoid including the header file.

So long as it is easy, that's OK. But how does the user know which GUID to use?

I think you got me wrong here. What I meant was that passing the GUID value in the capsule entry like how it is currently done in the u-boot.dtsi file would give the user an idea of how to specify the GUID value for an image. If we use the string that you suggest above for both binman tests as well as sandbox, the user might get confused as to how the GUID value is to be passed in the normal scenario. I want the sandbox capsule entries to also be kind of illustrative of how a capsule entry would look like.

-sughosh

Regards, Simon

hi Simon,

On Mon, 7 Aug 2023 at 07:04, Simon Glass sjg@chromium.org wrote:

Hi Sughosh,

On Sun, 6 Aug 2023 at 13:27, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

hi Simon,

On Mon, 7 Aug 2023 at 00:16, Simon Glass sjg@chromium.org wrote:

...

Hi Sughosh,

On Sun, 6 Aug 2023 at 09:35, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

hi Simon,

On Sat, 5 Aug 2023 at 22:50, Simon Glass sjg@chromium.org wrote:

...

Hi Sughosh,

On Sat, 5 Aug 2023 at 09:03, Simon Glass sjg@chromium.org wrote:

...

Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu sughosh.ganu@linaro.org wrote: > > Add support in binman for generating EFI capsules. The capsule > parameters can be specified through the capsule binman entry. Also add > test cases in binman for testing capsule generation. > > Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org > --- > Changes since V6: > * Add macros for the GUID strings in sandbox_efi_capsule.h > * Highlight that the private and public keys are mandatory for capsule > signing. > * Add a URL link to the UEFI spec, as used in the rst files. > * Use local vars for private and public keys in BuildSectionData() > * Use local vars for input payload and capsule filenames in > BuildSectionData(). > * Drop the ProcessContents() and SetImagePos() as the superclass > functions suffice. > * Use GUID macro names in the capsule test dts files. > * Rename efi_capsule_payload.bin to capsule_input.bin. > > > include/sandbox_efi_capsule.h | 14 ++

Please move this file to a later patch - see below.

Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

> tools/binman/entries.rst | 62 ++++++++ > tools/binman/etype/efi_capsule.py | 143 ++++++++++++++++++ > tools/binman/ftest.py | 121 +++++++++++++++ > tools/binman/test/307_capsule.dts | 23 +++ > tools/binman/test/308_capsule_signed.dts | 25 +++ > tools/binman/test/309_capsule_version.dts | 24 +++ > tools/binman/test/310_capsule_signed_ver.dts | 26 ++++ > tools/binman/test/311_capsule_oemflags.dts | 24 +++ > tools/binman/test/312_capsule_missing_key.dts | 24 +++ > .../binman/test/313_capsule_missing_index.dts | 22 +++ > .../binman/test/314_capsule_missing_guid.dts | 19 +++ > .../test/315_capsule_missing_payload.dts | 19 +++ > 13 files changed, 546 insertions(+) > create mode 100644 include/sandbox_efi_capsule.h > create mode 100644 tools/binman/etype/efi_capsule.py > create mode 100644 tools/binman/test/307_capsule.dts > create mode 100644 tools/binman/test/308_capsule_signed.dts > create mode 100644 tools/binman/test/309_capsule_version.dts > create mode 100644 tools/binman/test/310_capsule_signed_ver.dts > create mode 100644 tools/binman/test/311_capsule_oemflags.dts > create mode 100644 tools/binman/test/312_capsule_missing_key.dts > create mode 100644 tools/binman/test/313_capsule_missing_index.dts > create mode 100644 tools/binman/test/314_capsule_missing_guid.dts > create mode 100644 tools/binman/test/315_capsule_missing_payload.dts >

[..]

...

> diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts > new file mode 100644 > index 0000000000..86552ff83f > --- /dev/null > +++ b/tools/binman/test/307_capsule.dts > @@ -0,0 +1,23 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/dts-v1/; > + > +#include <sandbox_efi_capsule.h>

We can't include sandbox files in binman! I Does it actually matter what the GUID value is? Can they all be the same? For now I think it is best to have

#define BINMAN_TEST_GUID "xxx"

repeated at the top of each .dts file. Hopefully at some point we can figure out a sensible way to provide a decoder ring for this mess, so we can use actually names in the binman definition instead of GUIDs.

Oh, having thought about this a bit more, there is a much better way.

In the entry type, allow the image_guid to be a string, like 'binman-test' or 'sandbox-test'. Then binman can have a conversion function to figure out the GUID to pass to the tool, e.g. in efi_capsule.py:

TYPE_TO_GUID = { 'binman-test': '09123987987f98712', 'sandbox-test':, '98123987123123987123987', }

def get_guid(type_str): 'Get the GUID to use for a particular purpose""" return TYPE_TO_GUID.get(type_str, type_str)

Then you can use these same strings in the sandbox tests, without needing to include anything.

While we can use this method for both sandbox and binman tests, I would prefer using the actual GUID strings for the sandbox tests. This will serve as an illustrative example to the user for how to pass the image GUID value for generating the capsule. For the binman tests, I can use this logic to avoid including the header file.

So long as it is easy, that's OK. But how does the user know which GUID to use?

I think you got me wrong here. What I meant was that passing the GUID value in the capsule entry like how it is currently done in the u-boot.dtsi file would give the user an idea of how to specify the GUID value for an image. If we use the string that you suggest above for both binman tests as well as sandbox, the user might get confused as to how the GUID value is to be passed in the normal scenario. I want the sandbox capsule entries to also be kind of illustrative of how a capsule entry would look like.

OK I see.

Of course, I would like to see a decoder ring for GUIDs somewhere. Perhaps Binman is a good place for that, or a file it can read?

I'd suggest we start with your above solution for the binman tests. If we get consensus amongst the folks who work on EFI to have such a GUID decoder file, it can be worked upon later. I think these are things which are part of a larger point that you have about specifying GUIDs in a particular way, and that should be tackled separately, and not through this series, especially at this stage in the series. My 2 cents.

-sughosh

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:

...

Add support in binman for generating EFI capsules. The capsule parameters can be specified through the capsule binman entry. Also add test cases in binman for testing capsule generation.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6:

• Add macros for the GUID strings in sandbox_efi_capsule.h
• Highlight that the private and public keys are mandatory for capsule signing.
• Add a URL link to the UEFI spec, as used in the rst files.
• Use local vars for private and public keys in BuildSectionData()
• Use local vars for input payload and capsule filenames in BuildSectionData().
• Drop the ProcessContents() and SetImagePos() as the superclass functions suffice.
• Use GUID macro names in the capsule test dts files.
• Rename efi_capsule_payload.bin to capsule_input.bin.

include/sandbox_efi_capsule.h | 14 ++

Please move this file to a later patch - see below.

The idea was to also be able to run the binman capsule tests once this patch was applied. If we are to move this to a separate patch, it should be the one before this patch. But I guess based on your other reply, this might not be needed after all.

Could we have a single header file with all the GUIDs, i.e. sandbox, ARM, etc.

Umm, I am not too sure. Maybe we can take a call at a later point if there are too many files that start cropping up.

...

tools/binman/entries.rst | 62 ++++++++ tools/binman/etype/efi_capsule.py | 143 ++++++++++++++++++ tools/binman/ftest.py | 121 +++++++++++++++ tools/binman/test/307_capsule.dts | 23 +++ tools/binman/test/308_capsule_signed.dts | 25 +++ tools/binman/test/309_capsule_version.dts | 24 +++ tools/binman/test/310_capsule_signed_ver.dts | 26 ++++ tools/binman/test/311_capsule_oemflags.dts | 24 +++ tools/binman/test/312_capsule_missing_key.dts | 24 +++ .../binman/test/313_capsule_missing_index.dts | 22 +++ .../binman/test/314_capsule_missing_guid.dts | 19 +++ .../test/315_capsule_missing_payload.dts | 19 +++ 13 files changed, 546 insertions(+) create mode 100644 include/sandbox_efi_capsule.h create mode 100644 tools/binman/etype/efi_capsule.py create mode 100644 tools/binman/test/307_capsule.dts create mode 100644 tools/binman/test/308_capsule_signed.dts create mode 100644 tools/binman/test/309_capsule_version.dts create mode 100644 tools/binman/test/310_capsule_signed_ver.dts create mode 100644 tools/binman/test/311_capsule_oemflags.dts create mode 100644 tools/binman/test/312_capsule_missing_key.dts create mode 100644 tools/binman/test/313_capsule_missing_index.dts create mode 100644 tools/binman/test/314_capsule_missing_guid.dts create mode 100644 tools/binman/test/315_capsule_missing_payload.dts

diff --git a/include/sandbox_efi_capsule.h b/include/sandbox_efi_capsule.h new file mode 100644 index 0000000000..da71b87a18 --- /dev/null +++ b/include/sandbox_efi_capsule.h @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2023, Linaro Limited
• */

+#if !defined(_SANDBOX_EFI_CAPSULE_H_) +#define _SANDBOX_EFI_CAPSULE_H_

+#define SANDBOX_UBOOT_IMAGE_GUID "09d7cf52-0720-4710-91d1-08469b7fe9c8" +#define SANDBOX_UBOOT_ENV_IMAGE_GUID "5a7021f5-fef2-48b4-aaba-832e777418c0" +#define SANDBOX_FIT_IMAGE_GUID "3673b45d-6a7c-46f3-9e60-adabb03f7937" +#define SANDBOX_INCORRECT_GUID "058b7d83-50d5-4c47-a195-60d86ad341c4"

These should not be needed in binman tests.

...

+#endif /* _SANDBOX_EFI_CAPSULE_H_ */ diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index f2376932be..fc094b9c08 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -468,6 +468,68 @@ updating the EC on startup via software sync.

+.. _etype_efi_capsule:

+Entry: capsule: Entry for generating EFI Capsule files +------------------------------------------------------

+The parameters needed for generation of the capsules can +be provided as properties in the entry.

+Properties / Entry arguments:

• • image-index: Unique number for identifying corresponding

•  payload image. Number between 1 and descriptor count, i.e.
  

•  the total number of firmware images that can be updated.
  

• • image-type-id: Image GUID which will be used for identifying the

•  updatable image on the board.
  

• • hardware-instance: Optional number for identifying unique

•  hardware instance of a device in the system. Default value of 0
  

•  for images where value is not to be used.
  

• • fw-version: Optional value of image version that can be put on

•  the capsule through the Firmware Management Protocol(FMP) header.
  

• • monotonic-count: Count used when signing an image.
• • private-key: Path to PEM formatted .key private key file. This property

•  is mandatory for generating signed capsules.
  

• • public-key-cert: Path to PEM formatted .crt public key certificate

•  file. This property is mandatory for generating signed capsules.
  

• • oem-flags - Optional OEM flags to be passed through capsule header.
• Since this is a subclass of Entry_section, all properties of the parent
• class also apply here.

+For more details on the description of the capsule format, and the capsule +update functionality, refer Section 8.5 and Chapter 23 in the `UEFI +specification`_.

+The capsule parameters like image index and image GUID are passed as +properties in the entry. The payload to be used in the capsule is to be +provided as a subnode of the capsule entry.

+A typical capsule entry node would then look something like this::

• capsule {

•        type = "efi-capsule";
  

•        image-index = <0x1>;
  

•        /* Image GUID for testing capsule update */
  

•        image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
  

•        hardware-instance = <0x0>;
  

•        private-key = "path/to/the/private/key";
  

•        public-key-cert = "path/to/the/public-key-cert";
  

•        oem-flags = <0x8000>;
  

•        u-boot {
  

•        };
  

• };

+In the above example, the capsule payload is the U-Boot image. The +capsule entry would read the contents of the payload and put them +into the capsule. Any external file can also be specified as the +payload using the blob-ext subnode.

+.. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf

.. _etype_encrypted:

Entry: encrypted: Externally built encrypted binary blob diff --git a/tools/binman/etype/efi_capsule.py b/tools/binman/etype/efi_capsule.py new file mode 100644 index 0000000000..bfdca94e26 --- /dev/null +++ b/tools/binman/etype/efi_capsule.py @@ -0,0 +1,143 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2023 Linaro Limited +# +# Entry-type module for producing a EFI capsule +#

+import os

+from binman.entry import Entry +from binman.etype.section import Entry_section +from dtoc import fdt_util +from u_boot_pylib import tools

+class Entry_efi_capsule(Entry_section):

• """Generate EFI capsules
• The parameters needed for generation of the capsules can
• be provided as properties in the entry.
• Properties / Entry arguments:
• • image-index: Unique number for identifying corresponding

•  payload image. Number between 1 and descriptor count, i.e.
  

•  the total number of firmware images that can be updated.
  

• • image-type-id: Image GUID which will be used for identifying the

•  updatable image on the board.
  

• • hardware-instance: Optional number for identifying unique

•  hardware instance of a device in the system. Default value of 0
  

•  for images where value is not to be used.
  

• • fw-version: Optional value of image version that can be put on

•  the capsule through the Firmware Management Protocol(FMP) header.
  

• • monotonic-count: Count used when signing an image.
• • private-key: Path to PEM formatted .key private key file. This property

•  is mandatory for generating signed capsules.
  

• • public-key-cert: Path to PEM formatted .crt public key certificate

•  file. This property is mandatory for generating signed capsules.
  

• • oem-flags - Optional OEM flags to be passed through capsule header.
• Since this is a subclass of Entry_section, all properties of the parent
• class also apply here.
• For more details on the description of the capsule format, and the capsule
• update functionality, refer Section 8.5 and Chapter 23 in the `UEFI
• specification`_.
• The capsule parameters like image index and image GUID are passed as
• properties in the entry. The payload to be used in the capsule is to be
• provided as a subnode of the capsule entry.
• A typical capsule entry node would then look something like this
• capsule {

•        type = "efi-capsule";
  

•        image-index = <0x1>;
  

•        /* Image GUID for testing capsule update */
  

•        image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
  

•        hardware-instance = <0x0>;
  

•        private-key = "path/to/the/private/key";
  

•        public-key-cert = "path/to/the/public-key-cert";
  

•        oem-flags = <0x8000>;
  

•        u-boot {
  

•        };
  

• };
• In the above example, the capsule payload is the U-Boot image. The
• capsule entry would read the contents of the payload and put them
• into the capsule. Any external file can also be specified as the
• payload using the blob-ext subnode.
• .. _`UEFI specification`: https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf
• """
• def __init__(self, section, etype, node):

•    super().__init__(section, etype, node)
  

•    self.required_props = ['image-index', 'image-type-id']
  

•    self.image_index = 0
  

•    self.image_guid = ''
  

•    self.hardware_instance = 0
  

•    self.monotonic_count = 0
  

•    self.fw_version = 0
  

•    self.oem_flags = 0
  

•    self.private_key = ''
  

•    self.public_key_cert = ''
  

•    self.auth = 0
  

• def ReadNode(self):

•    self.ReadEntries()
  

•    super().ReadNode()
  

I believe those two lines should be swapped.

Again, like my earlier code for ProcessContents() and SetImagePos(), which was taken from mkimage.py as reference, this code is on similar lines to what is in intel_ifwi.py. Both these files are authored by you, so I took this as reference, especially mkimage.py.

...

•    self.image_index = fdt_util.GetInt(self._node, 'image-index')
  

•    self.image_guid = fdt_util.GetString(self._node, 'image-type-id')
  

•    self.fw_version = fdt_util.GetInt(self._node, 'fw-version')
  

•    self.hardware_instance = fdt_util.GetInt(self._node, 'hardware-instance')
  

•    self.monotonic_count = fdt_util.GetInt(self._node, 'monotonic-count')
  

•    self.oem_flags = fdt_util.GetInt(self._node, 'oem-flags')
  

•    self.private_key = fdt_util.GetString(self._node, 'private-key')
  

•    self.public_key_cert = fdt_util.GetString(self._node, 'public-key-cert')
  

•    if ((self.private_key and not self.public_key_cert) or (self.public_key_cert and not self.private_key)):
  

•        self.Raise('Both private key and public key certificate need to be provided')
  

•    elif not (self.private_key and self.public_key_cert):
  

•        self.auth = 0
  

•    else:
  

•        self.auth = 1
  

• def ReadEntries(self):

•    """Read the subnode to get the payload for this capsule"""
  

•    # We can have a single payload per capsule
  

•    if len(self._node.subnodes) == 0:
  

•        self.Raise('The capsule entry expects at least one subnode for payload')
  

Still need to drop this

? Should we not check if the input payload is missing? We cannot call the mkeficapsule tool without an input image(binary).

...

•    for node in self._node.subnodes:
  

•        entry = Entry.Create(self, node)
  

•        entry.ReadNode()
  

•        self._entries[entry.name] = entry
  

I think you can drop this method, since it should be the same as entry_Sectoin ?

Will check, but again, referenced from mkimage.py.

...

• def BuildSectionData(self, required):

•    private_key = ''
  

•    public_key_cert = ''
  

•    if self.auth:
  

•        if not os.path.isabs(self.private_key):
  

•            private_key =  tools.get_input_filename(self.private_key)
  

•        if not os.path.isabs(self.public_key_cert):
  

•            public_key_cert = tools.get_input_filename(self.public_key_cert)
  

•    data, payload, _ = self.collect_contents_to_file(
  

s/_/uniq/ since you need this below

...

•        self._entries.values(), 'capsule_payload')
  

'capsule_in' is better as it is an input to this entry type

...

•    outfile = self._filename if self._filename else self._node.name
  

You need a unique filename so cannot use self._node.name since it is not guaranteed to be unique.

See how mkimage does this, using uniq

Okay

...

•    capsule_fname = tools.get_output_filename(outfile)
  

•    ret = self.mkeficapsule.generate_capsule(self.image_index,
  

•                                              self.image_guid,
  

•                                              self.hardware_instance,
  

•                                              payload,
  

•                                              capsule_fname,
  

•                                              private_key,
  

•                                              public_key_cert,
  

•                                              self.monotonic_count,
  

•                                              self.fw_version,
  

•                                              self.oem_flags)
  

•    if ret is not None:
  

•        os.remove(payload)
  

•        return tools.read_file(capsule_fname)
  

• def AddBintools(self, btools):

•    self.mkeficapsule = self.AddBintool(btools, 'mkeficapsule')
  

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 36428ec343..9bda0157f7 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -48,6 +48,7 @@ U_BOOT_VPL_DATA = b'vpl76543210fedcbazywxyz_' BLOB_DATA = b'89' ME_DATA = b'0abcd' VGA_DATA = b'vga' +EFI_CAPSULE_DATA = b'efi' U_BOOT_DTB_DATA = b'udtb' U_BOOT_SPL_DTB_DATA = b'spldtb' U_BOOT_TPL_DTB_DATA = b'tpldtb' @@ -119,6 +120,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']

TEE_ADDR = 0x5678

+# Firmware Management Protocol(FMP) GUID +FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a' +# Image GUID specified in the DTS +CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'

class TestFunctional(unittest.TestCase): """Functional tests for binman

@@ -215,6 +221,7 @@ class TestFunctional(unittest.TestCase): TestFunctional._MakeInputFile('scp.bin', SCP_DATA) TestFunctional._MakeInputFile('rockchip-tpl.bin', ROCKCHIP_TPL_DATA) TestFunctional._MakeInputFile('ti_unsecure.bin', TI_UNSECURE_DATA)

•    TestFunctional._MakeInputFile('capsule_input.bin', EFI_CAPSULE_DATA)
  
     # Add a few .dtb files for testing
     TestFunctional._MakeInputFile('%s/test-fdt1.dtb' % TEST_FDT_SUBDIR,
  

@@ -7139,5 +7146,119 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self.assertEqual(fdt_util.GetString(key_node, "key-name-hint"), "key")

• def _CheckCapsule(self, data, signed_capsule=False, version_check=False,

•                  capoemflags=False):
  

•    fmp_signature = "4d535331" # 'M', 'S', 'S', '1'
  

•    fmp_size = "10"
  

•    fmp_fw_version = "02"
  

•    oemflag = "0080"
  

•    payload_data = EFI_CAPSULE_DATA
  

•    # Firmware Management Protocol(FMP) GUID - offset(0 - 32)
  

•    self.assertEqual(FW_MGMT_GUID, data.hex()[:32])
  

•    # Image GUID - offset(96 - 128)
  

•    self.assertEqual(CAPSULE_IMAGE_GUID, data.hex()[96:128])
  

As discussed please add a TODO here, so it is clear that you are planning to write a decoder tool like dump_image.

Okay

-sughosh

...

•    if capoemflags:
  

•        # OEM Flags - offset(40 - 44)
  

•        self.assertEqual(oemflag, data.hex()[40:44])
  

•    if signed_capsule and version_check:
  

•        # FMP header signature - offset(4770 - 4778)
  

•        self.assertEqual(fmp_signature, data.hex()[4770:4778])
  

•        # FMP header size - offset(4778 - 4780)
  

•        self.assertEqual(fmp_size, data.hex()[4778:4780])
  

•        # firmware version - offset(4786 - 4788)
  

•        self.assertEqual(fmp_fw_version, data.hex()[4786:4788])
  

•        # payload offset signed capsule(4802 - 4808)
  

•        self.assertEqual(payload_data.hex(), data.hex()[4802:4808])
  

•    elif signed_capsule:
  

•        # payload offset signed capsule(4770 - 4776)
  

•        self.assertEqual(payload_data.hex(), data.hex()[4770:4776])
  

•    elif version_check:
  

•        # FMP header signature - offset(184 - 192)
  

•        self.assertEqual(fmp_signature, data.hex()[184:192])
  

•        # FMP header size - offset(192 - 194)
  

•        self.assertEqual(fmp_size, data.hex()[192:194])
  

•        # firmware version - offset(200 - 202)
  

•        self.assertEqual(fmp_fw_version, data.hex()[200:202])
  

•        # payload offset for non-signed capsule with version header(216 - 222)
  

•        self.assertEqual(payload_data.hex(), data.hex()[216:222])
  

•    else:
  

•        # payload offset for non-signed capsule with no version header(184 - 190)
  

•        self.assertEqual(payload_data.hex(), data.hex()[184:190])
  

• def testCapsuleGen(self):

•    """Test generation of EFI capsule"""
  

•    data = self._DoReadFile('307_capsule.dts')
  

•    self._CheckCapsule(data)
  

• def testSignedCapsuleGen(self):

•    """Test generation of EFI capsule"""
  

•    data = tools.read_file(self.TestFile("key.key"))
  

•    self._MakeInputFile("key.key", data)
  

•    data = tools.read_file(self.TestFile("key.pem"))
  

•    self._MakeInputFile("key.crt", data)
  

•    data = self._DoReadFile('308_capsule_signed.dts')
  

•    self._CheckCapsule(data, signed_capsule=True)
  

• def testCapsuleGenVersionSupport(self):

•    """Test generation of EFI capsule with version support"""
  

•    data = self._DoReadFile('309_capsule_version.dts')
  

•    self._CheckCapsule(data, version_check=True)
  

• def testCapsuleGenSignedVer(self):

•    """Test generation of signed EFI capsule with version information"""
  

•    data = tools.read_file(self.TestFile("key.key"))
  

•    self._MakeInputFile("key.key", data)
  

•    data = tools.read_file(self.TestFile("key.pem"))
  

•    self._MakeInputFile("key.crt", data)
  

•    data = self._DoReadFile('310_capsule_signed_ver.dts')
  

•    self._CheckCapsule(data, signed_capsule=True, version_check=True)
  

• def testCapsuleGenCapOemFlags(self):

•    """Test generation of EFI capsule with OEM Flags set"""
  

•    data = self._DoReadFile('311_capsule_oemflags.dts')
  

•    self._CheckCapsule(data, capoemflags=True)
  

• def testCapsuleGenKeyMissing(self):

•    """Test that binman errors out on missing key"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('312_capsule_missing_key.dts')
  

•    self.assertIn("Both private key and public key certificate need to be provided",
  

•                  str(e.exception))
  

• def testCapsuleGenIndexMissing(self):

•    """Test that binman errors out on missing image index"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('313_capsule_missing_index.dts')
  

•    self.assertIn("entry is missing properties: image-index",
  

•                  str(e.exception))
  

• def testCapsuleGenGuidMissing(self):

•    """Test that binman errors out on missing image GUID"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('314_capsule_missing_guid.dts')
  

•    self.assertIn("entry is missing properties: image-type-id",
  

•                  str(e.exception))
  

• def testCapsuleGenPayloadMissing(self):

•    """Test that binman errors out on missing input(payload)image"""
  

•    with self.assertRaises(ValueError) as e:
  

•        self._DoReadFile('315_capsule_missing_payload.dts')
  

•    self.assertIn("The capsule entry expects at least one subnode for payload",
  

•                  str(e.exception))
  

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/307_capsule.dts b/tools/binman/test/307_capsule.dts new file mode 100644 index 0000000000..86552ff83f --- /dev/null +++ b/tools/binman/test/307_capsule.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+

+/dts-v1/;

+#include <sandbox_efi_capsule.h>

We can't include sandbox files in binman! I Does it actually matter what the GUID value is? Can they all be the same? For now I think it is best to have

#define BINMAN_TEST_GUID "xxx"

repeated at the top of each .dts file. Hopefully at some point we can figure out a sensible way to provide a decoder ring for this mess, so we can use actually names in the binman definition instead of GUIDs.

...

+/ {

•   #address-cells = <1>;
  

•   #size-cells = <1>;
  

•   binman {
  

•           efi-capsule {
  

•                   image-index = <0x1>;
  

•                   /* Image GUID for testing capsule update */
  

•                   image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
  

•                   hardware-instance = <0x0>;
  

•                   blob {
  

•                           filename = "capsule_input.bin";
  

•                   };
  

•           };
  

•   };
  

+};

Regards, Simon

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 files can now be generated as part of u-boot build through binman. Add capsule entry nodes in the u-boot.dtsi for the sandbox architecture for generating the capsules. These capsules are then used for testing the EFI capsule update functionality on the sandbox platforms. Also add binman nodes for generating the input files used for these capsules.

Remove the corresponding logic which was used for generation of these input files which is now superfluous.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6:

• Use macros defined in sandbox_efi_capsule for GUIDs and capsule input filenames.
• Generate the capsule input files through binman text entries.

arch/sandbox/dts/u-boot.dtsi | 363 ++++++++++++++++++ include/sandbox_efi_capsule.h | 11 + test/py/tests/test_efi_capsule/conftest.py | 168 +------- test/py/tests/test_efi_capsule/signature.dts | 10 - .../tests/test_efi_capsule/uboot_bin_env.its | 36 -- 5 files changed, 385 insertions(+), 203 deletions(-) delete mode 100644 test/py/tests/test_efi_capsule/signature.dts delete mode 100644 test/py/tests/test_efi_capsule/uboot_bin_env.its

I think you are still getting confused with using filenames when you don't need to. See below...

No, I don't think so. This is being done on purpose, since I want these files to be created. These are then copied to the efi capsule update tests' data directory, and are then used in testing the feature. Maybe if you want, I can put a comment to that effect.

...

diff --git a/arch/sandbox/dts/u-boot.dtsi b/arch/sandbox/dts/u-boot.dtsi index 60bd004937..f1b16b41c2 100644 --- a/arch/sandbox/dts/u-boot.dtsi +++ b/arch/sandbox/dts/u-boot.dtsi @@ -7,11 +7,374 @@ */

#ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT

+#include <sandbox_efi_capsule.h>

/ { #ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE signature { capsule-key = /incbin/(CONFIG_EFI_CAPSULE_ESL_FILE); }; #endif

•   binman: binman {
  

•           multiple-images;
  

•   };
  

+};

+&binman {

•   input1 {
  

•           filename = UBOOT_BIN_IMAGE_OLD;
  

•           text {
  

•                   text = "u-boot:Old";
  

•           };
  

•   };
  

•   input2 {
  

•           filename = UBOOT_BIN_IMAGE_NEW;
  

•           text {
  

•                   text = "u-boot:New";
  

•           };
  

•   };
  

•   input3 {
  

•           filename = UBOOT_ENV_IMAGE_OLD;
  

•           text {
  

•                   text = "u-boot-env:Old";
  

•           };
  

•   };
  

•   input4 {
  

•           filename = UBOOT_ENV_IMAGE_NEW;
  

•           text {
  

•                   text = "u-boot-env:New";
  

•           };
  

•   };
  

All of those nodes can be removed

...

•   itb {
  

•           filename = UBOOT_FIT_IMAGE;
  

•           fit {
  

•                   description = "Automatic U-Boot environment update";
  

•                   #address-cells = <2>;
  

•                   images {
  

•                           u-boot-bin {
  

•                                   description = "U-Boot binary on SPI Flash";
  

•                                   compression = "none";
  

•                                   type = "firmware";
  

•                                   arch = "sandbox";
  

•                                   load = <0>;
  

•                                   blob {
  

•                                           filename = UBOOT_BIN_IMAGE_NEW;
  

•                                   };
  

instead of this blob node, you should be able to write:

text { text = "u-boot:Old"; };

There is no need to provide filenames in every node.

I know, please check above.

...

•                                   hash-1 {
  

•                                           algo = "sha1";
  

•                                   };
  

•                           };
  

•                           u-boot-env {
  

•                                   description = "U-Boot environment on SPI Flash";
  

•                                   compression = "none";
  

•                                   type = "firmware";
  

•                                   arch = "sandbox";
  

•                                   load = <0>;
  

•                                   blob {
  

•                                           filename = UBOOT_ENV_IMAGE_NEW;
  

•                                   };
  

same here and below

...

•                                   hash-1 {
  

•                                           algo = "sha1";
  

•                                   };
  

•                           };
  

•                   };
  

•           };
  

•   };
  

•   capsule1 {
  

•           filename = "Test01";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x1>;
  

•                   image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
  

•                   blob {
  

•                           filename = UBOOT_BIN_IMAGE_NEW;
  

•                   };
  

again this should be a text node

same below

...

•           };
  

•   };
  

•   capsule2 {
  

•           filename = "Test02";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x2>;
  

•                   image-type-id = SANDBOX_UBOOT_ENV_IMAGE_GUID;
  

•                   blob {
  

•                           filename = UBOOT_ENV_IMAGE_NEW;
  

•                   };
  

•           };
  

•   };
  

•   capsule3 {
  

•           filename = "Test03";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x1>;
  

•                   image-type-id = SANDBOX_INCORRECT_GUID;
  

•                   blob {
  

•                           filename = UBOOT_BIN_IMAGE_NEW;
  

•                   };
  

•           };
  

•   };
  

•   capsule4 {
  

•           filename = "Test04";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x1>;
  

•                   image-type-id = SANDBOX_FIT_IMAGE_GUID;
  

•                   blob {
  

•                           filename =
  

[..]

...

•   capsule19 {
  

•           filename = "Test115";
  

[..]

We really need to talk about these tests. So many cases! Can you not reduce this?

Unfortunately no. These capsules are all needed for the feature testing. Which is why I was asking yesterday if you wanted these generated for normal builds as well, or only for CI runs.

-sughosh

On Sat, Aug 05, 2023 at 09:04:00AM -0600, Simon Glass wrote:

Hi Sughosh,

On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu sughosh.ganu@linaro.org wrote:

...

The EFI capsule files can now be generated as part of u-boot build through binman. Add capsule entry nodes in the u-boot.dtsi for the sandbox architecture for generating the capsules. These capsules are then used for testing the EFI capsule update functionality on the sandbox platforms. Also add binman nodes for generating the input files used for these capsules.

Remove the corresponding logic which was used for generation of these input files which is now superfluous.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V6:

• Use macros defined in sandbox_efi_capsule for GUIDs and capsule input filenames.
• Generate the capsule input files through binman text entries.

arch/sandbox/dts/u-boot.dtsi | 363 ++++++++++++++++++ include/sandbox_efi_capsule.h | 11 + test/py/tests/test_efi_capsule/conftest.py | 168 +------- test/py/tests/test_efi_capsule/signature.dts | 10 - .../tests/test_efi_capsule/uboot_bin_env.its | 36 -- 5 files changed, 385 insertions(+), 203 deletions(-) delete mode 100644 test/py/tests/test_efi_capsule/signature.dts delete mode 100644 test/py/tests/test_efi_capsule/uboot_bin_env.its

I think you are still getting confused with using filenames when you don't need to. See below...

...

diff --git a/arch/sandbox/dts/u-boot.dtsi b/arch/sandbox/dts/u-boot.dtsi index 60bd004937..f1b16b41c2 100644 --- a/arch/sandbox/dts/u-boot.dtsi +++ b/arch/sandbox/dts/u-boot.dtsi @@ -7,11 +7,374 @@ */

#ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT

+#include <sandbox_efi_capsule.h>

/ { #ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE signature { capsule-key = /incbin/(CONFIG_EFI_CAPSULE_ESL_FILE); }; #endif

•   binman: binman {
  

•           multiple-images;
  

•   };
  

+};

+&binman {

•   input1 {
  

•           filename = UBOOT_BIN_IMAGE_OLD;
  

•           text {
  

•                   text = "u-boot:Old";
  

•           };
  

•   };
  

•   input2 {
  

•           filename = UBOOT_BIN_IMAGE_NEW;
  

•           text {
  

•                   text = "u-boot:New";
  

•           };
  

•   };
  

•   input3 {
  

•           filename = UBOOT_ENV_IMAGE_OLD;
  

•           text {
  

•                   text = "u-boot-env:Old";
  

•           };
  

•   };
  

•   input4 {
  

•           filename = UBOOT_ENV_IMAGE_NEW;
  

•           text {
  

•                   text = "u-boot-env:New";
  

•           };
  

•   };
  

All of those nodes can be removed

...

•   itb {
  

•           filename = UBOOT_FIT_IMAGE;
  

•           fit {
  

•                   description = "Automatic U-Boot environment update";
  

•                   #address-cells = <2>;
  

•                   images {
  

•                           u-boot-bin {
  

•                                   description = "U-Boot binary on SPI Flash";
  

•                                   compression = "none";
  

•                                   type = "firmware";
  

•                                   arch = "sandbox";
  

•                                   load = <0>;
  

•                                   blob {
  

•                                           filename = UBOOT_BIN_IMAGE_NEW;
  

•                                   };
  

instead of this blob node, you should be able to write:

text { text = "u-boot:Old"; };

There is no need to provide filenames in every node.

...

•                                   hash-1 {
  

•                                           algo = "sha1";
  

•                                   };
  

•                           };
  

•                           u-boot-env {
  

•                                   description = "U-Boot environment on SPI Flash";
  

•                                   compression = "none";
  

•                                   type = "firmware";
  

•                                   arch = "sandbox";
  

•                                   load = <0>;
  

•                                   blob {
  

•                                           filename = UBOOT_ENV_IMAGE_NEW;
  

•                                   };
  

same here and below

...

•                                   hash-1 {
  

•                                           algo = "sha1";
  

•                                   };
  

•                           };
  

•                   };
  

•           };
  

•   };
  

•   capsule1 {
  

•           filename = "Test01";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x1>;
  

•                   image-type-id = SANDBOX_UBOOT_IMAGE_GUID;
  

•                   blob {
  

•                           filename = UBOOT_BIN_IMAGE_NEW;
  

•                   };
  

again this should be a text node

same below

...

•           };
  

•   };
  

•   capsule2 {
  

•           filename = "Test02";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x2>;
  

•                   image-type-id = SANDBOX_UBOOT_ENV_IMAGE_GUID;
  

•                   blob {
  

•                           filename = UBOOT_ENV_IMAGE_NEW;
  

•                   };
  

•           };
  

•   };
  

•   capsule3 {
  

•           filename = "Test03";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x1>;
  

•                   image-type-id = SANDBOX_INCORRECT_GUID;
  

•                   blob {
  

•                           filename = UBOOT_BIN_IMAGE_NEW;
  

•                   };
  

•           };
  

•   };
  

•   capsule4 {
  

•           filename = "Test04";
  

•           capsule {
  

•                   type = "efi-capsule";
  

•                   image-index = <0x1>;
  

•                   image-type-id = SANDBOX_FIT_IMAGE_GUID;
  

•                   blob {
  

•                           filename =
  

[..]

...

•   capsule19 {
  

•           filename = "Test115";
  

[..]

We really need to talk about these tests. So many cases! Can you not reduce this?