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,

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

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

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

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

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

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