Re: [PATCH v4 03/11] efi_loader: capsule: add back efi_get_public_key_data()

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Oct 20, 2021 at 07:39:37AM -0600, Simon Glass wrote:
> Hi Masami,
> 
> On Wed, 20 Oct 2021 at 02:18, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>:
> > >
> > > Hi Takahiro,
> > >
> > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to
> > > > .rodata"") failed to revert the removal of efi_get_public_key_data().
> > > >
> > > > Add back this function and move it under lib/efi_loader so that other
> > > > platforms can utilize it. It is now declared as a weak function so that
> > > > it can be replaced with a platform-specific implementation.
> > > >
> > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to
> > > >         .rodata"")
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 36 insertions(+)
> > > >
> > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > > index b75e4bcba1a9..44f5da61a9be 100644
> > > > --- a/lib/efi_loader/efi_capsule.c
> > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > @@ -11,15 +11,20 @@
> > > >  #include <common.h>
> > > >  #include <efi_loader.h>
> > > >  #include <efi_variable.h>
> > > > +#include <env.h>
> > > > +#include <fdtdec.h>
> > > >  #include <fs.h>
> > > >  #include <malloc.h>
> > > >  #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;
> > > > @@ -251,6 +256,37 @@ out:
> > > >  }
> > > >
> > > >  #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
> > >
> > > I don't think this should be weak. What other way is there of handling
> > > this and why would it be platform-specific?
> >
> > I have a question about the current design of the capsule auth key.
> > If the platform has its own key-storage, how can the platform use the
> > platform specific storage? Does such platform load the key from the storage
> > and generate the dtb node in the platform initialization code? (or
> > device driver?)
> 
> Are we talking about a public key (which I assume from the function
> naming) or some secret key. What is an auth key?

Surely, a public key (more strictly, x509 certificate under the current
implementation)

> As I understand it, the public key should be provided by the platform
> in devicetree that U-Boot uses, by whatever prior stage has access to
> the key.

I still believe that some platform provider may want to save the key
in a *safer* storage, which should be at least read-only for non-authorized
writers. But if this issue (__weak or not) is the only blocking factor
for my entire patch series, I'm willing to drop __weak for now since
someone with production system may change it in the future when he has
a good reason for that :)

-Takahiro Akashi


> Regards,
> Simon