[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 at chromium.org
> <mailto:sjg at chromium.org>> wrote:
>
>     On Mon, 12 Apr 2021 at 16:06, Sughosh Ganu <sughosh.ganu at linaro.org
>     <mailto:sughosh.ganu at 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 at linaro.org
>     <mailto:sughosh.ganu at 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
>