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

Heinrich Schuchardt
2021-04-24T04:47:07Z

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
>