Re: [PATCH 1/3] efi_capsule: Move signature from DTB to .rodata

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

> > >

[...]

> > > +config EFI_CAPSULE_KEY_PATH
> > > +       string "Path to .esl file for capsule authentication"
> > > +       depends on EFI_CAPSULE_AUTHENTICATE
> > > +       help
> > > +         Provide the .esl file used for capsule authentication
> 
> We might be friendly if we add what "esl" means here.
> More importantly, we are able to contain more than one signatures
> if we want.

Sure, I'll replace it on the help message

> 
> > > +
> > >  config EFI_DEVICE_PATH_TO_TEXT
> > >         bool "Device path to text protocol"
> > >         default y
> > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > index fd344cea29b0..9b369430e258 100644
> > > --- a/lib/efi_loader/Makefile
> > > +++ b/lib/efi_loader/Makefile
> > > @@ -20,11 +20,19 @@ always += helloworld.efi
> > >  targets += helloworld.o
> > >  endif
> > >
> > > +ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y)
> > > +EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH))
> > > +ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","")
> > > +$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH)
> > > +endif
> > > +endif
> > > +
> > >  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> > >  obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
> > >  obj-y += efi_boottime.o
> > >  obj-y += efi_helper.o
> > >  obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> > > +obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o
> 
> We should give users another choice here to allow them to add their
> own solution for key storage.
> Or only enable this line if "CONFIG_EFI_CAPSULE_KEY_PATH" != null?
> 
> > >  obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o
> > >  obj-y += efi_console.o
> > >  obj-y += efi_device_path.o
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index b878e71438b8..50e93cad4ee5 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -16,6 +16,7 @@
> > >  #include <mapmem.h>
> > >  #include <sort.h>
> > >
> > > +#include <asm/sections.h>
> > >  #include <crypto/pkcs7.h>
> > >  #include <crypto/pkcs7_parser.h>
> > >  #include <linux/err.h>
> > > @@ -222,12 +223,23 @@ skip:
> > >  const efi_guid_t efi_guid_capsule_root_cert_guid =
> > >         EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > >
> > > +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> 
> static?

yea

> 
> > > +{
> > > +       const void *blob = __efi_capsule_sig_begin;
> > > +       const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin;
> 
> It seems that the length can be calculated at compile time.
> 

Yea but you still need the __efi_capsule_sig_begin.  What's the proposal
here? Replace __efi_capsule_sig_end with the size on the .S file?

> > > +       *pkey = (void *)blob;
> > > +       *pkey_len = len;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
> > >                                       void **image, efi_uintn_t *image_size)
> > >  {
> > >         u8 *buf;
> > >         int ret;
> > > -       void *fdt_pkey, *pkey;
> > > +       void *stored_pkey, *pkey;
> > >         efi_uintn_t pkey_len;
> > >         uint64_t monotonic_count;
> > >         struct efi_signature_store *truststore;
> > > @@ -286,7 +298,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> > >                 goto out;
> > >         }
> > >
> > > -       ret = efi_get_public_key_data(&fdt_pkey, &pkey_len);
> > > +       ret = efi_get_public_key_data(&stored_pkey, &pkey_len);
> > >         if (ret < 0)
> > >                 goto out;
> > >
> > > @@ -294,7 +306,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> > >         if (!pkey)
> > >                 goto out;
> > >
> > > -       memcpy(pkey, fdt_pkey, pkey_len);
> > > +       memcpy(pkey, stored_pkey, pkey_len);
> > >         truststore = efi_build_signature_store(pkey, pkey_len);
> > >         if (!truststore)
> > >                 goto out;
> > > diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S
> > > new file mode 100644
> > > index 000000000000..f7047a42e39d
> > > --- /dev/null
> > > +++ b/lib/efi_loader/efi_capsule_key.S
> > > @@ -0,0 +1,8 @@
> 
> Should we have "#include <config.h>" here?

Hmm maybe. Compiling didn't cause any problems, but it seems we can add
that include


> Otherwise it looks good.
> 
> -Takahiro Akashi

Thanks
/Ilias
> 
> > > +.section .rodata.capsule_key.init,"a"
> > > +.balign 16
> > > +.global __efi_capsule_sig_begin
> > > +__efi_capsule_sig_begin:
> > > +.incbin CONFIG_EFI_CAPSULE_KEY_PATH
> > > +__efi_capsule_sig_end:
> > > +.global __efi_capsule_sig_end
> > > +.balign 16
> > > --
> > > 2.32.0.rc0
> > >
> > 
> > 
> > --
> > Masami Hiramatsu