[RFC PATCH 0/4] capsule: Embed the public key ESL as part of build

[RFC PATCH 0/4] capsule: Embed the public key ESL as part of build

[Sughosh Ganu]

This is a RFC series which takes a different approach to embedding the
public key EFI Signature List(ESL) needed for capsule authentication
into the platform's DTB.

The earlier approach [1] was using a u-boot.dtsi file to embed the
key. But this approach has a few issues. 1) The path of the incbin file
is not relative to $(srctree), but relative to the directory of the
dts file which is including the dtsi. 2) The u-boot.dtsi file only
gets included in the DTB if there are no other *u-boot.dtsi files
being included. 3) A separate u-boot.dtsi is needed per arch.

To get around these issues, this approach generates a dtsi
file(.capsule_esl.dtsi) with the public key node during build. This
generated dtsi file contains the resolved path to the ESL and is then
included for the DTB generation.

The first patch of the series also cleans up the logic to include the
dtsi files, by collating all the dtsi files to be included into a
single variable.

Since this is a RFC, I have only build tested this on sandbox variants
and not put this through a CI run.

These patches need to be applied on top of the series for generating
the capsules as part of the build [2].

[1] - https://lists.denx.de/pipermail/u-boot/2023-August/526323.html
[2] - https://lore.kernel.org/u-boot/20230812153024.334563-1-sughosh.ganu@linaro.org/T/#m85a50079007acf8943cfe8efcc7d78d23a40db7c


Sughosh Ganu (4):
  scripts/Makefile.lib: Collate all dtsi files for inclusion
  scripts/Makefile.lib: Embed capsule public key in platform's dtb
  sandbox: capsule: Add path to the public key ESL file
  doc: capsule: Document the new mechanism to embed ESL file into dtb

 configs/sandbox_defconfig          |  1 +
 configs/sandbox_flattree_defconfig |  1 +
 doc/develop/uefi/uefi.rst          | 19 +++++--------------
 lib/efi_loader/Kconfig             |  9 +++++++++
 lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
 scripts/Makefile.lib               | 28 ++++++++++++++++++++++++----
 6 files changed, 51 insertions(+), 18 deletions(-)
 create mode 100644 lib/efi_loader/capsule_esl.dtsi.in

-- 
2.34.1

[RFC PATCH 1/4] scripts/Makefile.lib: Collate all dtsi files for inclusion

[Sughosh Ganu]

At the time of building a device-tree file, all the *u-boot.dtsi files
are looked for, in a particular order, and the first file found is
included. Then, the list of files specified in the
CONFIG_DEVICE_TREE_INCLUDES symbol are included.

Combine these files that are to be included into a variable, and then
include all these files in one go.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 scripts/Makefile.lib | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index f5ab7af0f4..f41b16781d 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -179,10 +179,13 @@ ifdef DEVICE_TREE_DEBUG
 u_boot_dtsi_options_debug = $(warning $(u_boot_dtsi_options_raw))
 endif
 
-# We use the first match
-u_boot_dtsi = $(strip $(u_boot_dtsi_options_debug) \
+# We use the first match to be included
+include_files = $(strip $(u_boot_dtsi_options_debug) \
 	$(notdir $(firstword $(u_boot_dtsi_options))))
 
+# The CONFIG_DEVICE_TREE_INCLUDES also need to be included
+include_files += $(CONFIG_DEVICE_TREE_INCLUDES)
+
 # Modified for U-Boot
 dtc_cpp_flags  = -Wp,-MD,$(depfile).pre.tmp -nostdinc                    \
 		 $(UBOOTINCLUDE)                                         \
@@ -320,8 +323,8 @@ quiet_cmd_dtc = DTC     $@
 # Bring in any U-Boot-specific include at the end of the file
 # And finally any custom .dtsi fragments specified with CONFIG_DEVICE_TREE_INCLUDES
 cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
-	(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
-	$(foreach f,$(subst $(quote),,$(CONFIG_DEVICE_TREE_INCLUDES)), \
+	(cat $< > $(pre-tmp)); \
+	$(foreach f,$(subst $(quote),,$(include_files)), \
 	  echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
 	$(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \
 	$(DTC) -O dtb -o $@ -b 0 \
-- 
2.34.1

[RFC PATCH 2/4] scripts/Makefile.lib: Embed capsule public key in platform's dtb

[Sughosh Ganu]

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 target for generating a dtsi file which contains the signature
node with the ESL file included as a property under the signature
node. Include the dtsi file in the dtb. This brings the embedding of
the ESL in the dtb into the U-Boot build flow.

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>
---
 lib/efi_loader/Kconfig             |  9 +++++++++
 lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
 scripts/Makefile.lib               | 17 +++++++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 lib/efi_loader/capsule_esl.dtsi.in

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 9989e3f384..f40a62a0ba 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -272,6 +272,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 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
diff --git a/lib/efi_loader/capsule_esl.dtsi.in b/lib/efi_loader/capsule_esl.dtsi.in
new file mode 100644
index 0000000000..61a9f2b25e
--- /dev/null
+++ b/lib/efi_loader/capsule_esl.dtsi.in
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0+
+/**
+ * Devicetree file with the public key EFI Signature List(ESL)
+ * node. This file is used to generate the dtsi file to be
+ * included into the DTB.
+*/
+/ {
+	signature {
+		capsule-key = /incbin/("ESL_BIN_FILE");
+	};
+};
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index f41b16781d..cf4eef0b05 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -334,8 +334,25 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
 		; \
 	sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
 
+ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
+quiet_cmd_capsule_esl_gen = CAPSULE_ESL_GEN $@
+cmd_capsule_esl_gen = \
+	$(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" $(capsule_esl_input_file) > $@)
+
+$(obj)/.capsule_esl.dtsi:
+	$(call cmd_capsule_esl_gen)
+
+capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in
+capsule_esl_dtsi = .capsule_esl.dtsi
+capsule_esl_path=$(abspath $(srctree)/$(subst $(quote),,$(CONFIG_EFI_CAPSULE_ESL_FILE)))
+include_files += $(capsule_esl_dtsi)
+
+$(obj)/%.dtb: $(src)/%.dts $(DTC) $(obj)/.capsule_esl.dtsi FORCE
+	$(call if_changed_dep,dtc)
+else
 $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
 	$(call if_changed_dep,dtc)
+endif
 
 pre-tmp = $(subst $(comma),_,$(dot-target).pre.tmp)
 dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp)
-- 
2.34.1

[RFC PATCH 3/4] sandbox: capsule: Add path to the public key ESL file

[Sughosh Ganu]

Add the path to the public key EFI Signature List(ESL) file for the
sandbox variants which enable capsule authentication. This ESL file
gets embedded into the platform's device-tree as part of the build.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 configs/sandbox_defconfig          | 1 +
 configs/sandbox_flattree_defconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 1cd1c2ed7c..9f349d482b 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -340,6 +340,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/capsule_pub_esl_good.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..2a24b38cfb 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/capsule_pub_esl_good.esl"
 CONFIG_UNIT_TEST=y
 CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
-- 
2.34.1

[RFC PATCH 4/4] doc: capsule: Document the new mechanism to embed ESL file into dtb

[Sughosh Ganu]

Update the document to specify how the EFI Signature List(ESL) file
can be embedded into the platform's dtb as part of the u-boot build.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 doc/develop/uefi/uefi.rst | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
index 1ab5e5e2d1..6a0709efe3 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -538,20 +538,11 @@ and used by the steps highlighted below.
             ...
     }
 
-You can do step-4 manually with
-
-.. code-block:: console
-
-    $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts
-    $ fdtoverlay -i orig.dtb -o new.dtb -v signature.dtbo
-
-where signature.dts looks like::
-
-    &{/} {
-            signature {
-                    capsule-key = /incbin/("CRT.esl");
-            };
-    };
+You can perform step-4 by defining the Kconfig symbol
+CONFIG_EFI_CAPSULE_ESL_FILE. This symbol defines the path to the esl
+file generated in step-2. Once the symbol has been populated with the
+path to the esl file, the esl file will automatically get embedded
+into the platform's dtb as part of U-Boot build.
 
 Anti-rollback Protection
 ************************
-- 
2.34.1

Re: [RFC PATCH 1/4] scripts/Makefile.lib: Collate all dtsi files for inclusion

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

On Mon, Aug 14, 2023 at 02:33:06PM +0530, Sughosh Ganu wrote:

> At the time of building a device-tree file, all the *u-boot.dtsi files
> are looked for, in a particular order, and the first file found is
> included. Then, the list of files specified in the
> CONFIG_DEVICE_TREE_INCLUDES symbol are included.
> 
> Combine these files that are to be included into a variable, and then
> include all these files in one go.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  scripts/Makefile.lib | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index f5ab7af0f4..f41b16781d 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -179,10 +179,13 @@ ifdef DEVICE_TREE_DEBUG
>  u_boot_dtsi_options_debug = $(warning $(u_boot_dtsi_options_raw))
>  endif
>  
> -# We use the first match
> -u_boot_dtsi = $(strip $(u_boot_dtsi_options_debug) \
> +# We use the first match to be included
> +include_files = $(strip $(u_boot_dtsi_options_debug) \
>  	$(notdir $(firstword $(u_boot_dtsi_options))))
>  
> +# The CONFIG_DEVICE_TREE_INCLUDES also need to be included
> +include_files += $(CONFIG_DEVICE_TREE_INCLUDES)

This is what I wanted, logic-wise, but I think include_files is too
vague.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC PATCH 2/4] scripts/Makefile.lib: Embed capsule public key in platform's dtb

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2711 bytes --]

On Mon, Aug 14, 2023 at 02:33:07PM +0530, Sughosh Ganu 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 target for generating a dtsi file which contains the signature
> node with the ESL file included as a property under the signature
> node. Include the dtsi file in the dtb. This brings the embedding of
> the ESL in the dtb into the U-Boot build flow.
> 
> 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>
> ---
>  lib/efi_loader/Kconfig             |  9 +++++++++
>  lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
>  scripts/Makefile.lib               | 17 +++++++++++++++++
>  3 files changed, 37 insertions(+)
>  create mode 100644 lib/efi_loader/capsule_esl.dtsi.in
> 
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 9989e3f384..f40a62a0ba 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -272,6 +272,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.

[snip]
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index f41b16781d..cf4eef0b05 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -334,8 +334,25 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>  		; \
>  	sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
>  
> +ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> +quiet_cmd_capsule_esl_gen = CAPSULE_ESL_GEN $@
> +cmd_capsule_esl_gen = \
> +	$(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" $(capsule_esl_input_file) > $@)
> +
> +$(obj)/.capsule_esl.dtsi:
> +	$(call cmd_capsule_esl_gen)
> +
> +capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in
> +capsule_esl_dtsi = .capsule_esl.dtsi
> +capsule_esl_path=$(abspath $(srctree)/$(subst $(quote),,$(CONFIG_EFI_CAPSULE_ESL_FILE)))

This seems right.

> +include_files += $(capsule_esl_dtsi)
> +
> +$(obj)/%.dtb: $(src)/%.dts $(DTC) $(obj)/.capsule_esl.dtsi FORCE
> +	$(call if_changed_dep,dtc)
> +else
>  $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
>  	$(call if_changed_dep,dtc)
> +endif

I think this implies to me that we should have been depending on
$(include_files) (once renamed to be less vague) here already ?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC PATCH 1/4] scripts/Makefile.lib: Collate all dtsi files for inclusion

[Sughosh Ganu]

hi Tom,

On Mon, 14 Aug 2023 at 20:34, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Aug 14, 2023 at 02:33:06PM +0530, Sughosh Ganu wrote:
>
> > At the time of building a device-tree file, all the *u-boot.dtsi files
> > are looked for, in a particular order, and the first file found is
> > included. Then, the list of files specified in the
> > CONFIG_DEVICE_TREE_INCLUDES symbol are included.
> >
> > Combine these files that are to be included into a variable, and then
> > include all these files in one go.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  scripts/Makefile.lib | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index f5ab7af0f4..f41b16781d 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -179,10 +179,13 @@ ifdef DEVICE_TREE_DEBUG
> >  u_boot_dtsi_options_debug = $(warning $(u_boot_dtsi_options_raw))
> >  endif
> >
> > -# We use the first match
> > -u_boot_dtsi = $(strip $(u_boot_dtsi_options_debug) \
> > +# We use the first match to be included
> > +include_files = $(strip $(u_boot_dtsi_options_debug) \
> >       $(notdir $(firstword $(u_boot_dtsi_options))))
> >
> > +# The CONFIG_DEVICE_TREE_INCLUDES also need to be included
> > +include_files += $(CONFIG_DEVICE_TREE_INCLUDES)
>
> This is what I wanted, logic-wise, but I think include_files is too
> vague.

Okay. How about dtsi_include_list, or dtsi_list? If not, can you
suggest a name which you think would be apt. Thanks.

-sughosh

Re: [RFC PATCH 2/4] scripts/Makefile.lib: Embed capsule public key in platform's dtb

[Sughosh Ganu]

hi Tom,

On Mon, 14 Aug 2023 at 20:34, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Aug 14, 2023 at 02:33:07PM +0530, Sughosh Ganu 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 target for generating a dtsi file which contains the signature
> > node with the ESL file included as a property under the signature
> > node. Include the dtsi file in the dtb. This brings the embedding of
> > the ESL in the dtb into the U-Boot build flow.
> >
> > 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>
> > ---
> >  lib/efi_loader/Kconfig             |  9 +++++++++
> >  lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
> >  scripts/Makefile.lib               | 17 +++++++++++++++++
> >  3 files changed, 37 insertions(+)
> >  create mode 100644 lib/efi_loader/capsule_esl.dtsi.in
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 9989e3f384..f40a62a0ba 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -272,6 +272,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.

I think Simon wanted to keep this so that it would not break the build
if no option was provided.

>
> [snip]
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index f41b16781d..cf4eef0b05 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -334,8 +334,25 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> >               ; \
> >       sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
> >
> > +ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> > +quiet_cmd_capsule_esl_gen = CAPSULE_ESL_GEN $@
> > +cmd_capsule_esl_gen = \
> > +     $(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" $(capsule_esl_input_file) > $@)
> > +
> > +$(obj)/.capsule_esl.dtsi:
> > +     $(call cmd_capsule_esl_gen)
> > +
> > +capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in
> > +capsule_esl_dtsi = .capsule_esl.dtsi
> > +capsule_esl_path=$(abspath $(srctree)/$(subst $(quote),,$(CONFIG_EFI_CAPSULE_ESL_FILE)))
>
> This seems right.
>
> > +include_files += $(capsule_esl_dtsi)
> > +
> > +$(obj)/%.dtb: $(src)/%.dts $(DTC) $(obj)/.capsule_esl.dtsi FORCE
> > +     $(call if_changed_dep,dtc)
> > +else
> >  $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> >       $(call if_changed_dep,dtc)
> > +endif
>
> I think this implies to me that we should have been depending on
> $(include_files) (once renamed to be less vague) here already ?

Okay. That can be done. I had kept the .capsule_esl.dtsi as a
dependency primarily because that file needs to be generated before it
can be included. I suppose the same can apply to some other dtsi as
well.

-sughosh

Re: [RFC PATCH 1/4] scripts/Makefile.lib: Collate all dtsi files for inclusion

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

On Tue, Aug 15, 2023 at 12:12:57AM +0530, Sughosh Ganu wrote:
> hi Tom,
> 
> On Mon, 14 Aug 2023 at 20:34, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Aug 14, 2023 at 02:33:06PM +0530, Sughosh Ganu wrote:
> >
> > > At the time of building a device-tree file, all the *u-boot.dtsi files
> > > are looked for, in a particular order, and the first file found is
> > > included. Then, the list of files specified in the
> > > CONFIG_DEVICE_TREE_INCLUDES symbol are included.
> > >
> > > Combine these files that are to be included into a variable, and then
> > > include all these files in one go.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >  scripts/Makefile.lib | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index f5ab7af0f4..f41b16781d 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -179,10 +179,13 @@ ifdef DEVICE_TREE_DEBUG
> > >  u_boot_dtsi_options_debug = $(warning $(u_boot_dtsi_options_raw))
> > >  endif
> > >
> > > -# We use the first match
> > > -u_boot_dtsi = $(strip $(u_boot_dtsi_options_debug) \
> > > +# We use the first match to be included
> > > +include_files = $(strip $(u_boot_dtsi_options_debug) \
> > >       $(notdir $(firstword $(u_boot_dtsi_options))))
> > >
> > > +# The CONFIG_DEVICE_TREE_INCLUDES also need to be included
> > > +include_files += $(CONFIG_DEVICE_TREE_INCLUDES)
> >
> > This is what I wanted, logic-wise, but I think include_files is too
> > vague.
> 
> Okay. How about dtsi_include_list, or dtsi_list? If not, can you
> suggest a name which you think would be apt. Thanks.

dtsi_include_list sounds good.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC PATCH 2/4] scripts/Makefile.lib: Embed capsule public key in platform's dtb

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4248 bytes --]

On Tue, Aug 15, 2023 at 12:19:36AM +0530, Sughosh Ganu wrote:
> hi Tom,
> 
> On Mon, 14 Aug 2023 at 20:34, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Aug 14, 2023 at 02:33:07PM +0530, Sughosh Ganu 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 target for generating a dtsi file which contains the signature
> > > node with the ESL file included as a property under the signature
> > > node. Include the dtsi file in the dtb. This brings the embedding of
> > > the ESL in the dtb into the U-Boot build flow.
> > >
> > > 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>
> > > ---
> > >  lib/efi_loader/Kconfig             |  9 +++++++++
> > >  lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
> > >  scripts/Makefile.lib               | 17 +++++++++++++++++
> > >  3 files changed, 37 insertions(+)
> > >  create mode 100644 lib/efi_loader/capsule_esl.dtsi.in
> > >
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index 9989e3f384..f40a62a0ba 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -272,6 +272,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.
> 
> I think Simon wanted to keep this so that it would not break the build
> if no option was provided.

No, that means that there's a missing dependency.  Blank defaults are
almost never right.  And "so that we don't break configs" means that
something else is wrong, generally a missing dependency.  And maybe I
cut out too much context here as yes, this option needs to depend on
some part of the capsule framework.

> > [snip]
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index f41b16781d..cf4eef0b05 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -334,8 +334,25 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> > >               ; \
> > >       sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
> > >
> > > +ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> > > +quiet_cmd_capsule_esl_gen = CAPSULE_ESL_GEN $@
> > > +cmd_capsule_esl_gen = \
> > > +     $(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" $(capsule_esl_input_file) > $@)
> > > +
> > > +$(obj)/.capsule_esl.dtsi:
> > > +     $(call cmd_capsule_esl_gen)
> > > +
> > > +capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in
> > > +capsule_esl_dtsi = .capsule_esl.dtsi
> > > +capsule_esl_path=$(abspath $(srctree)/$(subst $(quote),,$(CONFIG_EFI_CAPSULE_ESL_FILE)))
> >
> > This seems right.
> >
> > > +include_files += $(capsule_esl_dtsi)
> > > +
> > > +$(obj)/%.dtb: $(src)/%.dts $(DTC) $(obj)/.capsule_esl.dtsi FORCE
> > > +     $(call if_changed_dep,dtc)
> > > +else
> > >  $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> > >       $(call if_changed_dep,dtc)
> > > +endif
> >
> > I think this implies to me that we should have been depending on
> > $(include_files) (once renamed to be less vague) here already ?
> 
> Okay. That can be done. I had kept the .capsule_esl.dtsi as a
> dependency primarily because that file needs to be generated before it
> can be included. I suppose the same can apply to some other dtsi as
> well.

You shouldn't need to have it be explicit, if it's in dtsi_include_list
as there's a rule so make can resolve how to satisfy the dependency.
But since we dynamically #include the other files, I don't know that
they will be caught with the normal dependency logic.  But, it will
still be cleaner to have the dtb rules depend on the automagic #include
files too, rather than special casing the rule here I believe.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [RFC PATCH 2/4] scripts/Makefile.lib: Embed capsule public key in platform's dtb

[Sughosh Ganu]

hi Tom,

On Tue, 15 Aug 2023 at 00:48, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Aug 15, 2023 at 12:19:36AM +0530, Sughosh Ganu wrote:
> > hi Tom,
> >
> > On Mon, 14 Aug 2023 at 20:34, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Aug 14, 2023 at 02:33:07PM +0530, Sughosh Ganu 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 target for generating a dtsi file which contains the signature
> > > > node with the ESL file included as a property under the signature
> > > > node. Include the dtsi file in the dtb. This brings the embedding of
> > > > the ESL in the dtb into the U-Boot build flow.
> > > >
> > > > 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>
> > > > ---
> > > >  lib/efi_loader/Kconfig             |  9 +++++++++
> > > >  lib/efi_loader/capsule_esl.dtsi.in | 11 +++++++++++
> > > >  scripts/Makefile.lib               | 17 +++++++++++++++++
> > > >  3 files changed, 37 insertions(+)
> > > >  create mode 100644 lib/efi_loader/capsule_esl.dtsi.in
> > > >
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index 9989e3f384..f40a62a0ba 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -272,6 +272,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.
> >
> > I think Simon wanted to keep this so that it would not break the build
> > if no option was provided.
>
> No, that means that there's a missing dependency.  Blank defaults are
> almost never right.  And "so that we don't break configs" means that
> something else is wrong, generally a missing dependency.  And maybe I
> cut out too much context here as yes, this option needs to depend on
> some part of the capsule framework.

Okay. Yes, the above config does actually depend on EFI_CAPSULE_AUTHENTICATE.

>
> > > [snip]
> > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > index f41b16781d..cf4eef0b05 100644
> > > > --- a/scripts/Makefile.lib
> > > > +++ b/scripts/Makefile.lib
> > > > @@ -334,8 +334,25 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
> > > >               ; \
> > > >       sed "s:$(pre-tmp):$(<):" $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
> > > >
> > > > +ifdef CONFIG_EFI_CAPSULE_AUTHENTICATE
> > > > +quiet_cmd_capsule_esl_gen = CAPSULE_ESL_GEN $@
> > > > +cmd_capsule_esl_gen = \
> > > > +     $(shell sed "s:ESL_BIN_FILE:$(capsule_esl_path):" $(capsule_esl_input_file) > $@)
> > > > +
> > > > +$(obj)/.capsule_esl.dtsi:
> > > > +     $(call cmd_capsule_esl_gen)
> > > > +
> > > > +capsule_esl_input_file=$(srctree)/lib/efi_loader/capsule_esl.dtsi.in
> > > > +capsule_esl_dtsi = .capsule_esl.dtsi
> > > > +capsule_esl_path=$(abspath $(srctree)/$(subst $(quote),,$(CONFIG_EFI_CAPSULE_ESL_FILE)))
> > >
> > > This seems right.
> > >
> > > > +include_files += $(capsule_esl_dtsi)
> > > > +
> > > > +$(obj)/%.dtb: $(src)/%.dts $(DTC) $(obj)/.capsule_esl.dtsi FORCE
> > > > +     $(call if_changed_dep,dtc)
> > > > +else
> > > >  $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> > > >       $(call if_changed_dep,dtc)
> > > > +endif
> > >
> > > I think this implies to me that we should have been depending on
> > > $(include_files) (once renamed to be less vague) here already ?
> >
> > Okay. That can be done. I had kept the .capsule_esl.dtsi as a
> > dependency primarily because that file needs to be generated before it
> > can be included. I suppose the same can apply to some other dtsi as
> > well.
>
> You shouldn't need to have it be explicit, if it's in dtsi_include_list
> as there's a rule so make can resolve how to satisfy the dependency.
> But since we dynamically #include the other files, I don't know that
> they will be caught with the normal dependency logic.  But, it will
> still be cleaner to have the dtb rules depend on the automagic #include
> files too, rather than special casing the rule here I believe.

Okay, in that case, there can be a single rule for making the dtb.
Only the logic for generating the .capsule_esl.dtsi can then be put
under the ifdef.

-sughosh