[PATCH 0/5] Add support for embedding public key in platform's dtb

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Sughosh Ganu]

Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config option
when capsule authentication is enabled.

Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE
which are used for enabling embedding of the public key in the dtb,
and specifying the esl file name.

Patch 3 moves efi_capsule_auth_enabled as a weak function, which can
be used as a default mechanism for checking if capsule authentication
has been enabled.

Patch 4 adds a default weak function for retrieving the public key
from the platform's dtb.

Patch 5 adds the functionality to embed the esl file into the
platform's dtb during the platform build.

I have tested this functionality on the STM32MP157C DK2 board.

[1] - https://lists.denx.de/pipermail/u-boot/2021-March/442867.html 

Sughosh Ganu (5):
  efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
    authentication is enabled
  efi_loader: Kconfig: Add symbols for embedding the public key into the
    platform's dtb
  efi_capsule: Add a weak function to check whether capsule
    authentication is enabled
  efi_capsule: Add a weak function to get the public key needed for
    capsule authentication
  Makefile: Add provision for embedding public key in platform's dtb

 Makefile                              | 10 ++++++
 board/emulation/common/qemu_capsule.c |  6 ----
 lib/efi_loader/Kconfig                | 16 ++++++++++
 lib/efi_loader/efi_capsule.c          | 44 ++++++++++++++++++++++++---
 4 files changed, 66 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH 1/5] efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule authentication is enabled

[Sughosh Ganu]

Enable building of the crypto helper functions used during capsule
authentication by selecting IMAGE_SIGN_INFO.

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

This was not detected when support for capsule auth was added to the
qemu arm64 platform. This is because the platform includes
CONFIG_FIT_SIGNATURE which selects IMAGE_SIGN_INFO.


 lib/efi_loader/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index e44f004f3f..0b99d7c774 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -173,6 +173,7 @@ config EFI_CAPSULE_AUTHENTICATE
 	select X509_CERTIFICATE_PARSER
 	select PKCS7_MESSAGE_PARSER
 	select PKCS7_VERIFY
+	select IMAGE_SIGN_INFO
 	default n
 	help
 	  Select this option if you want to enable capsule
-- 
2.17.1

[PATCH 2/5] efi_loader: Kconfig: Add symbols for embedding the public key into the platform's dtb

[Sughosh Ganu]

Add config options EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE which are to
be used for embedding the public key to be used for capsule
authentication into the platform's device tree.

The embedding of the public key would take place during the platform
build process.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 lib/efi_loader/Kconfig | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 0b99d7c774..de3083a979 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -179,6 +179,21 @@ config EFI_CAPSULE_AUTHENTICATE
 	  Select this option if you want to enable capsule
 	  authentication
 
+config EFI_PKEY_DTB_EMBED
+	bool "Embed the public key in the Device Tree"
+	default n
+	depends on EFI_CAPSULE_AUTHENTICATE
+	help
+	  Select this option if the public key used for capsule
+	  authentication is to be embedded into the platform's
+	  device tree.
+
+config EFI_PKEY_FILE
+	string "Public Key esl file to be embedded into the Device Tree"
+	help
+	  Specify the absolute path of the public key esl file that is
+	  to be embedded in the platform's device tree.
+
 config EFI_CAPSULE_FIRMWARE_FIT
 	bool "FMP driver for FIT image"
 	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
-- 
2.17.1

[PATCH 3/5] efi_capsule: Add a weak function to check whether capsule authentication is enabled

[Sughosh Ganu]

Define a weak function which checks if the environment variable
capsule_authentication_enabled has been set, for enabling capsule
authentication. Other platforms might have a different mechanism to
determine this, and would then define their own platform specific
function.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 board/emulation/common/qemu_capsule.c | 6 ------
 lib/efi_loader/efi_capsule.c          | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c
index 5cb461d52b..6b8a87022a 100644
--- a/board/emulation/common/qemu_capsule.c
+++ b/board/emulation/common/qemu_capsule.c
@@ -41,9 +41,3 @@ int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
 
 	return 0;
 }
-
-bool efi_capsule_auth_enabled(void)
-{
-	return env_get("capsule_authentication_enabled") != NULL ?
-		true : false;
-}
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 0cfff0daf7..1423b675c8 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -218,6 +218,12 @@ __weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
 	return 0;
 }
 
+__weak bool efi_capsule_auth_enabled(void)
+{
+	return env_get("capsule_authentication_enabled") ?
+		true : false;
+}
+
 efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
 				      void **image, efi_uintn_t *image_size)
 {
-- 
2.17.1

[PATCH 4/5] efi_capsule: Add a weak function to get the public key needed for capsule authentication

[Sughosh Ganu]

Define a weak function which would be used in the scenario where the
public key is stored on the platform's dtb. This dtb is concatenated
with the u-boot binary during the build process. Platforms which have
a different mechanism for getting the public key would define their
own platform specific function.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 lib/efi_loader/efi_capsule.c | 38 ++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 1423b675c8..fc5e1c0856 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -14,10 +14,13 @@
 #include <mapmem.h>
 #include <sort.h>
 
+#include <asm/global_data.h>
 #include <crypto/pkcs7.h>
 #include <crypto/pkcs7_parser.h>
 #include <linux/err.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
 static const efi_guid_t efi_guid_firmware_management_capsule_id =
 		EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
@@ -210,11 +213,38 @@ const efi_guid_t efi_guid_capsule_root_cert_guid =
 
 __weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
 {
-	/* The platform is supposed to provide
-	 * a method for getting the public key
-	 * stored in the form of efi signature
-	 * list
+	/*
+	 * This is a function for retrieving the public key from the
+	 * platform's device tree. The platform's device tree has been
+	 * concatenated with the u-boot binary.
+	 * If a platform has a different mechanism to get the public
+	 * key, it can define it's own function.
 	 */
+	const void *fdt_blob = gd->fdt_blob;
+	const void *blob;
+	const char *cnode_name = "capsule-key";
+	const char *snode_name = "signature";
+	int sig_node;
+	int len;
+
+	sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name);
+	if (sig_node < 0) {
+		EFI_PRINT("Unable to get signature node offset\n");
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len);
+
+	if (!blob || len < 0) {
+		EFI_PRINT("Unable to get capsule-key value\n");
+		*pkey = NULL;
+		*pkey_len = 0;
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	*pkey = (void *)blob;
+	*pkey_len = len;
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH 5/5] Makefile: Add provision for embedding public key in platform's dtb

[Sughosh Ganu]

Add provision for embedding the public key used for capsule
authentication in the platform's dtb. This is done by invoking the
mkeficapsule utility which puts the public key in the efi signature
list(esl) format into the dtb.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 Makefile | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Makefile b/Makefile
index 193aa4d1c9..0d50c6a805 100644
--- a/Makefile
+++ b/Makefile
@@ -1010,6 +1010,10 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; }
 quiet_cmd_lzma = LZMA    $@
 cmd_lzma = lzma -c -z -k -9 $< > $@
 
+quiet_cmd_mkeficapsule = MKEFICAPSULE     $@
+cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \
+	-D $@
+
 cfg: u-boot.cfg
 
 quiet_cmd_cfgcheck = CFGCHK  $2
@@ -1104,8 +1108,14 @@ endif
 PHONY += dtbs
 dtbs: dts/dt.dtb
 	@:
+ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE)$(CONFIG_EFI_PKEY_DTB_EMBED),yy)
+dts/dt.dtb: u-boot tools
+	$(Q)$(MAKE) $(build)=dts dtbs
+	$(call cmd,mkeficapsule)
+else
 dts/dt.dtb: u-boot
 	$(Q)$(MAKE) $(build)=dts dtbs
+endif
 
 quiet_cmd_copy = COPY    $@
       cmd_copy = cp $< $@
-- 
2.17.1

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Simon Glass]

Hi,

On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config option
> when capsule authentication is enabled.
>
> Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE
> which are used for enabling embedding of the public key in the dtb,
> and specifying the esl file name.
>
> Patch 3 moves efi_capsule_auth_enabled as a weak function, which can
> be used as a default mechanism for checking if capsule authentication
> has been enabled.
>
> Patch 4 adds a default weak function for retrieving the public key
> from the platform's dtb.
>
> Patch 5 adds the functionality to embed the esl file into the
> platform's dtb during the platform build.
>
> I have tested this functionality on the STM32MP157C DK2 board.
>
> [1] - https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
>
> Sughosh Ganu (5):
>   efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
>     authentication is enabled
>   efi_loader: Kconfig: Add symbols for embedding the public key into the
>     platform's dtb
>   efi_capsule: Add a weak function to check whether capsule
>     authentication is enabled
>   efi_capsule: Add a weak function to get the public key needed for
>     capsule authentication
>   Makefile: Add provision for embedding public key in platform's dtb
>
>  Makefile                              | 10 ++++++
>  board/emulation/common/qemu_capsule.c |  6 ----
>  lib/efi_loader/Kconfig                | 16 ++++++++++
>  lib/efi_loader/efi_capsule.c          | 44 ++++++++++++++++++++++++---
>  4 files changed, 66 insertions(+), 10 deletions(-)
>
> --
> 2.17.1
>

We need to rethink the use of weak functions for this sort of thing,
or we will end up with an unnavigable mess at some point. If we need
to adjust the flow of boot, let's adjust the flow rather than adding
hooks everywhere.

Regards,
Simon

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Sughosh Ganu]

hi Simon,

On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org> wrote:

> Hi,
>
> On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config option
> > when capsule authentication is enabled.
> >
> > Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE
> > which are used for enabling embedding of the public key in the dtb,
> > and specifying the esl file name.
> >
> > Patch 3 moves efi_capsule_auth_enabled as a weak function, which can
> > be used as a default mechanism for checking if capsule authentication
> > has been enabled.
> >
> > Patch 4 adds a default weak function for retrieving the public key
> > from the platform's dtb.
> >
> > Patch 5 adds the functionality to embed the esl file into the
> > platform's dtb during the platform build.
> >
> > I have tested this functionality on the STM32MP157C DK2 board.
> >
> > [1] - https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
> >
> > Sughosh Ganu (5):
> >   efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
> >     authentication is enabled
> >   efi_loader: Kconfig: Add symbols for embedding the public key into the
> >     platform's dtb
> >   efi_capsule: Add a weak function to check whether capsule
> >     authentication is enabled
> >   efi_capsule: Add a weak function to get the public key needed for
> >     capsule authentication
> >   Makefile: Add provision for embedding public key in platform's dtb
> >
> >  Makefile                              | 10 ++++++
> >  board/emulation/common/qemu_capsule.c |  6 ----
> >  lib/efi_loader/Kconfig                | 16 ++++++++++
> >  lib/efi_loader/efi_capsule.c          | 44 ++++++++++++++++++++++++---
> >  4 files changed, 66 insertions(+), 10 deletions(-)
> >
> > --
> > 2.17.1
> >
>
> We need to rethink the use of weak functions for this sort of thing,
> or we will end up with an unnavigable mess at some point. If we need
> to adjust the flow of boot, let's adjust the flow rather than adding
> hooks everywhere.


There are two weak functions being added. One is for retrieving the public
key to be used for the capsule authentication, and the other is for
checking for whether capsule authentication has been enabled. The reason
why a weak function is needed is because platforms can have other
mechanisms for retrieval of the public key or for testing if capsule
authentication has been enabled.

If we consider the case of public key retrieval, the majority of platforms
would be built with the device tree concatenated with the u-boot binary.
The weak function would cater to all of those platforms -- having a weak
function would mean that we are not required to repeat the same
functionality for every platform that uses the same mechanism for
extracting the public key. However, there would be platforms where the
public key is obtained through a different mechanism which is platform
specific. Those platforms would have to define their own function to get
the public key. Same for checking whether capsule authentication feature
has been enabled or not.

-sughosh

[PATCH 1/5] efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule authentication is enabled

[Heinrich Schuchardt]

On 07.04.21 13:53, Sughosh Ganu wrote:
> Enable building of the crypto helper functions used during capsule
> authentication by selecting IMAGE_SIGN_INFO.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> This was not detected when support for capsule auth was added to the
> qemu arm64 platform. This is because the platform includes
> CONFIG_FIT_SIGNATURE which selects IMAGE_SIGN_INFO.
>
>
>  lib/efi_loader/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index e44f004f3f..0b99d7c774 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -173,6 +173,7 @@ config EFI_CAPSULE_AUTHENTICATE
>  	select X509_CERTIFICATE_PARSER
>  	select PKCS7_MESSAGE_PARSER
>  	select PKCS7_VERIFY
> +	select IMAGE_SIGN_INFO
>  	default n
>  	help
>  	  Select this option if you want to enable capsule
>

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Heinrich Schuchardt]

On 08.04.21 08:53, Sughosh Ganu wrote:
> hi?Simon,
>
> On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org
> <mailto:sjg@chromium.org>> wrote:
>
>     Hi,
>
>     On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu <sughosh.ganu@linaro.org
>     <mailto:sughosh.ganu@linaro.org>> wrote:
>     >
>     > Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config option
>     > when capsule authentication is enabled.
>     >
>     > Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE
>     > which are used for enabling embedding of the public key in the dtb,
>     > and specifying the esl file name.
>     >
>     > Patch 3 moves efi_capsule_auth_enabled as a weak function, which can
>     > be used as a default mechanism for checking if capsule authentication
>     > has been enabled.
>     >
>     > Patch 4 adds a default weak function for retrieving the public key
>     > from the platform's dtb.
>     >
>     > Patch 5 adds the functionality to embed the esl file into the
>     > platform's dtb during the platform build.
>     >
>     > I have tested this functionality on the STM32MP157C DK2 board.
>     >
>     > [1] -
>     https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
>     <https://lists.denx.de/pipermail/u-boot/2021-March/442867.html>
>     >
>     > Sughosh Ganu (5):
>     >? ?efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
>     >? ? ?authentication is enabled
>     >? ?efi_loader: Kconfig: Add symbols for embedding the public key
>     into the
>     >? ? ?platform's dtb
>     >? ?efi_capsule: Add a weak function to check whether capsule
>     >? ? ?authentication is enabled
>     >? ?efi_capsule: Add a weak function to get the public key needed for
>     >? ? ?capsule authentication
>     >? ?Makefile: Add provision for embedding public key in platform's dtb
>     >
>     >? Makefile? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | 10 ++++++
>     >? board/emulation/common/qemu_capsule.c |? 6 ----
>     >? lib/efi_loader/Kconfig? ? ? ? ? ? ? ? | 16 ++++++++++
>     >? lib/efi_loader/efi_capsule.c? ? ? ? ? | 44
>     ++++++++++++++++++++++++---
>     >? 4 files changed, 66 insertions(+), 10 deletions(-)
>     >
>     > --
>     > 2.17.1
>     >
>
>     We need to rethink the use of weak functions for this sort of thing,
>     or we will end up with an unnavigable mess at some point. If we need
>     to adjust the flow of boot, let's adjust the flow rather than adding
>     hooks everywhere.
>
>
> There are two weak functions being added. One is for retrieving the
> public key to be used for the capsule authentication, and the other is
> for checking for whether capsule authentication has been enabled. The
> reason why a weak function is needed is because platforms can have other
> mechanisms for retrieval of the public key or for testing if capsule
> authentication has been enabled.
>
> If we consider the case of public key retrieval, the majority of
> platforms would be built with the device tree concatenated with the
> u-boot binary. The weak function would cater to all of those platforms
> -- having a weak function would mean that we are not required to repeat
> the same functionality for every platform that uses the same mechanism
> for extracting the public key. However, there would be platforms where
> the public key is obtained through a different mechanism which is
> platform specific. Those platforms would have to define their own
> function to get the public key. Same for checking whether capsule
> authentication feature has been enabled or not.
>
> -sughosh

Hello Sughosh,

Could you, please, explain why there could be a need to use public keys
for capsule verification that are not compiled into U-Boot. I cannot see
how this would increase security.

I cannot imagine any scenario where you would want to allow switching
off capsule authentication if it has been built into U-Boot.

Best regards

Heinrich

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Sughosh Ganu]

hi Heinrich,

On Thu, 8 Apr 2021 at 14:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 08.04.21 08:53, Sughosh Ganu wrote:
> > hi Simon,
> >
> > On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org
> > <mailto:sjg@chromium.org>> wrote:
> >
> >     Hi,
> >
> >     On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu <sughosh.ganu@linaro.org
> >     <mailto:sughosh.ganu@linaro.org>> wrote:
> >     >
> >     > Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config
> option
> >     > when capsule authentication is enabled.
> >     >
> >     > Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and
> EFI_PKEY_FILE
> >     > which are used for enabling embedding of the public key in the dtb,
> >     > and specifying the esl file name.
> >     >
> >     > Patch 3 moves efi_capsule_auth_enabled as a weak function, which
> can
> >     > be used as a default mechanism for checking if capsule
> authentication
> >     > has been enabled.
> >     >
> >     > Patch 4 adds a default weak function for retrieving the public key
> >     > from the platform's dtb.
> >     >
> >     > Patch 5 adds the functionality to embed the esl file into the
> >     > platform's dtb during the platform build.
> >     >
> >     > I have tested this functionality on the STM32MP157C DK2 board.
> >     >
> >     > [1] -
> >     https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
> >     <https://lists.denx.de/pipermail/u-boot/2021-March/442867.html>
> >     >
> >     > Sughosh Ganu (5):
> >     >   efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
> >     >     authentication is enabled
> >     >   efi_loader: Kconfig: Add symbols for embedding the public key
> >     into the
> >     >     platform's dtb
> >     >   efi_capsule: Add a weak function to check whether capsule
> >     >     authentication is enabled
> >     >   efi_capsule: Add a weak function to get the public key needed for
> >     >     capsule authentication
> >     >   Makefile: Add provision for embedding public key in platform's
> dtb
> >     >
> >     >  Makefile                              | 10 ++++++
> >     >  board/emulation/common/qemu_capsule.c |  6 ----
> >     >  lib/efi_loader/Kconfig                | 16 ++++++++++
> >     >  lib/efi_loader/efi_capsule.c          | 44
> >     ++++++++++++++++++++++++---
> >     >  4 files changed, 66 insertions(+), 10 deletions(-)
> >     >
> >     > --
> >     > 2.17.1
> >     >
> >
> >     We need to rethink the use of weak functions for this sort of thing,
> >     or we will end up with an unnavigable mess at some point. If we need
> >     to adjust the flow of boot, let's adjust the flow rather than adding
> >     hooks everywhere.
> >
> >
> > There are two weak functions being added. One is for retrieving the
> > public key to be used for the capsule authentication, and the other is
> > for checking for whether capsule authentication has been enabled. The
> > reason why a weak function is needed is because platforms can have other
> > mechanisms for retrieval of the public key or for testing if capsule
> > authentication has been enabled.
> >
> > If we consider the case of public key retrieval, the majority of
> > platforms would be built with the device tree concatenated with the
> > u-boot binary. The weak function would cater to all of those platforms
> > -- having a weak function would mean that we are not required to repeat
> > the same functionality for every platform that uses the same mechanism
> > for extracting the public key. However, there would be platforms where
> > the public key is obtained through a different mechanism which is
> > platform specific. Those platforms would have to define their own
> > function to get the public key. Same for checking whether capsule
> > authentication feature has been enabled or not.
> >
> > -sughosh
>
> Hello Sughosh,
>
> Could you, please, explain why there could be a need to use public keys
> for capsule verification that are not compiled into U-Boot. I cannot see
> how this would increase security.
>

With the changes that have been made in the Makefile(patch 5/5), the public
key is now embedded into the platform's dtb, and subsequently this dtb is
concatenated with the u-boot binary to create a single u-boot.bin image.
This image can then be verified during the trusted boot flow to check
against any kind of tampering. This takes care of your concern of not
having the public key separately on the disk, which makes it open to
tampering, with the public key now embedded as part of the u-boot image.
You had suggested embedding the public key as part of the u-boot image. I
have embedded it within the platform's dtb which is part of the u-boot
image. This becomes a generic solution which is platform and architecture
agnostic. I believe concatenating the platform's dtb with the u-boot binary
is the standard flow for production images.

I cannot imagine any scenario where you would want to allow switching
> off capsule authentication if it has been built into U-Boot.
>

This is only an additional knob for any user who might want to perform a
capsule update without authentication -- with this additional knob, the
user can use the same image for updating a capsule which does not have an
authentication header. The user would not be required to recompile the
image to turn off CONFIG_EFI_CAPSULE_AUTHENTICATE config option. But if you
don't see this necessary, i can remove this additional check. In that case,
the capsule will be authenticated when CONFIG_EFI_CAPSULE_AUTHENTICATE is
enabled.

-sughosh

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Heinrich Schuchardt]

On 08.04.21 12:10, Sughosh Ganu wrote:
> hi Heinrich,
>
> On Thu, 8 Apr 2021 at 14:17, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     On 08.04.21 08:53, Sughosh Ganu wrote:
>     > hi?Simon,
>     >
>     > On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org
>     <mailto:sjg@chromium.org>
>     > <mailto:sjg at chromium.org <mailto:sjg@chromium.org>>> wrote:
>     >
>     >? ? ?Hi,
>     >
>     >? ? ?On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu
>     <sughosh.ganu at linaro.org <mailto:sughosh.ganu@linaro.org>
>     >? ? ?<mailto:sughosh.ganu@linaro.org
>     <mailto:sughosh.ganu@linaro.org>>> wrote:
>     >? ? ?>
>     >? ? ?> Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO
>     config option
>     >? ? ?> when capsule authentication is enabled.
>     >? ? ?>
>     >? ? ?> Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and
>     EFI_PKEY_FILE
>     >? ? ?> which are used for enabling embedding of the public key in
>     the dtb,
>     >? ? ?> and specifying the esl file name.
>     >? ? ?>
>     >? ? ?> Patch 3 moves efi_capsule_auth_enabled as a weak function,
>     which can
>     >? ? ?> be used as a default mechanism for checking if capsule
>     authentication
>     >? ? ?> has been enabled.
>     >? ? ?>
>     >? ? ?> Patch 4 adds a default weak function for retrieving the
>     public key
>     >? ? ?> from the platform's dtb.
>     >? ? ?>
>     >? ? ?> Patch 5 adds the functionality to embed the esl file into the
>     >? ? ?> platform's dtb during the platform build.
>     >? ? ?>
>     >? ? ?> I have tested this functionality on the STM32MP157C DK2 board.
>     >? ? ?>
>     >? ? ?> [1] -
>     >? ? ?https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
>     <https://lists.denx.de/pipermail/u-boot/2021-March/442867.html>
>     >? ? ?<https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
>     <https://lists.denx.de/pipermail/u-boot/2021-March/442867.html>>
>     >? ? ?>
>     >? ? ?> Sughosh Ganu (5):
>     >? ? ?>? ?efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
>     >? ? ?>? ? ?authentication is enabled
>     >? ? ?>? ?efi_loader: Kconfig: Add symbols for embedding the public key
>     >? ? ?into the
>     >? ? ?>? ? ?platform's dtb
>     >? ? ?>? ?efi_capsule: Add a weak function to check whether capsule
>     >? ? ?>? ? ?authentication is enabled
>     >? ? ?>? ?efi_capsule: Add a weak function to get the public key
>     needed for
>     >? ? ?>? ? ?capsule authentication
>     >? ? ?>? ?Makefile: Add provision for embedding public key in
>     platform's dtb
>     >? ? ?>
>     >? ? ?>? Makefile? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | 10 ++++++
>     >? ? ?>? board/emulation/common/qemu_capsule.c |? 6 ----
>     >? ? ?>? lib/efi_loader/Kconfig? ? ? ? ? ? ? ? | 16 ++++++++++
>     >? ? ?>? lib/efi_loader/efi_capsule.c? ? ? ? ? | 44
>     >? ? ?++++++++++++++++++++++++---
>     >? ? ?>? 4 files changed, 66 insertions(+), 10 deletions(-)
>     >? ? ?>
>     >? ? ?> --
>     >? ? ?> 2.17.1
>     >? ? ?>
>     >
>     >? ? ?We need to rethink the use of weak functions for this sort of
>     thing,
>     >? ? ?or we will end up with an unnavigable mess at some point. If
>     we need
>     >? ? ?to adjust the flow of boot, let's adjust the flow rather than
>     adding
>     >? ? ?hooks everywhere.
>     >
>     >
>     > There are two weak functions being added. One is for retrieving the
>     > public key to be used for the capsule authentication, and the other is
>     > for checking for whether capsule authentication has been enabled. The
>     > reason why a weak function is needed is because platforms can have
>     other
>     > mechanisms for retrieval of the public key or for testing if capsule
>     > authentication has been enabled.
>     >
>     > If we consider the case of public key retrieval, the majority of
>     > platforms would be built with the device tree concatenated with the
>     > u-boot binary. The weak function would cater to all of those platforms
>     > -- having a weak function would mean that we are not required to
>     repeat
>     > the same functionality for every platform that uses the same mechanism
>     > for extracting the public key. However, there would be platforms where
>     > the public key is obtained through a different mechanism which is
>     > platform specific. Those platforms would have to define their own
>     > function to get the public key. Same for checking whether capsule
>     > authentication feature has been enabled or not.
>     >
>     > -sughosh
>
>     Hello Sughosh,
>
>     Could you, please, explain why there could be a need to use public keys
>     for capsule verification that are not compiled into U-Boot. I cannot see
>     how this would increase security.
>
>
> With the changes that have been made in the Makefile(patch 5/5), the
> public key is now embedded into the platform's dtb, and subsequently
> this dtb is concatenated with the u-boot binary to create a single
> u-boot.bin image. This image can then be verified during the trusted
> boot flow to check against any kind of tampering. This takes care of
> your concern of not having the public key separately on the disk, which
> makes it open to tampering, with the public key now embedded as part of
> the u-boot image. You had suggested embedding the public key as part of
> the u-boot image. I have embedded it within the platform's dtb which is
> part of the u-boot image. This becomes a generic solution which is
> platform and architecture agnostic. I believe concatenating the
> platform's dtb with the u-boot binary is the standard flow for
> production images.

Embedding the key in the device-tree is fine. I am just trying to
understand why you need the extensibility via a weak function.

>
>     I cannot imagine any scenario where you would want to allow switching
>     off capsule authentication if it has been built into U-Boot.
>
>
> This is only an additional knob for any user who might want to perform a
> capsule update without authentication -- with this additional knob, the
> user can use?the same image for updating a capsule which does not have
> an authentication header. The user would not be required to recompile
> the image to turn off CONFIG_EFI_CAPSULE_AUTHENTICATE config option. But
> if you don't see this necessary, i can remove this additional check. In
> that case, the capsule will be authenticated when
> CONFIG_EFI_CAPSULE_AUTHENTICATE is enabled.

I would prefer to reduce the number of "knobs" that you have to check
when rolling out secure firmware.

Best regards

Heinrich

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Sughosh Ganu]

On Thu, 8 Apr 2021 at 16:51, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 08.04.21 12:10, Sughosh Ganu wrote:
> > hi Heinrich,
> >
> > On Thu, 8 Apr 2021 at 14:17, Heinrich Schuchardt <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> >
> >     On 08.04.21 08:53, Sughosh Ganu wrote:
> >     > hi Simon,
> >     >
> >     > On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org
> >     <mailto:sjg@chromium.org>
> >     > <mailto:sjg at chromium.org <mailto:sjg@chromium.org>>> wrote:
> >     >
> >     >     Hi,
> >     >
> >     >     On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu
> >     <sughosh.ganu at linaro.org <mailto:sughosh.ganu@linaro.org>
> >     >     <mailto:sughosh.ganu@linaro.org
> >     <mailto:sughosh.ganu@linaro.org>>> wrote:
> >     >     >
> >     >     > Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO
> >     config option
> >     >     > when capsule authentication is enabled.
> >     >     >
> >     >     > Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and
> >     EFI_PKEY_FILE
> >     >     > which are used for enabling embedding of the public key in
> >     the dtb,
> >     >     > and specifying the esl file name.
> >     >     >
> >     >     > Patch 3 moves efi_capsule_auth_enabled as a weak function,
> >     which can
> >     >     > be used as a default mechanism for checking if capsule
> >     authentication
> >     >     > has been enabled.
> >     >     >
> >     >     > Patch 4 adds a default weak function for retrieving the
> >     public key
> >     >     > from the platform's dtb.
> >     >     >
> >     >     > Patch 5 adds the functionality to embed the esl file into the
> >     >     > platform's dtb during the platform build.
> >     >     >
> >     >     > I have tested this functionality on the STM32MP157C DK2
> board.
> >     >     >
> >     >     > [1] -
> >     >     https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
> >     <https://lists.denx.de/pipermail/u-boot/2021-March/442867.html>
> >     >     <https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
> >     <https://lists.denx.de/pipermail/u-boot/2021-March/442867.html>>
> >     >     >
> >     >     > Sughosh Ganu (5):
> >     >     >   efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
> >     >     >     authentication is enabled
> >     >     >   efi_loader: Kconfig: Add symbols for embedding the public
> key
> >     >     into the
> >     >     >     platform's dtb
> >     >     >   efi_capsule: Add a weak function to check whether capsule
> >     >     >     authentication is enabled
> >     >     >   efi_capsule: Add a weak function to get the public key
> >     needed for
> >     >     >     capsule authentication
> >     >     >   Makefile: Add provision for embedding public key in
> >     platform's dtb
> >     >     >
> >     >     >  Makefile                              | 10 ++++++
> >     >     >  board/emulation/common/qemu_capsule.c |  6 ----
> >     >     >  lib/efi_loader/Kconfig                | 16 ++++++++++
> >     >     >  lib/efi_loader/efi_capsule.c          | 44
> >     >     ++++++++++++++++++++++++---
> >     >     >  4 files changed, 66 insertions(+), 10 deletions(-)
> >     >     >
> >     >     > --
> >     >     > 2.17.1
> >     >     >
> >     >
> >     >     We need to rethink the use of weak functions for this sort of
> >     thing,
> >     >     or we will end up with an unnavigable mess at some point. If
> >     we need
> >     >     to adjust the flow of boot, let's adjust the flow rather than
> >     adding
> >     >     hooks everywhere.
> >     >
> >     >
> >     > There are two weak functions being added. One is for retrieving the
> >     > public key to be used for the capsule authentication, and the
> other is
> >     > for checking for whether capsule authentication has been enabled.
> The
> >     > reason why a weak function is needed is because platforms can have
> >     other
> >     > mechanisms for retrieval of the public key or for testing if
> capsule
> >     > authentication has been enabled.
> >     >
> >     > If we consider the case of public key retrieval, the majority of
> >     > platforms would be built with the device tree concatenated with the
> >     > u-boot binary. The weak function would cater to all of those
> platforms
> >     > -- having a weak function would mean that we are not required to
> >     repeat
> >     > the same functionality for every platform that uses the same
> mechanism
> >     > for extracting the public key. However, there would be platforms
> where
> >     > the public key is obtained through a different mechanism which is
> >     > platform specific. Those platforms would have to define their own
> >     > function to get the public key. Same for checking whether capsule
> >     > authentication feature has been enabled or not.
> >     >
> >     > -sughosh
> >
> >     Hello Sughosh,
> >
> >     Could you, please, explain why there could be a need to use public
> keys
> >     for capsule verification that are not compiled into U-Boot. I cannot
> see
> >     how this would increase security.
> >
> >
> > With the changes that have been made in the Makefile(patch 5/5), the
> > public key is now embedded into the platform's dtb, and subsequently
> > this dtb is concatenated with the u-boot binary to create a single
> > u-boot.bin image. This image can then be verified during the trusted
> > boot flow to check against any kind of tampering. This takes care of
> > your concern of not having the public key separately on the disk, which
> > makes it open to tampering, with the public key now embedded as part of
> > the u-boot image. You had suggested embedding the public key as part of
> > the u-boot image. I have embedded it within the platform's dtb which is
> > part of the u-boot image. This becomes a generic solution which is
> > platform and architecture agnostic. I believe concatenating the
> > platform's dtb with the u-boot binary is the standard flow for
> > production images.
>
> Embedding the key in the device-tree is fine. I am just trying to
> understand why you need the extensibility via a weak function.
>

This is to provide flexibility for any platform that might have a different
mechanism of passing/retrieving the public key. Some platforms might have
their dtb passed from an earlier stage firmware, like tf-a. Or there could
be a read-only device like a fuse which houses the keys to be used. Having
a weak default would allow such platforms to implement a platform specific
function to retrieve the public key. So if there is no technical
disadvantage of having a weak default, I think keeping this flexibility for
platforms would be good.


>
> >
> >     I cannot imagine any scenario where you would want to allow switching
> >     off capsule authentication if it has been built into U-Boot.
> >
> >
> > This is only an additional knob for any user who might want to perform a
> > capsule update without authentication -- with this additional knob, the
> > user can use the same image for updating a capsule which does not have
> > an authentication header. The user would not be required to recompile
> > the image to turn off CONFIG_EFI_CAPSULE_AUTHENTICATE config option. But
> > if you don't see this necessary, i can remove this additional check. In
> > that case, the capsule will be authenticated when
> > CONFIG_EFI_CAPSULE_AUTHENTICATE is enabled.
>
> I would prefer to reduce the number of "knobs" that you have to check
> when rolling out secure firmware.
>

Okay. I will remove this extra check in the next version. Whether the
platform authenticates the capsule or not would then depend solely on the
config option.

-sughosh

[PATCH 3/5] efi_capsule: Add a weak function to check whether capsule authentication is enabled

[Heinrich Schuchardt]

On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> Define a weak function which checks if the environment variable
> capsule_authentication_enabled has been set, for enabling capsule
> authentication. Other platforms might have a different mechanism to
> determine this, and would then define their own platform specific
> function.

I cannot yet understand the concrete need for such a function.

Is there any Linaro customer who actually needs it? What is the use case?

Best regards

Heinrich

>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   board/emulation/common/qemu_capsule.c | 6 ------
>   lib/efi_loader/efi_capsule.c          | 6 ++++++
>   2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c
> index 5cb461d52b..6b8a87022a 100644
> --- a/board/emulation/common/qemu_capsule.c
> +++ b/board/emulation/common/qemu_capsule.c
> @@ -41,9 +41,3 @@ int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
>
>   	return 0;
>   }
> -
> -bool efi_capsule_auth_enabled(void)
> -{
> -	return env_get("capsule_authentication_enabled") != NULL ?
> -		true : false;
> -}
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 0cfff0daf7..1423b675c8 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -218,6 +218,12 @@ __weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
>   	return 0;
>   }
>
> +__weak bool efi_capsule_auth_enabled(void)
> +{
> +	return env_get("capsule_authentication_enabled") ?
> +		true : false;
> +}
> +
>   efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
>   				      void **image, efi_uintn_t *image_size)
>   {
>

[PATCH 4/5] efi_capsule: Add a weak function to get the public key needed for capsule authentication

[Heinrich Schuchardt]

On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> Define a weak function which would be used in the scenario where the
> public key is stored on the platform's dtb. This dtb is concatenated
> with the u-boot binary during the build process. Platforms which have
> a different mechanism for getting the public key would define their
> own platform specific function.

Storing the public key in U-Boot's dtb is reasonable. But what is the
use case for a weak function?

Best regards

Heinrich

>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   lib/efi_loader/efi_capsule.c | 38 ++++++++++++++++++++++++++++++++----
>   1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 1423b675c8..fc5e1c0856 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -14,10 +14,13 @@
>   #include <mapmem.h>
>   #include <sort.h>
>
> +#include <asm/global_data.h>
>   #include <crypto/pkcs7.h>
>   #include <crypto/pkcs7_parser.h>
>   #include <linux/err.h>
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>   const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
>   static const efi_guid_t efi_guid_firmware_management_capsule_id =
>   		EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> @@ -210,11 +213,38 @@ const efi_guid_t efi_guid_capsule_root_cert_guid =
>
>   __weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
>   {
> -	/* The platform is supposed to provide
> -	 * a method for getting the public key
> -	 * stored in the form of efi signature
> -	 * list
> +	/*
> +	 * This is a function for retrieving the public key from the
> +	 * platform's device tree. The platform's device tree has been
> +	 * concatenated with the u-boot binary.
> +	 * If a platform has a different mechanism to get the public
> +	 * key, it can define it's own function.
>   	 */
> +	const void *fdt_blob = gd->fdt_blob;
> +	const void *blob;
> +	const char *cnode_name = "capsule-key";
> +	const char *snode_name = "signature";
> +	int sig_node;
> +	int len;
> +
> +	sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name);
> +	if (sig_node < 0) {
> +		EFI_PRINT("Unable to get signature node offset\n");
> +		return -FDT_ERR_NOTFOUND;
> +	}
> +
> +	blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len);
> +
> +	if (!blob || len < 0) {
> +		EFI_PRINT("Unable to get capsule-key value\n");
> +		*pkey = NULL;
> +		*pkey_len = 0;
> +		return -FDT_ERR_NOTFOUND;
> +	}
> +
> +	*pkey = (void *)blob;
> +	*pkey_len = len;
> +
>   	return 0;
>   }
>
>

[PATCH 5/5] Makefile: Add provision for embedding public key in platform's dtb

[Heinrich Schuchardt]

On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> Add provision for embedding the public key used for capsule
> authentication in the platform's dtb. This is done by invoking the
> mkeficapsule utility which puts the public key in the efi signature
> list(esl) format into the dtb.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   Makefile | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 193aa4d1c9..0d50c6a805 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1010,6 +1010,10 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; }
>   quiet_cmd_lzma = LZMA    $@
>   cmd_lzma = lzma -c -z -k -9 $< > $@
>
> +quiet_cmd_mkeficapsule = MKEFICAPSULE     $@
> +cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \
> +	-D $@
> +

tools/mkeficapsule --help does neither show a parameter -K nor a
parameter -D. Please, update tools/mkeficapsule.c before using these. A
man-page for mkeficapsule in doc/usage/ would be helpful.

$ tools/mkeficapsule --help
Usage: mkeficapsule [options] <output file>
Options:
         --fit <fit image>       new FIT image file
         --raw <raw image>       new raw image file
         --index <index>         update image index
         --instance <instance>   update hardware instance
         --public-key <key file> public key esl file
         --dtb <dtb file>        dtb file
         --overlay               the dtb file is an overlay
         --help                  print a help message

Best regards

Heinrich

>   cfg: u-boot.cfg
>
>   quiet_cmd_cfgcheck = CFGCHK  $2
> @@ -1104,8 +1108,14 @@ endif
>   PHONY += dtbs
>   dtbs: dts/dt.dtb
>   	@:
> +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE)$(CONFIG_EFI_PKEY_DTB_EMBED),yy)
> +dts/dt.dtb: u-boot tools
> +	$(Q)$(MAKE) $(build)=dts dtbs
> +	$(call cmd,mkeficapsule)
> +else
>   dts/dt.dtb: u-boot
>   	$(Q)$(MAKE) $(build)=dts dtbs
> +endif
>
>   quiet_cmd_copy = COPY    $@
>         cmd_copy = cp $< $@
>

[PATCH 2/5] efi_loader: Kconfig: Add symbols for embedding the public key into the platform's dtb

[Heinrich Schuchardt]

On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> Add config options EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE which are to
> be used for embedding the public key to be used for capsule
> authentication into the platform's device tree.
>
> The embedding of the public key would take place during the platform
> build process.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   lib/efi_loader/Kconfig | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 0b99d7c774..de3083a979 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -179,6 +179,21 @@ config EFI_CAPSULE_AUTHENTICATE
>   	  Select this option if you want to enable capsule
>   	  authentication
>
> +config EFI_PKEY_DTB_EMBED
> +	bool "Embed the public key in the Device Tree"
> +	default n

"default n" has no effect. So this line is superfluous.

But shouldn't this be default=y and tested via Python tests?

> +	depends on EFI_CAPSULE_AUTHENTICATE
> +	help
> +	  Select this option if the public key used for capsule
> +	  authentication is to be embedded into the platform's
> +	  device tree.
> +
> +config EFI_PKEY_FILE
> +	string "Public Key esl file to be embedded into the Device Tree"

Please, provide a default value (e.g. "eficapsule.esl") that we can rely
on in our Python tests.

Best regards

Heinrich

> +	help
> +	  Specify the absolute path of the public key esl file that is
> +	  to be embedded in the platform's device tree.
> +
>   config EFI_CAPSULE_FIRMWARE_FIT
>   	bool "FMP driver for FIT image"
>   	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
>

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Simon Glass]

Hi Sughosh,

On Thu, 8 Apr 2021 at 18:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi,
>>
>> On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>> >
>> > Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config option
>> > when capsule authentication is enabled.
>> >
>> > Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE
>> > which are used for enabling embedding of the public key in the dtb,
>> > and specifying the esl file name.
>> >
>> > Patch 3 moves efi_capsule_auth_enabled as a weak function, which can
>> > be used as a default mechanism for checking if capsule authentication
>> > has been enabled.
>> >
>> > Patch 4 adds a default weak function for retrieving the public key
>> > from the platform's dtb.
>> >
>> > Patch 5 adds the functionality to embed the esl file into the
>> > platform's dtb during the platform build.
>> >
>> > I have tested this functionality on the STM32MP157C DK2 board.
>> >
>> > [1] - https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
>> >
>> > Sughosh Ganu (5):
>> >   efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
>> >     authentication is enabled
>> >   efi_loader: Kconfig: Add symbols for embedding the public key into the
>> >     platform's dtb
>> >   efi_capsule: Add a weak function to check whether capsule
>> >     authentication is enabled
>> >   efi_capsule: Add a weak function to get the public key needed for
>> >     capsule authentication
>> >   Makefile: Add provision for embedding public key in platform's dtb
>> >
>> >  Makefile                              | 10 ++++++
>> >  board/emulation/common/qemu_capsule.c |  6 ----
>> >  lib/efi_loader/Kconfig                | 16 ++++++++++
>> >  lib/efi_loader/efi_capsule.c          | 44 ++++++++++++++++++++++++---
>> >  4 files changed, 66 insertions(+), 10 deletions(-)
>> >
>> > --
>> > 2.17.1
>> >
>>
>> We need to rethink the use of weak functions for this sort of thing,
>> or we will end up with an unnavigable mess at some point. If we need
>> to adjust the flow of boot, let's adjust the flow rather than adding
>> hooks everywhere.
>
>
> There are two weak functions being added. One is for retrieving the public key to be used for the capsule authentication, and the other is for checking for whether capsule authentication has been enabled. The reason why a weak function is needed is because platforms can have other mechanisms for retrieval of the public key or for testing if capsule authentication has been enabled.
>
> If we consider the case of public key retrieval, the majority of platforms would be built with the device tree concatenated with the u-boot binary. The weak function would cater to all of those platforms -- having a weak function would mean that we are not required to repeat the same functionality for every platform that uses the same mechanism for extracting the public key. However, there would be platforms where the public key is obtained through a different mechanism which is platform specific. Those platforms would have to define their own function to get the public key. Same for checking whether capsule authentication feature has been enabled or not.
>
> -sughosh

Yes, I get it, but if this is to be a critical feature and part of the
grand new design for verified boot using UEFI, surely we should be
building a new front door, not digging a tunnel in the bathroom :-)

We can either use drivers with driver model, or perhaps have a Kconfig
that enables calling the function (so we get a link error if not
provided). Or if there will be more than one handler, a linker_list.

Perhaps it is time to consider a 'hook' system, with a command to let
us see what hooks are active for any particular event?

Regards,
Simon

[PATCH 3/5] efi_capsule: Add a weak function to check whether capsule authentication is enabled

[Sughosh Ganu]

On Fri, 9 Apr 2021 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> > Define a weak function which checks if the environment variable
> > capsule_authentication_enabled has been set, for enabling capsule
> > authentication. Other platforms might have a different mechanism to
> > determine this, and would then define their own platform specific
> > function.
>
> I cannot yet understand the concrete need for such a function.
>
> Is there any Linaro customer who actually needs it? What is the use case?
>

This point was discussed in another mail thread[1]. I will be dropping the
additional knob, so this weak function will go away.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2021-April/446694.html


> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   board/emulation/common/qemu_capsule.c | 6 ------
> >   lib/efi_loader/efi_capsule.c          | 6 ++++++
> >   2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/board/emulation/common/qemu_capsule.c
> b/board/emulation/common/qemu_capsule.c
> > index 5cb461d52b..6b8a87022a 100644
> > --- a/board/emulation/common/qemu_capsule.c
> > +++ b/board/emulation/common/qemu_capsule.c
> > @@ -41,9 +41,3 @@ int efi_get_public_key_data(void **pkey, efi_uintn_t
> *pkey_len)
> >
> >       return 0;
> >   }
> > -
> > -bool efi_capsule_auth_enabled(void)
> > -{
> > -     return env_get("capsule_authentication_enabled") != NULL ?
> > -             true : false;
> > -}
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 0cfff0daf7..1423b675c8 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -218,6 +218,12 @@ __weak int efi_get_public_key_data(void **pkey,
> efi_uintn_t *pkey_len)
> >       return 0;
> >   }
> >
> > +__weak bool efi_capsule_auth_enabled(void)
> > +{
> > +     return env_get("capsule_authentication_enabled") ?
> > +             true : false;
> > +}
> > +
> >   efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t
> capsule_size,
> >                                     void **image, efi_uintn_t
> *image_size)
> >   {
> >
>
>

[PATCH 4/5] efi_capsule: Add a weak function to get the public key needed for capsule authentication

[Sughosh Ganu]

On Fri, 9 Apr 2021 at 01:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> > Define a weak function which would be used in the scenario where the
> > public key is stored on the platform's dtb. This dtb is concatenated
> > with the u-boot binary during the build process. Platforms which have
> > a different mechanism for getting the public key would define their
> > own platform specific function.
>
> Storing the public key in U-Boot's dtb is reasonable. But what is the
> use case for a weak function?
>

This point was discussed in another mail thread[1].

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2021-April/446694.html


> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   lib/efi_loader/efi_capsule.c | 38 ++++++++++++++++++++++++++++++++----
> >   1 file changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 1423b675c8..fc5e1c0856 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -14,10 +14,13 @@
> >   #include <mapmem.h>
> >   #include <sort.h>
> >
> > +#include <asm/global_data.h>
> >   #include <crypto/pkcs7.h>
> >   #include <crypto/pkcs7_parser.h>
> >   #include <linux/err.h>
> >
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> >   const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
> >   static const efi_guid_t efi_guid_firmware_management_capsule_id =
> >               EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > @@ -210,11 +213,38 @@ const efi_guid_t efi_guid_capsule_root_cert_guid =
> >
> >   __weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> >   {
> > -     /* The platform is supposed to provide
> > -      * a method for getting the public key
> > -      * stored in the form of efi signature
> > -      * list
> > +     /*
> > +      * This is a function for retrieving the public key from the
> > +      * platform's device tree. The platform's device tree has been
> > +      * concatenated with the u-boot binary.
> > +      * If a platform has a different mechanism to get the public
> > +      * key, it can define it's own function.
> >        */
> > +     const void *fdt_blob = gd->fdt_blob;
> > +     const void *blob;
> > +     const char *cnode_name = "capsule-key";
> > +     const char *snode_name = "signature";
> > +     int sig_node;
> > +     int len;
> > +
> > +     sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name);
> > +     if (sig_node < 0) {
> > +             EFI_PRINT("Unable to get signature node offset\n");
> > +             return -FDT_ERR_NOTFOUND;
> > +     }
> > +
> > +     blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len);
> > +
> > +     if (!blob || len < 0) {
> > +             EFI_PRINT("Unable to get capsule-key value\n");
> > +             *pkey = NULL;
> > +             *pkey_len = 0;
> > +             return -FDT_ERR_NOTFOUND;
> > +     }
> > +
> > +     *pkey = (void *)blob;
> > +     *pkey_len = len;
> > +
> >       return 0;
> >   }
> >
> >
>
>

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Sughosh Ganu]

hi Simon,

On Fri, 9 Apr 2021 at 05:26, Simon Glass <sjg@chromium.org> wrote:

> Hi Sughosh,
>
> On Thu, 8 Apr 2021 at 18:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi,
> >>
> >> On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu <sughosh.ganu@linaro.org>
> wrote:
> >> >
> >> > Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config option
> >> > when capsule authentication is enabled.
> >> >
> >> > Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE
> >> > which are used for enabling embedding of the public key in the dtb,
> >> > and specifying the esl file name.
> >> >
> >> > Patch 3 moves efi_capsule_auth_enabled as a weak function, which can
> >> > be used as a default mechanism for checking if capsule authentication
> >> > has been enabled.
> >> >
> >> > Patch 4 adds a default weak function for retrieving the public key
> >> > from the platform's dtb.
> >> >
> >> > Patch 5 adds the functionality to embed the esl file into the
> >> > platform's dtb during the platform build.
> >> >
> >> > I have tested this functionality on the STM32MP157C DK2 board.
> >> >
> >> > [1] - https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
> >> >
> >> > Sughosh Ganu (5):
> >> >   efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
> >> >     authentication is enabled
> >> >   efi_loader: Kconfig: Add symbols for embedding the public key into
> the
> >> >     platform's dtb
> >> >   efi_capsule: Add a weak function to check whether capsule
> >> >     authentication is enabled
> >> >   efi_capsule: Add a weak function to get the public key needed for
> >> >     capsule authentication
> >> >   Makefile: Add provision for embedding public key in platform's dtb
> >> >
> >> >  Makefile                              | 10 ++++++
> >> >  board/emulation/common/qemu_capsule.c |  6 ----
> >> >  lib/efi_loader/Kconfig                | 16 ++++++++++
> >> >  lib/efi_loader/efi_capsule.c          | 44
> ++++++++++++++++++++++++---
> >> >  4 files changed, 66 insertions(+), 10 deletions(-)
> >> >
> >> > --
> >> > 2.17.1
> >> >
> >>
> >> We need to rethink the use of weak functions for this sort of thing,
> >> or we will end up with an unnavigable mess at some point. If we need
> >> to adjust the flow of boot, let's adjust the flow rather than adding
> >> hooks everywhere.
> >
> >
> > There are two weak functions being added. One is for retrieving the
> public key to be used for the capsule authentication, and the other is for
> checking for whether capsule authentication has been enabled. The reason
> why a weak function is needed is because platforms can have other
> mechanisms for retrieval of the public key or for testing if capsule
> authentication has been enabled.
> >
> > If we consider the case of public key retrieval, the majority of
> platforms would be built with the device tree concatenated with the u-boot
> binary. The weak function would cater to all of those platforms -- having a
> weak function would mean that we are not required to repeat the same
> functionality for every platform that uses the same mechanism for
> extracting the public key. However, there would be platforms where the
> public key is obtained through a different mechanism which is platform
> specific. Those platforms would have to define their own function to get
> the public key. Same for checking whether capsule authentication feature
> has been enabled or not.
> >
> > -sughosh
>
> Yes, I get it, but if this is to be a critical feature and part of the
> grand new design for verified boot using UEFI, surely we should be
> building a new front door, not digging a tunnel in the bathroom :-)
>
> We can either use drivers with driver model, or perhaps have a Kconfig
> that enables calling the function (so we get a link error if not
> provided). Or if there will be more than one handler, a linker_list.
>

I will use the Kconfig symbol for selecting the function to use for
retrieving the public key. So, in the current scenario, the function would
be under the CONFIG_EFI_PKEY_DTB_EMBED symbol. This should cater to the
majority of the cases. If some platform wants to use a different method to
get the public key, it would need to define it's own function.

-sughosh


>
> Perhaps it is time to consider a 'hook' system, with a command to let
> us see what hooks are active for any particular event?
>
> Regards,
> Simon
>

[PATCH 0/5] Add support for embedding public key in platform's dtb

[Simon Glass]

Hi Sughosh,

On Fri, 9 Apr 2021 at 23:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Fri, 9 Apr 2021 at 05:26, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Sughosh,
>>
>> On Thu, 8 Apr 2021 at 18:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>> >
>> > hi Simon,
>> >
>> > On Wed, 7 Apr 2021 at 21:44, Simon Glass <sjg@chromium.org> wrote:
>> >>
>> >> Hi,
>> >>
>> >> On Wed, 7 Apr 2021 at 23:54, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>> >> >
>> >> > Patch 1 fixes an issue of selection of IMAGE_SIGN_INFO config option
>> >> > when capsule authentication is enabled.
>> >> >
>> >> > Patch 2 add two config symbols, EFI_PKEY_DTB_EMBED and EFI_PKEY_FILE
>> >> > which are used for enabling embedding of the public key in the dtb,
>> >> > and specifying the esl file name.
>> >> >
>> >> > Patch 3 moves efi_capsule_auth_enabled as a weak function, which can
>> >> > be used as a default mechanism for checking if capsule authentication
>> >> > has been enabled.
>> >> >
>> >> > Patch 4 adds a default weak function for retrieving the public key
>> >> > from the platform's dtb.
>> >> >
>> >> > Patch 5 adds the functionality to embed the esl file into the
>> >> > platform's dtb during the platform build.
>> >> >
>> >> > I have tested this functionality on the STM32MP157C DK2 board.
>> >> >
>> >> > [1] - https://lists.denx.de/pipermail/u-boot/2021-March/442867.html
>> >> >
>> >> > Sughosh Ganu (5):
>> >> >   efi_loader: Kconfig: Select IMAGE_SIGN_INFO when capsule
>> >> >     authentication is enabled
>> >> >   efi_loader: Kconfig: Add symbols for embedding the public key into the
>> >> >     platform's dtb
>> >> >   efi_capsule: Add a weak function to check whether capsule
>> >> >     authentication is enabled
>> >> >   efi_capsule: Add a weak function to get the public key needed for
>> >> >     capsule authentication
>> >> >   Makefile: Add provision for embedding public key in platform's dtb
>> >> >
>> >> >  Makefile                              | 10 ++++++
>> >> >  board/emulation/common/qemu_capsule.c |  6 ----
>> >> >  lib/efi_loader/Kconfig                | 16 ++++++++++
>> >> >  lib/efi_loader/efi_capsule.c          | 44 ++++++++++++++++++++++++---
>> >> >  4 files changed, 66 insertions(+), 10 deletions(-)
>> >> >
>> >> > --
>> >> > 2.17.1
>> >> >
>> >>
>> >> We need to rethink the use of weak functions for this sort of thing,
>> >> or we will end up with an unnavigable mess at some point. If we need
>> >> to adjust the flow of boot, let's adjust the flow rather than adding
>> >> hooks everywhere.
>> >
>> >
>> > There are two weak functions being added. One is for retrieving the public key to be used for the capsule authentication, and the other is for checking for whether capsule authentication has been enabled. The reason why a weak function is needed is because platforms can have other mechanisms for retrieval of the public key or for testing if capsule authentication has been enabled.
>> >
>> > If we consider the case of public key retrieval, the majority of platforms would be built with the device tree concatenated with the u-boot binary. The weak function would cater to all of those platforms -- having a weak function would mean that we are not required to repeat the same functionality for every platform that uses the same mechanism for extracting the public key. However, there would be platforms where the public key is obtained through a different mechanism which is platform specific. Those platforms would have to define their own function to get the public key. Same for checking whether capsule authentication feature has been enabled or not.
>> >
>> > -sughosh
>>
>> Yes, I get it, but if this is to be a critical feature and part of the
>> grand new design for verified boot using UEFI, surely we should be
>> building a new front door, not digging a tunnel in the bathroom :-)
>>
>> We can either use drivers with driver model, or perhaps have a Kconfig
>> that enables calling the function (so we get a link error if not
>> provided). Or if there will be more than one handler, a linker_list.
>
>
> I will use the Kconfig symbol for selecting the function to use for retrieving the public key. So, in the current scenario, the function would be under the CONFIG_EFI_PKEY_DTB_EMBED symbol. This should cater to the majority of the cases. If some platform wants to use a different method to get the public key, it would need to define it's own function.

I wonder why another method would be needed, but if it, then someone
can turn your thing into a choice, I suppose.

Regards,
Simon

[PATCH 5/5] Makefile: Add provision for embedding public key in platform's dtb

[AKASHI Takahiro]

On Thu, Apr 08, 2021 at 09:58:17PM +0200, Heinrich Schuchardt wrote:
> On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> > Add provision for embedding the public key used for capsule
> > authentication in the platform's dtb. This is done by invoking the
> > mkeficapsule utility which puts the public key in the efi signature
> > list(esl) format into the dtb.
> > 
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   Makefile | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/Makefile b/Makefile
> > index 193aa4d1c9..0d50c6a805 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1010,6 +1010,10 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; }
> >   quiet_cmd_lzma = LZMA    $@
> >   cmd_lzma = lzma -c -z -k -9 $< > $@
> > 
> > +quiet_cmd_mkeficapsule = MKEFICAPSULE     $@
> > +cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \
> > +	-D $@
> > +
> 
> tools/mkeficapsule --help does neither show a parameter -K nor a
> parameter -D.

This clearly shows that the feature with -K/-D has nothing to do with
creating a capsule file.
Two totally different things in one place (command).
And the dtb overlay operation can be achieved by using standard commands.

I believe that the feature should be removed from mkeficapsule.

-Takahiro Akashi


> Please, update tools/mkeficapsule.c before using these. A
> man-page for mkeficapsule in doc/usage/ would be helpful.
> 
> $ tools/mkeficapsule --help
> Usage: mkeficapsule [options] <output file>
> Options:
>         --fit <fit image>       new FIT image file
>         --raw <raw image>       new raw image file
>         --index <index>         update image index
>         --instance <instance>   update hardware instance
>         --public-key <key file> public key esl file
>         --dtb <dtb file>        dtb file
>         --overlay               the dtb file is an overlay
>         --help                  print a help message
> 
> Best regards
> 
> Heinrich
> 
> >   cfg: u-boot.cfg
> > 
> >   quiet_cmd_cfgcheck = CFGCHK  $2
> > @@ -1104,8 +1108,14 @@ endif
> >   PHONY += dtbs
> >   dtbs: dts/dt.dtb
> >   	@:
> > +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE)$(CONFIG_EFI_PKEY_DTB_EMBED),yy)
> > +dts/dt.dtb: u-boot tools
> > +	$(Q)$(MAKE) $(build)=dts dtbs
> > +	$(call cmd,mkeficapsule)
> > +else
> >   dts/dt.dtb: u-boot
> >   	$(Q)$(MAKE) $(build)=dts dtbs
> > +endif
> > 
> >   quiet_cmd_copy = COPY    $@
> >         cmd_copy = cp $< $@
> > 
> 

[PATCH 5/5] Makefile: Add provision for embedding public key in platform's dtb

[Masami Hiramatsu]

2021?4?28?(?) 14:44 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Thu, Apr 08, 2021 at 09:58:17PM +0200, Heinrich Schuchardt wrote:
> > On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> > > Add provision for embedding the public key used for capsule
> > > authentication in the platform's dtb. This is done by invoking the
> > > mkeficapsule utility which puts the public key in the efi signature
> > > list(esl) format into the dtb.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >   Makefile | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 193aa4d1c9..0d50c6a805 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1010,6 +1010,10 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; }
> > >   quiet_cmd_lzma = LZMA    $@
> > >   cmd_lzma = lzma -c -z -k -9 $< > $@
> > >
> > > +quiet_cmd_mkeficapsule = MKEFICAPSULE     $@
> > > +cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \
> > > +   -D $@
> > > +
> >
> > tools/mkeficapsule --help does neither show a parameter -K nor a
> > parameter -D.
>
> This clearly shows that the feature with -K/-D has nothing to do with
> creating a capsule file.
> Two totally different things in one place (command).
> And the dtb overlay operation can be achieved by using standard commands.

If I understand correctly,  we need the following steps,
1. prepare the key for signing
2. make dtb overlay from that key
3. sign the capsule with the key

And Sughosh's implementation is using mkeficapsule for 2 and 3.
Takahiro pointed that mkeficapsule is only for 3 because of its name
and avoid confusion.

Is that correct?

What would you think about changing the tool name?
E.g.

For step 2.
capsuletool dtb --public-key pubkey [--overlay] target.dtb

For step 3.
capsuletool capsule --raw u-boot.bin --index 1 --public-key pubkey u-boot.cap

Then we can expand it for inspection, verify etc.

Thank you,

>
> I believe that the feature should be removed from mkeficapsule.
>
> -Takahiro Akashi
>
>
> > Please, update tools/mkeficapsule.c before using these. A
> > man-page for mkeficapsule in doc/usage/ would be helpful.
> >
> > $ tools/mkeficapsule --help
> > Usage: mkeficapsule [options] <output file>
> > Options:
> >         --fit <fit image>       new FIT image file
> >         --raw <raw image>       new raw image file
> >         --index <index>         update image index
> >         --instance <instance>   update hardware instance
> >         --public-key <key file> public key esl file
> >         --dtb <dtb file>        dtb file
> >         --overlay               the dtb file is an overlay
> >         --help                  print a help message
> >
> > Best regards
> >
> > Heinrich
> >
> > >   cfg: u-boot.cfg
> > >
> > >   quiet_cmd_cfgcheck = CFGCHK  $2
> > > @@ -1104,8 +1108,14 @@ endif
> > >   PHONY += dtbs
> > >   dtbs: dts/dt.dtb
> > >     @:
> > > +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE)$(CONFIG_EFI_PKEY_DTB_EMBED),yy)
> > > +dts/dt.dtb: u-boot tools
> > > +   $(Q)$(MAKE) $(build)=dts dtbs
> > > +   $(call cmd,mkeficapsule)
> > > +else
> > >   dts/dt.dtb: u-boot
> > >     $(Q)$(MAKE) $(build)=dts dtbs
> > > +endif
> > >
> > >   quiet_cmd_copy = COPY    $@
> > >         cmd_copy = cp $< $@
> > >
> >



-- 
Masami Hiramatsu

[PATCH 5/5] Makefile: Add provision for embedding public key in platform's dtb

[AKASHI Takahiro]

On Wed, Apr 28, 2021 at 03:31:36PM +0900, Masami Hiramatsu wrote:
> 2021?4?28?(?) 14:44 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >
> > On Thu, Apr 08, 2021 at 09:58:17PM +0200, Heinrich Schuchardt wrote:
> > > On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> > > > Add provision for embedding the public key used for capsule
> > > > authentication in the platform's dtb. This is done by invoking the
> > > > mkeficapsule utility which puts the public key in the efi signature
> > > > list(esl) format into the dtb.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >   Makefile | 10 ++++++++++
> > > >   1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/Makefile b/Makefile
> > > > index 193aa4d1c9..0d50c6a805 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -1010,6 +1010,10 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; }
> > > >   quiet_cmd_lzma = LZMA    $@
> > > >   cmd_lzma = lzma -c -z -k -9 $< > $@
> > > >
> > > > +quiet_cmd_mkeficapsule = MKEFICAPSULE     $@
> > > > +cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \
> > > > +   -D $@
> > > > +
> > >
> > > tools/mkeficapsule --help does neither show a parameter -K nor a
> > > parameter -D.
> >
> > This clearly shows that the feature with -K/-D has nothing to do with
> > creating a capsule file.
> > Two totally different things in one place (command).
> > And the dtb overlay operation can be achieved by using standard commands.
> 
> If I understand correctly,  we need the following steps,
> 1. prepare the key for signing
> 2. make dtb overlay from that key
> 3. sign the capsule with the key
> 
> And Sughosh's implementation is using mkeficapsule for 2 and 3.
> Takahiro pointed that mkeficapsule is only for 3 because of its name
> and avoid confusion.
> 
> Is that correct?
> 
> What would you think about changing the tool name?
> E.g.
> 
> For step 2.
> capsuletool dtb --public-key pubkey [--overlay] target.dtb

My point is: as this command line shows, it has nothing to do
with a capsule file. It simply deals with dtb blob for overlaying.
(So 'capsuletool' is not appropriate.)

-Takahiro Akashi


> For step 3.
> capsuletool capsule --raw u-boot.bin --index 1 --public-key pubkey u-boot.cap
> 
> Then we can expand it for inspection, verify etc.
> 
> Thank you,
> 
> >
> > I believe that the feature should be removed from mkeficapsule.
> >
> > -Takahiro Akashi
> >
> >
> > > Please, update tools/mkeficapsule.c before using these. A
> > > man-page for mkeficapsule in doc/usage/ would be helpful.
> > >
> > > $ tools/mkeficapsule --help
> > > Usage: mkeficapsule [options] <output file>
> > > Options:
> > >         --fit <fit image>       new FIT image file
> > >         --raw <raw image>       new raw image file
> > >         --index <index>         update image index
> > >         --instance <instance>   update hardware instance
> > >         --public-key <key file> public key esl file
> > >         --dtb <dtb file>        dtb file
> > >         --overlay               the dtb file is an overlay
> > >         --help                  print a help message
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > >   cfg: u-boot.cfg
> > > >
> > > >   quiet_cmd_cfgcheck = CFGCHK  $2
> > > > @@ -1104,8 +1108,14 @@ endif
> > > >   PHONY += dtbs
> > > >   dtbs: dts/dt.dtb
> > > >     @:
> > > > +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE)$(CONFIG_EFI_PKEY_DTB_EMBED),yy)
> > > > +dts/dt.dtb: u-boot tools
> > > > +   $(Q)$(MAKE) $(build)=dts dtbs
> > > > +   $(call cmd,mkeficapsule)
> > > > +else
> > > >   dts/dt.dtb: u-boot
> > > >     $(Q)$(MAKE) $(build)=dts dtbs
> > > > +endif
> > > >
> > > >   quiet_cmd_copy = COPY    $@
> > > >         cmd_copy = cp $< $@
> > > >
> > >
> 
> 
> 
> -- 
> Masami Hiramatsu

[PATCH 5/5] Makefile: Add provision for embedding public key in platform's dtb

[Masami Hiramatsu]

Hi,

2021?5?7?(?) 17:15 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Wed, Apr 28, 2021 at 03:31:36PM +0900, Masami Hiramatsu wrote:
> > 2021?4?28?(?) 14:44 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> > >
> > > On Thu, Apr 08, 2021 at 09:58:17PM +0200, Heinrich Schuchardt wrote:
> > > > On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> > > > > Add provision for embedding the public key used for capsule
> > > > > authentication in the platform's dtb. This is done by invoking the
> > > > > mkeficapsule utility which puts the public key in the efi signature
> > > > > list(esl) format into the dtb.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >   Makefile | 10 ++++++++++
> > > > >   1 file changed, 10 insertions(+)
> > > > >
> > > > > diff --git a/Makefile b/Makefile
> > > > > index 193aa4d1c9..0d50c6a805 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -1010,6 +1010,10 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; }
> > > > >   quiet_cmd_lzma = LZMA    $@
> > > > >   cmd_lzma = lzma -c -z -k -9 $< > $@
> > > > >
> > > > > +quiet_cmd_mkeficapsule = MKEFICAPSULE     $@
> > > > > +cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \
> > > > > +   -D $@
> > > > > +
> > > >
> > > > tools/mkeficapsule --help does neither show a parameter -K nor a
> > > > parameter -D.
> > >
> > > This clearly shows that the feature with -K/-D has nothing to do with
> > > creating a capsule file.
> > > Two totally different things in one place (command).
> > > And the dtb overlay operation can be achieved by using standard commands.
> >
> > If I understand correctly,  we need the following steps,
> > 1. prepare the key for signing
> > 2. make dtb overlay from that key
> > 3. sign the capsule with the key
> >
> > And Sughosh's implementation is using mkeficapsule for 2 and 3.
> > Takahiro pointed that mkeficapsule is only for 3 because of its name
> > and avoid confusion.
> >
> > Is that correct?
> >
> > What would you think about changing the tool name?
> > E.g.
> >
> > For step 2.
> > capsuletool dtb --public-key pubkey [--overlay] target.dtb
>
> My point is: as this command line shows, it has nothing to do
> with a capsule file. It simply deals with dtb blob for overlaying.
> (So 'capsuletool' is not appropriate.)

But if the capsuletool provide the devicetree template for the capsule
something like test/py/tests/test_efi_capsule/pubkey.dts, we can say
it is related to the capsule, because the dts is obviously for capsule.
What would you think?

Thank you,


>
> -Takahiro Akashi
>
>
> > For step 3.
> > capsuletool capsule --raw u-boot.bin --index 1 --public-key pubkey u-boot.cap
> >
> > Then we can expand it for inspection, verify etc.
> >
> > Thank you,
> >
> > >
> > > I believe that the feature should be removed from mkeficapsule.
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > Please, update tools/mkeficapsule.c before using these. A
> > > > man-page for mkeficapsule in doc/usage/ would be helpful.
> > > >
> > > > $ tools/mkeficapsule --help
> > > > Usage: mkeficapsule [options] <output file>
> > > > Options:
> > > >         --fit <fit image>       new FIT image file
> > > >         --raw <raw image>       new raw image file
> > > >         --index <index>         update image index
> > > >         --instance <instance>   update hardware instance
> > > >         --public-key <key file> public key esl file
> > > >         --dtb <dtb file>        dtb file
> > > >         --overlay               the dtb file is an overlay
> > > >         --help                  print a help message
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > > >
> > > > >   cfg: u-boot.cfg
> > > > >
> > > > >   quiet_cmd_cfgcheck = CFGCHK  $2
> > > > > @@ -1104,8 +1108,14 @@ endif
> > > > >   PHONY += dtbs
> > > > >   dtbs: dts/dt.dtb
> > > > >     @:
> > > > > +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE)$(CONFIG_EFI_PKEY_DTB_EMBED),yy)
> > > > > +dts/dt.dtb: u-boot tools
> > > > > +   $(Q)$(MAKE) $(build)=dts dtbs
> > > > > +   $(call cmd,mkeficapsule)
> > > > > +else
> > > > >   dts/dt.dtb: u-boot
> > > > >     $(Q)$(MAKE) $(build)=dts dtbs
> > > > > +endif
> > > > >
> > > > >   quiet_cmd_copy = COPY    $@
> > > > >         cmd_copy = cp $< $@
> > > > >
> > > >
> >
> >
> >
> > --
> > Masami Hiramatsu



--
Masami Hiramatsu

[PATCH 5/5] Makefile: Add provision for embedding public key in platform's dtb

[Masami Hiramatsu]

2021?5?7?(?) 18:57 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>
> Hi,
>
> 2021?5?7?(?) 17:15 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >
> > On Wed, Apr 28, 2021 at 03:31:36PM +0900, Masami Hiramatsu wrote:
> > > 2021?4?28?(?) 14:44 AKASHI Takahiro <takahiro.akashi@linaro.org>:
> > > >
> > > > On Thu, Apr 08, 2021 at 09:58:17PM +0200, Heinrich Schuchardt wrote:
> > > > > On 4/7/21 1:53 PM, Sughosh Ganu wrote:
> > > > > > Add provision for embedding the public key used for capsule
> > > > > > authentication in the platform's dtb. This is done by invoking the
> > > > > > mkeficapsule utility which puts the public key in the efi signature
> > > > > > list(esl) format into the dtb.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > >   Makefile | 10 ++++++++++
> > > > > >   1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/Makefile b/Makefile
> > > > > > index 193aa4d1c9..0d50c6a805 100644
> > > > > > --- a/Makefile
> > > > > > +++ b/Makefile
> > > > > > @@ -1010,6 +1010,10 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; }
> > > > > >   quiet_cmd_lzma = LZMA    $@
> > > > > >   cmd_lzma = lzma -c -z -k -9 $< > $@
> > > > > >
> > > > > > +quiet_cmd_mkeficapsule = MKEFICAPSULE     $@
> > > > > > +cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \
> > > > > > +   -D $@
> > > > > > +
> > > > >
> > > > > tools/mkeficapsule --help does neither show a parameter -K nor a
> > > > > parameter -D.
> > > >
> > > > This clearly shows that the feature with -K/-D has nothing to do with
> > > > creating a capsule file.
> > > > Two totally different things in one place (command).
> > > > And the dtb overlay operation can be achieved by using standard commands.
> > >
> > > If I understand correctly,  we need the following steps,
> > > 1. prepare the key for signing
> > > 2. make dtb overlay from that key
> > > 3. sign the capsule with the key
> > >
> > > And Sughosh's implementation is using mkeficapsule for 2 and 3.
> > > Takahiro pointed that mkeficapsule is only for 3 because of its name
> > > and avoid confusion.
> > >
> > > Is that correct?
> > >
> > > What would you think about changing the tool name?
> > > E.g.
> > >
> > > For step 2.
> > > capsuletool dtb --public-key pubkey [--overlay] target.dtb
> >
> > My point is: as this command line shows, it has nothing to do
> > with a capsule file. It simply deals with dtb blob for overlaying.
> > (So 'capsuletool' is not appropriate.)
>
> But if the capsuletool provide the devicetree template for the capsule
> something like test/py/tests/test_efi_capsule/pubkey.dts, we can say
> it is related to the capsule, because the dts is obviously for capsule.
> What would you think?

Ah, wait. I misunderstood. It seems that the efi_get_public_key_data() is
platform dependent. Thus isn't it hard to provide a unified tool to embed
the key data into the dtb because it is usable for some platform but
not usable for others?

Thank you,

-- 
Masami Hiramatsu