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

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

On Sat, 17 Jul 2021 at 14:35, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> > > >

[...]

> > > >  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?

Actually once you enable the capsule authentication compilation fails
now if a file is not provided, with a message asking for the use to
provide a valid filepath.  I'd prefer leaving it as is and once we get
a hardware that can override the embedded key, we can add an extra
Kconfig option.

> >
> > > >  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

Once we do get support for hardware stored signatures, this can be
changed to a __weak function.

>
> >
> > > > +{
> > > > +       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