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