On 4/15/21 12:25 PM, Sughosh Ganu wrote:
On Thu, 15 Apr 2021 at 01:08, Simon Glass <s...@chromium.org
<mailto:s...@chromium.org>> wrote:
On Mon, 12 Apr 2021 at 16:06, Sughosh Ganu <sughosh.g...@linaro.org
<mailto:sughosh.g...@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.g...@linaro.org
<mailto:sughosh.g...@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
