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

[Simon Glass]

Hi François,

On Mon, 25 Oct 2021 at 01:14, François Ozog <francois.ozog at linaro.org> wrote:
>
>
>
> 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.

There is no such thing, in practice. There is just one devicetree. We
need to resolve this as we are still not aligned on this, after many
months. I think in fact there is difference of opinion about the
nature of the firmware image itself. I see it is a combined whole,
where the various blobs are an unfortunate result of firmware
fragmentation and must be stitched together with binman, so they can
each see the full picture to the extent needed.

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

Yes please


- Simon

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