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

[Masami Hiramatsu <masami.hiramatsu@linaro.org>]

Hi Francois,

2021年10月25日(月) 15:28 François Ozog <francois.ozog@linaro.org>:
>
>
>
> Le lun. 25 oct. 2021 à 07:14, AKASHI Takahiro <takahiro.akashi@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@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.
>
>
> 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.

>> 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@linaro.org | Skype: ffozog
>


--
Masami Hiramatsu