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

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

[Sughosh Ganu]

These patches add support for embedding the public key efi signature
list(esl) file into the platform's device tree. The current solution
for the Qemu arm64 platform has the public key as part of an overlay,
and stored on the Efi System Partition(ESP). Having the provision to
embed the public key into the platform's dtb which is then
concatenated with the u-boot binary is a better approach, recommended
by Heinrich[1].

Patch 1 removes the existing additional check for authenticating the
capsule using the env variable.

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 adds a function for retrieving the public key which has been
embedded in the platform's dtb.

Patch 4 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, and it
works as expected.

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


Changes since V1:

* As pointed out by Heinrich in the review, remove the extra check of
  the env variable 'capsule_authentication_enabled'for authenticating
  the capsule. The capsule authentication will now be done based on
  whether the corresponding config symbol is enabled.
* Provide a default name for public key file, eficapsule.esl as
  suggested by Heinrich.
* Remove the superfluous default n statement for EFI_PKEY_DTB_EMBED
* Remove the weak function, and add the functionality to retrieve the
  public key under the config symbol CONFIG_EFI_PKEY_DTB_EMBED.

Sughosh Ganu (4):
  efi_loader: capsule: Remove the check for
    capsule_authentication_enabled environment variable
  efi_loader: Kconfig: Add symbols for embedding the public key into the
    platform's dtb
  efi_capsule: Add a 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                | 15 ++++++++++
 lib/efi_loader/efi_capsule.c          | 43 +++++++++++++++++++++++----
 lib/efi_loader/efi_firmware.c         |  5 ++--
 5 files changed, 65 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] efi_loader: capsule: Remove the check for capsule_authentication_enabled environment variable

[Sughosh Ganu]

The current capsule authentication code checks if the environment
variable capsule_authentication_enabled is set, for authenticating the
capsule. This is in addition to the check for the config symbol
CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment
variable. The capsule will now be authenticated if the config symbol
is set.

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

Changes since V1:
* As pointed out by Heinrich in the review, remove the extra check of
  the env variable 'capsule_authentication_enabled'for authenticating
  the capsule. The capsule authentication will now be done based on
  whether the corresponding config symbol is enabled.

 board/emulation/common/qemu_capsule.c | 6 ------
 lib/efi_loader/efi_firmware.c         | 5 ++---
 2 files changed, 2 insertions(+), 9 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_firmware.c b/lib/efi_loader/efi_firmware.c
index 7a3cca2793..a1b88dbfc2 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -190,7 +190,7 @@ static efi_status_t efi_get_dfu_info(
 				IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
 
 		/* Check if the capsule authentication is enabled */
-		if (env_get("capsule_authentication_enabled"))
+		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE))
 			image_info[0].attributes_setting |=
 				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
 
@@ -421,8 +421,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
 	/* Authenticate the capsule if authentication enabled */
-	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
-	    env_get("capsule_authentication_enabled")) {
+	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)) {
 		capsule_payload = NULL;
 		capsule_payload_size = 0;
 		status = efi_capsule_authenticate(image, image_size,
-- 
2.17.1

[PATCH v2 2/4] 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>
---

Changes since V1:
* Provide a default name for public key file, eficapsule.esl as
  suggested by Heinrich.
* Remove the superfluous default n statement for EFI_PKEY_DTB_EMBED

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

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 79b488823a..089accaaaa 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"
+	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"
+	default "eficapsule.esl"
+	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 v2 3/4] efi_capsule: Add a function to get the public key needed for capsule authentication

[Sughosh Ganu]

Define a 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 under a different Kconfig symbol.

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

Changes since V1:
* Remove the weak function, and add the functionality to retrieve the
  public key under the config symbol CONFIG_EFI_PKEY_DTB_EMBED.

 lib/efi_loader/efi_capsule.c | 43 +++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 2cc8f2dee0..d95e9377fe 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;
@@ -208,15 +211,45 @@ skip:
 const efi_guid_t efi_guid_capsule_root_cert_guid =
 	EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
 
-__weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
+#if defined(CONFIG_EFI_PKEY_DTB_EMBED)
+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 kconfig symbol and define a
+	 * function to retrieve the public key
 	 */
+	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;
 }
+#endif /* CONFIG_EFI_PKEY_DTB_EMBED */
 
 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 v2 4/4] 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>
---

Changes since V1: None

 Makefile | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Makefile b/Makefile
index b72d8d20c0..ebd4a6477c 100644
--- a/Makefile
+++ b/Makefile
@@ -1011,6 +1011,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
@@ -1161,8 +1165,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 v2 3/4] efi_capsule: Add a function to get the public key needed for capsule authentication

[Simon Glass]

On Mon, 12 Apr 2021 at 16:06, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Define a 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 under a different Kconfig symbol.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V1:
> * Remove the weak function, and add the functionality to retrieve the
>   public key under the config symbol CONFIG_EFI_PKEY_DTB_EMBED.
>
>  lib/efi_loader/efi_capsule.c | 43 +++++++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 5 deletions(-)

Is this defined in a header file somewhere?

>
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 2cc8f2dee0..d95e9377fe 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;
> @@ -208,15 +211,45 @@ skip:
>  const efi_guid_t efi_guid_capsule_root_cert_guid =
>         EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
>
> -__weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> +#if defined(CONFIG_EFI_PKEY_DTB_EMBED)

Can you drop the #ifdef ?

> +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)

pkey should really have a time...what is it?

>  {
> -       /* 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 kconfig symbol and define a
> +        * function to retrieve the public key
>          */
> +       const void *fdt_blob = gd->fdt_blob;
> +       const void *blob;

prop? val? It is not a DT blob

> +       const char *cnode_name = "capsule-key";
> +       const char *snode_name = "signature";

I believe these FIT things are defined in image.h

> +       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;
> +       }

Can you use the livetree API?

> +
> +       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;
>  }
> +#endif /* CONFIG_EFI_PKEY_DTB_EMBED */
>
>  efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
>                                       void **image, efi_uintn_t *image_size)
> --
> 2.17.1
>

Regards,
Simon

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

[Sughosh Ganu]

On Thu, 15 Apr 2021 at 01:08, Simon Glass <sjg@chromium.org> wrote:

> On Mon, 12 Apr 2021 at 16:06, Sughosh Ganu <sughosh.ganu@linaro.org>
> wrote:
> >
> > Define a 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 under a different Kconfig symbol.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V1:
> > * Remove the weak function, and add the functionality to retrieve the
> >   public key under the config symbol CONFIG_EFI_PKEY_DTB_EMBED.
> >
> >  lib/efi_loader/efi_capsule.c | 43 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 38 insertions(+), 5 deletions(-)
>
> Is this defined in a header file somewhere?
>

No, I will declare the function in a header. Will do so in the next version.


>
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 2cc8f2dee0..d95e9377fe 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;
> > @@ -208,15 +211,45 @@ skip:
> >  const efi_guid_t efi_guid_capsule_root_cert_guid =
> >         EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> >
> > -__weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> > +#if defined(CONFIG_EFI_PKEY_DTB_EMBED)
>
> Can you drop the #ifdef ?
>

It will be good to keep the ifdef. This way, if some other platform wants
to define a function for getting the public key using a different, platform
specific method(for e.g. getting the keys from some read-only device like a
fuse), it will be possible to do so. Without the ifdef, this becomes the
only way to get the public key.


> > +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
>
> pkey should really have a time...what is it?
>

This is really a public key certificate in an efi signature list(esl)
format. The parsing of the certificate and it's use for capsule
authentication is done by the same set of functions which perform image
authentication for the  uefi secure boot feature.


> >  {
> > -       /* 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 kconfig symbol and define a
> > +        * function to retrieve the public key
> >          */
> > +       const void *fdt_blob = gd->fdt_blob;
> > +       const void *blob;
>
> prop? val? It is not a DT blob
>

Okay.


>
> > +       const char *cnode_name = "capsule-key";
> > +       const char *snode_name = "signature";
>
> I believe these FIT things are defined in image.h
>

These are based on the node names that are populated by the mkeficapsule
utility. If you don't have a strong opinion on this, I would like to keep
them as is. I can define macros for them.


>
> > +       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;
> > +       }
>
> Can you use the livetree API?
>

Can you please point me to the specific API that you are referring to.
Thanks.

-sughosh


>
> > +
> > +       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;
> >  }
> > +#endif /* CONFIG_EFI_PKEY_DTB_EMBED */
> >
> >  efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t
> capsule_size,
> >                                       void **image, efi_uintn_t
> *image_size)
> > --
> > 2.17.1
> >
>
> Regards,
> Simon
>

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

[Heinrich Schuchardt]

On 4/15/21 12:25 PM, Sughosh Ganu wrote:
>
> On Thu, 15 Apr 2021 at 01:08, Simon Glass <sjg@chromium.org
> <mailto:sjg@chromium.org>> wrote:
>
>     On Mon, 12 Apr 2021 at 16:06, Sughosh Ganu <sughosh.ganu@linaro.org
>     <mailto:sughosh.ganu@linaro.org>> wrote:
>      >
>      > Define a 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 under a different Kconfig symbol.
>      >
>      > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>     <mailto:sughosh.ganu@linaro.org>>
>      > ---
>      >
>      > Changes since V1:
>      > * Remove the weak function, and add the functionality to retrieve the
>      >? ?public key under the config symbol CONFIG_EFI_PKEY_DTB_EMBED.
>      >
>      >? lib/efi_loader/efi_capsule.c | 43
>     +++++++++++++++++++++++++++++++-----
>      >? 1 file changed, 38 insertions(+), 5 deletions(-)
>
>     Is this defined in a header file somewhere?
>
>
> No, I will declare the function in a header. Will do so in the next version.
>
>
>      >
>      > diff --git a/lib/efi_loader/efi_capsule.c
>     b/lib/efi_loader/efi_capsule.c
>      > index 2cc8f2dee0..d95e9377fe 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;
>      > @@ -208,15 +211,45 @@ skip:
>      >? const efi_guid_t efi_guid_capsule_root_cert_guid =
>      >? ? ? ? ?EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
>      >
>      > -__weak int efi_get_public_key_data(void **pkey, efi_uintn_t
>     *pkey_len)
>      > +#if defined(CONFIG_EFI_PKEY_DTB_EMBED)
>
>     Can you drop the #ifdef ?
>
>
> It will be good to keep the ifdef. This way, if some other platform
> wants to define a function for getting the public key using a different,
> platform specific method(for e.g. getting the keys from some read-only
> device like a fuse), it will be possible to do so. Without the ifdef,
> this becomes the only way to get the public key.

We prefer 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' if
possible. See scripts/checkpatch.pl. This allows the compiler to check
the syntax of all code. With gcc -Os or -O2 the code on the dead branch
will be eliminated from the binary. In some cases variables or static
function will have to marked as __maybe_unused to follow this concept.

Best regards

Heinrich

>
>
>      > +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
>
>     pkey should really have a time...what is it?
>
>
> This is really a public key certificate in an efi signature list(esl)
> format. The parsing of the certificate and it's use for capsule
> authentication is done by the same set of functions which perform image
> authentication for the??uefi secure boot feature.
>
>
>      >? {
>      > -? ? ? ?/* 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 kconfig symbol and define a
>      > +? ? ? ? * function to retrieve the public key
>      >? ? ? ? ? */
>      > +? ? ? ?const void *fdt_blob = gd->fdt_blob;
>      > +? ? ? ?const void *blob;
>
>     prop? val? It is not a DT blob
>
>
> Okay.
>
>
>      > +? ? ? ?const char *cnode_name = "capsule-key";
>      > +? ? ? ?const char *snode_name = "signature";
>
>     I believe these FIT things are defined in image.h
>
>
> These are based on the node names that are populated by the mkeficapsule
> utility. If you don't have a strong opinion on this, I would like to
> keep them as is. I can define macros for them.
>
>
>      > +? ? ? ?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;
>      > +? ? ? ?}
>
>     Can you use the livetree API?
>
>
> Can you please point me to the specific API that you are referring to.
> Thanks.
>
> -sughosh
>
>
>      > +
>      > +? ? ? ?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;
>      >? }
>      > +#endif /* CONFIG_EFI_PKEY_DTB_EMBED */
>      >
>      >? efi_status_t efi_capsule_authenticate(const void *capsule,
>     efi_uintn_t capsule_size,
>      >? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?void **image, efi_uintn_t
>     *image_size)
>      > --
>      > 2.17.1
>      >
>
>     Regards,
>     Simon
>

[PATCH v2 1/4] efi_loader: capsule: Remove the check for capsule_authentication_enabled environment variable

[Heinrich Schuchardt]

On 4/12/21 5:05 PM, Sughosh Ganu wrote:
> The current capsule authentication code checks if the environment
> variable capsule_authentication_enabled is set, for authenticating the
> capsule. This is in addition to the check for the config symbol
> CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment
> variable. The capsule will now be authenticated if the config symbol
> is set.
>
> Signed-off-by: Sughosh Ganu<sughosh.ganu@linaro.org>

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

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

[Heinrich Schuchardt]

On 4/12/21 5:05 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>
> ---
>
> Changes since V1:
> * Provide a default name for public key file, eficapsule.esl as
>    suggested by Heinrich.
> * Remove the superfluous default n statement for EFI_PKEY_DTB_EMBED
>
>   lib/efi_loader/Kconfig | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 79b488823a..089accaaaa 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"
> +	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"
> +	default "eficapsule.esl"

This config symbol should depend on EFI_PKEY_DTB_EMBED.

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 v2 2/4] efi_loader: Kconfig: Add symbols for embedding the public key into the platform's dtb

[AKASHI Takahiro]

On Sun, Apr 25, 2021 at 09:24:39AM +0200, Heinrich Schuchardt wrote:
> On 4/12/21 5:05 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>
> > ---
> > 
> > Changes since V1:
> > * Provide a default name for public key file, eficapsule.esl as
> >    suggested by Heinrich.
> > * Remove the superfluous default n statement for EFI_PKEY_DTB_EMBED
> > 
> >   lib/efi_loader/Kconfig | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> > 
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 79b488823a..089accaaaa 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"
> > +	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"
> > +	default "eficapsule.esl"
> 
> This config symbol should depend on EFI_PKEY_DTB_EMBED.

What is embedded here is a *list* of X509 certificate, not a single public key.
"esl" stands for EFI Signature List.
The symbol name as well as help text are confusing.

-Takahiro Akashi

> 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 v2 2/4] efi_loader: Kconfig: Add symbols for embedding the public key into the platform's dtb

[AKASHI Takahiro]

On Wed, Apr 28, 2021 at 01:55:18PM +0900, AKASHI Takahiro wrote:
> On Sun, Apr 25, 2021 at 09:24:39AM +0200, Heinrich Schuchardt wrote:
> > On 4/12/21 5:05 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>
> > > ---
> > > 
> > > Changes since V1:
> > > * Provide a default name for public key file, eficapsule.esl as
> > >    suggested by Heinrich.
> > > * Remove the superfluous default n statement for EFI_PKEY_DTB_EMBED
> > > 
> > >   lib/efi_loader/Kconfig | 15 +++++++++++++++
> > >   1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index 79b488823a..089accaaaa 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"
> > > +	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"
> > > +	default "eficapsule.esl"
> > 
> > This config symbol should depend on EFI_PKEY_DTB_EMBED.
> 
> What is embedded here is a *list* of X509 certificate, not a single public key.
> "esl" stands for EFI Signature List.
> The symbol name as well as help text are confusing.

In addition, "signature" means a hash value of image
as well as X509 in UEFI terms.
So as far as we use efi_signature_verify(), any type of
"signature" will be allowed.

We must be clear here.

-Takahiro Akashi


> -Takahiro Akashi
> 
> > 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 v2 3/4] efi_capsule: Add a function to get the public key needed for capsule authentication

[AKASHI Takahiro]

Simon,

On Mon, Apr 12, 2021 at 08:35:25PM +0530, Sughosh Ganu wrote:
> Define a 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 under a different Kconfig symbol.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> 
> Changes since V1:
> * Remove the weak function, and add the functionality to retrieve the
>   public key under the config symbol CONFIG_EFI_PKEY_DTB_EMBED.
> 
>  lib/efi_loader/efi_capsule.c | 43 +++++++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 2cc8f2dee0..d95e9377fe 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;
> @@ -208,15 +211,45 @@ skip:
>  const efi_guid_t efi_guid_capsule_root_cert_guid =
>  	EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
>  
> -__weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> +#if defined(CONFIG_EFI_PKEY_DTB_EMBED)
> +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 kconfig symbol and define a
> +	 * function to retrieve the public key
>  	 */
> +	const void *fdt_blob = gd->fdt_blob;
> +	const void *blob;
> +	const char *cnode_name = "capsule-key";
> +	const char *snode_name = "signature";

# Again, "key" is not the best word to describe the value.

The syntax assumed here in a device tree (control FDT) is;
/ {
   signature {
       capsule-key = "...";
   };
   ...
};

"signature" node is already used as a directory to hold public keys
for FIT image verification (vboot).
(doc/uImage.FIT/signature.txt)

While it is unlikely that both EFI capsule authentication and
FIT image authentication are enabled at the same time, I'm a bit
concerned if the mixture of different contents might cause
some confusion.
For instance, "required-mode" which has nothing to do with UEFI capsule
may exist directly under "signagture."

Do you have any thoughts?

-Takahiro Akashi

> +	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;
>  }
> +#endif /* CONFIG_EFI_PKEY_DTB_EMBED */
>  
>  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 v2 4/4] Makefile: Add provision for embedding public key in platform's dtb

[AKASHI Takahiro]

On Mon, Apr 12, 2021 at 08:35:26PM +0530, 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>
> ---
> 
> Changes since V1: None
> 
>  Makefile | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index b72d8d20c0..ebd4a6477c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1011,6 +1011,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 $@

Instead, we can do

$ dtc -@ -I dts -O dtb -o pubkey.dtbo pubkey.dts
$ fdtoverlay -i test.dtb -o test_pubkey.dtb -v pubkey.dtbo

-Takahiro Akashi


> +
>  cfg: u-boot.cfg
>  
>  quiet_cmd_cfgcheck = CFGCHK  $2
> @@ -1161,8 +1165,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 v2 1/4] efi_loader: capsule: Remove the check for capsule_authentication_enabled environment variable

[Heinrich Schuchardt]

On 4/12/21 5:05 PM, Sughosh Ganu wrote:
> The current capsule authentication code checks if the environment
> variable capsule_authentication_enabled is set, for authenticating the
> capsule. This is in addition to the check for the config symbol
> CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment
> variable. The capsule will now be authenticated if the config symbol
> is set.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

doc/board/emulation/qemu_capsule_update.rst mentions the environment
variable. So this file needs to be updated too.

Will you provide an extra patch or update this one?

Best regards

Heinrich

> ---
>
> Changes since V1:
> * As pointed out by Heinrich in the review, remove the extra check of
>    the env variable 'capsule_authentication_enabled'for authenticating
>    the capsule. The capsule authentication will now be done based on
>    whether the corresponding config symbol is enabled.
>
>   board/emulation/common/qemu_capsule.c | 6 ------
>   lib/efi_loader/efi_firmware.c         | 5 ++---
>   2 files changed, 2 insertions(+), 9 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_firmware.c b/lib/efi_loader/efi_firmware.c
> index 7a3cca2793..a1b88dbfc2 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -190,7 +190,7 @@ static efi_status_t efi_get_dfu_info(
>   				IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
>
>   		/* Check if the capsule authentication is enabled */
> -		if (env_get("capsule_authentication_enabled"))
> +		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE))
>   			image_info[0].attributes_setting |=
>   				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
>
> @@ -421,8 +421,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>   		return EFI_EXIT(EFI_INVALID_PARAMETER);
>
>   	/* Authenticate the capsule if authentication enabled */
> -	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> -	    env_get("capsule_authentication_enabled")) {
> +	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)) {
>   		capsule_payload = NULL;
>   		capsule_payload_size = 0;
>   		status = efi_capsule_authenticate(image, image_size,
>

[PATCH v2 1/4] efi_loader: capsule: Remove the check for capsule_authentication_enabled environment variable

[AKASHI Takahiro]

On Mon, Apr 12, 2021 at 08:35:23PM +0530, Sughosh Ganu wrote:
> The current capsule authentication code checks if the environment
> variable capsule_authentication_enabled is set, for authenticating the
> capsule. This is in addition to the check for the config symbol
> CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment
> variable. The capsule will now be authenticated if the config symbol
> is set.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> 
> Changes since V1:
> * As pointed out by Heinrich in the review, remove the extra check of
>   the env variable 'capsule_authentication_enabled'for authenticating
>   the capsule. The capsule authentication will now be done based on
>   whether the corresponding config symbol is enabled.
> 
>  board/emulation/common/qemu_capsule.c | 6 ------
>  lib/efi_loader/efi_firmware.c         | 5 ++---
>  2 files changed, 2 insertions(+), 9 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_firmware.c b/lib/efi_loader/efi_firmware.c
> index 7a3cca2793..a1b88dbfc2 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -190,7 +190,7 @@ static efi_status_t efi_get_dfu_info(
>  				IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
>  
>  		/* Check if the capsule authentication is enabled */
> -		if (env_get("capsule_authentication_enabled"))
> +		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE))
>  			image_info[0].attributes_setting |=
>  				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
>  
> @@ -421,8 +421,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
>  	/* Authenticate the capsule if authentication enabled */
> -	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> -	    env_get("capsule_authentication_enabled")) {
> +	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)) {

This change is not enough;
1. When a *signed* capsule file is applied on U-Boot with
EFI_CAPSULE_AUTHENTICATE disabled, the "authenticode" data
must be excluded from the data to write.
In other words, the offset and the size in a capsule file, 
image & image_size, must also be updated before writing even
if the authentication is not performed.

Otherwise, wrong data will be stored.

2. UEFI specification requires that the authentication must be
performed only if IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED is
set on the image (image type or ESRT?).
We must always check with the attribute.

-Takahiro Akashi

>  		capsule_payload = NULL;
>  		capsule_payload_size = 0;
>  		status = efi_capsule_authenticate(image, image_size,
> -- 
> 2.17.1
> 

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

[AKASHI Takahiro]

On Mon, Apr 12, 2021 at 08:35:24PM +0530, 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>
> ---
> 
> Changes since V1:
> * Provide a default name for public key file, eficapsule.esl as
>   suggested by Heinrich.
> * Remove the superfluous default n statement for EFI_PKEY_DTB_EMBED
> 
>  lib/efi_loader/Kconfig | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 79b488823a..089accaaaa 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"
> +	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"
> +	default "eficapsule.esl"
> +	help
> +	  Specify the absolute path of the public key esl file that is

While requiring the *absolute* path, the *default* value is not.

-Takahiro Akashi


> +	  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 v2 3/4] efi_capsule: Add a function to get the public key needed for capsule authentication

[AKASHI Takahiro]

On Thu, Apr 15, 2021 at 03:55:52PM +0530, Sughosh Ganu wrote:
> On Thu, 15 Apr 2021 at 01:08, Simon Glass <sjg@chromium.org> wrote:
> 
> > On Mon, 12 Apr 2021 at 16:06, Sughosh Ganu <sughosh.ganu@linaro.org>
> > wrote:
> > >
> > > Define a 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 under a different Kconfig symbol.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > > Changes since V1:
> > > * Remove the weak function, and add the functionality to retrieve the
> > >   public key under the config symbol CONFIG_EFI_PKEY_DTB_EMBED.
> > >
> > >  lib/efi_loader/efi_capsule.c | 43 +++++++++++++++++++++++++++++++-----
> > >  1 file changed, 38 insertions(+), 5 deletions(-)
> >
> > Is this defined in a header file somewhere?
> >
> 
> No, I will declare the function in a header. Will do so in the next version.
> 
> 
> >
> > >
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 2cc8f2dee0..d95e9377fe 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;
> > > @@ -208,15 +211,45 @@ skip:
> > >  const efi_guid_t efi_guid_capsule_root_cert_guid =
> > >         EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > >
> > > -__weak int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> > > +#if defined(CONFIG_EFI_PKEY_DTB_EMBED)
> >
> > Can you drop the #ifdef ?
> >
> 
> It will be good to keep the ifdef. This way, if some other platform wants
> to define a function for getting the public key using a different, platform
> specific method(for e.g. getting the keys from some read-only device like a
> fuse), it will be possible to do so. Without the ifdef, this becomes the
> only way to get the public key.
> 
> 
> > > +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> >
> > pkey should really have a time...what is it?
> >
> 
> This is really a public key certificate in an efi signature list(esl)
> format. The parsing of the certificate and it's use for capsule
> authentication is done by the same set of functions which perform image
> authentication for the  uefi secure boot feature.
> 
> 
> > >  {
> > > -       /* 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 kconfig symbol and define a
> > > +        * function to retrieve the public key
> > >          */
> > > +       const void *fdt_blob = gd->fdt_blob;
> > > +       const void *blob;
> >
> > prop? val? It is not a DT blob
> >
> 
> Okay.
> 
> 
> >
> > > +       const char *cnode_name = "capsule-key";
> > > +       const char *snode_name = "signature";
> >
> > I believe these FIT things are defined in image.h
> >
> 
> These are based on the node names that are populated by the mkeficapsule
> utility. If you don't have a strong opinion on this, I would like to keep
> them as is. I can define macros for them.
> 
> 
> >
> > > +       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;
> > > +       }
> >
> > Can you use the livetree API?
> >
> 
> Can you please point me to the specific API that you are referring to.
> Thanks.

ofnode_*()
doc/develop/driver-model/livetree.rst

My concern here is that it is utterly unsafe to keep a public key/
certificate in a device tree if the control fdt can be changed by
users. Among others, fdt command (or CONFIG_OF_CONTROL) should be
turned off.

-Takahiro Akashi

> -sughosh
> 
> 
> >
> > > +
> > > +       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;
> > >  }
> > > +#endif /* CONFIG_EFI_PKEY_DTB_EMBED */
> > >
> > >  efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t
> > capsule_size,
> > >                                       void **image, efi_uintn_t
> > *image_size)
> > > --
> > > 2.17.1
> > >
> >
> > Regards,
> > Simon
> >