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

[François Ozog]

Le lun. 25 oct. 2021 à 09:05, Masami Hiramatsu <masami.hiramatsu at linaro.org>
a écrit :

> Hi Francois,
>
> 2021年10月25日(月) 15:28 François Ozog <francois.ozog at linaro.org>:
> >
> >
> >
> > Le lun. 25 oct. 2021 à 07:14, AKASHI Takahiro <
> takahiro.akashi at linaro.org> a écrit :
> >>
> >> 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 at linaro.org> wrote:
> >> > >
> >> > > Hi Simon,
> >> > >
> >> > > 2021年10月15日(金) 9:40 Simon Glass <sjg at chromium.org>:
> >> > > >
> >> > > > Hi Takahiro,
> >> > > >
> >> > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <
> takahiro.akashi at 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 at 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.
> >
> >
> > indeed. And U-Boot may not be entitled at all to check the capsule
> signature. For example updating SCP firmware with a key that is in secure
> world and will never leave this world.
>
> I think secure world firmware updates should be discussed in another
> thread, like with FWU. At this point, the capsule signature will be
> only authenticated by U-Boot, because we haven't passed the capsule
> image to the secure world yet.

i took a wrong example. The choice of authentication is to be done by the
capsule driver designer and is outside scope of U-Boot. And the key may be
in a separate storage or even the driver may invoke secure world for the
authentication (FF-A API or other platform specific). U-Boot may have a
capsule driver to update U-Boot components such as external env file. The
location of the key can be in a u-boot specific device tree.


>
> >> 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 :)
> >
> >
> > If that need be….
>
> Agreed.
>
> Thank you,
>
> >
> >>
> >>
> >> -Takahiro Akashi
> >>
> >>
> >> > Regards,
> >> > Simon
> >
> > --
> > François-Frédéric Ozog | Director Business Development
> > T: +33.67221.6485
> > francois.ozog at linaro.org | Skype: ffozog
> >
>
>
> --
> Masami Hiramatsu
>
-- 
François-Frédéric Ozog | *Director Business Development*
T: +33.67221.6485
francois.ozog at linaro.org | Skype: ffozog