From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Tue, 11 May 2021 10:14:17 +0900 Subject: [PATCH v2 3/4] efi_capsule: Add a function to get the public key needed for capsule authentication In-Reply-To: References: <20210412150526.29822-1-sughosh.ganu@linaro.org> <20210412150526.29822-4-sughosh.ganu@linaro.org> Message-ID: <20210511011417.GA13929@laputa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Apr 15, 2021 at 03:55:52PM +0530, Sughosh Ganu wrote: > On Thu, 15 Apr 2021 at 01:08, Simon Glass wrote: > > > On Mon, 12 Apr 2021 at 16:06, 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 > > > --- > > > > > > 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 > > > #include > > > > > > +#include > > > #include > > > #include > > > #include > > > > > > +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 > >