[U-Boot] [EXT] Re: [PATCH 4/6] spl: mmc: support loading i.MX container format file

[Marek Vasut <marek.vasut@gmail.com>]

On 6/5/19 3:18 AM, Peng Fan wrote:
>> Subject: Re: [EXT] Re: [U-Boot] [PATCH 4/6] spl: mmc: support loading i.MX
>> container format file
>>
>> On 6/4/19 5:27 AM, Peng Fan wrote:
>>>> Subject: Re: [EXT] Re: [U-Boot] [PATCH 4/6] spl: mmc: support loading
>>>> i.MX container format file
>>>>
>>>> On 5/30/19 9:06 AM, Ye Li wrote:
>>>>> On 2019/5/27 19:31, Marek Vasut wrote:
>>>>>> Caution: EXT Email
>>>>>>
>>>>>> On 5/27/19 11:49 AM, Peng Fan wrote:
>>>>>>> Hi Marek, Lukasz,
>>>>>>>
>>>>>>>> Subject: Re: [EXT] Re: [U-Boot] [PATCH 4/6] spl: mmc: support
>>>>>>>> loading i.MX container format file
>>>>>>>>
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 2019/5/22 19:41, Marek Vasut wrote:
>>>>>>>>> Caution: EXT Email
>>>>>>>>>
>>>>>>>>> On 5/22/19 9:34 AM, Lukasz Majewski wrote:
>>>>>>>>> [...]
>>>>>>>>>>>>>>>> By using above approach we do have the NXP's "container"
>>>>>>>>>>>>>>>> format only seen in the SPL (which is OK, as for example
>>>>>>>>>>>>>>>> Samsung does similar thing with FBL/BL1). When SPL is
>>>> "trused"
>>>>>>>>>>>>>>>> we may use available facilities.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The issue to me is that sc_seco_authenticate could not
>>>>>>>>>>>>>>> take a FIT image as input.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Is the sc_seco_authenticate an API accessible from SPL,
>>>>>>>>>>>>>> U-Boot proper or Linux crypro engine driver?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, it is an API accessible in SPL/U-Boot stage. I do not
>>>>>>>>>>>>> know about Linux crypto driver.
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe it would be worth to check how Linux handle this? Maybe
>>>>>>>>>>>> it would shed some more light on it?
>>>>>>>>>>>
>>>>>>>>>>> I am not familiar with that, so might be stupid question below.
>>>>>>>>>>> Does it really matter?
>>>>>>>>>>
>>>>>>>>>> I would check it just out of curiosity.
>>>>>>>>>
>>>>>>>>> Yes, it matters, because there should be such API. How would
>>>>>>>>> Linux authenticate e.g. userspace binaries if there wasn't one,
>>>>>>>>> surely not by wrapping every single object into the custom
>>>>>>>>> vendor-specific
>>>> container ?
>>>>>>>>> And if there is one, you can use it to authenticate raw binaries
>>>>>>>>> from U-Boot SPL too, e.g. fitImage blobs with an associated
>> signature.
>>>>>>>>>
>>>>>>>>
>>>>>>>> iMX8 AHAB uses RSA key pair for authentication, the on-chip thing
>>>>>>>> we called SRK is a array of public key hash which is dedicated
>>>>>>>> for AHAB. It is not a real key. The real public key is in container.
>>>>>>>> AHAB will check the public key with the on-chip SRK before using
>>>>>>>> it to authenticate the image.
>>>>>>>> Seco which contains the crypto engine on imx8 does not allow to
>>>>>>>> use the SRK by user. No such API exported.
>>>>>>>> And the fuse of SRK is locked, can't be read directly.
>>>>>>>>
>>>>>>>> Actually on imx6/imx7/imx8m, the SPL and u-boot are already using
>>>>>>>> ROM HAB to implement the trust chain, like SPL authenticates
>>>>>>>> u-boot, u-boot authenticatse kernel. We just follow this same way
>>>>>>>> on imx8, the difference is
>>>>>>>> imx8 needs container format for signed image. We prefer directly
>>>>>>>> loading container image than fit image.
>>>>>>>> If we pack fit image into container, obviously this will cause
>>>>>>>> one more
>>>> copy.
>>>>>>>> As a boot loader, isn't it better to have more image format supported?
>>>>>>
>>>>>> If the functionality of the new image format is a subset of already
>>>>>> present image format, then no, that's called a duplication.
>>>>>>
>>>>>>>> We
>>>>>>>> don't force to use container, just set it as default. Users still
>>>>>>>> can choose fit or raw image.
>>>>>>
>>>>>> They can. however they cannot authenticate the fitImage because the
>>>>>> firmware doesn't provide the necessary API for that ?
>>>>>>
>>>>>>>
>>>>>>> Do you have more comment?
>>>>>>
>>>>>> How could Linux use this iMX8 chain of trust stuff to authenticate e.g.
>>>>>> userspace binaries ? It's the same thing as authenticating blob in
>>>>>> a fitImage.
>>>>>>
>>>>>
>>>>> Userspace binaries don't use the same key pair. They are signed by
>>>>> software vendors' key. The private key for chip secure boot is only
>>>>> hold by
>>>> device manufacturer.
>>>>> For example, android needs to authenticate a signed APK. Its public
>>>>> key (Key
>>>> A) is in system FS.
>>>>> iMX trust chain only reaches to kernel + ramdisk. Then the chain
>>>>> hands over
>>>> to android.
>>>>> In ramdisk, android puts another public Key (Key B) used by
>>>>> dm-verify for system FS. So once system FS is verified ok, then the
>>>>> public key A becomes trusted. Finally we can use public key A for APK
>> authentication.
>>>>
>>>> So can we put a public key into the SPL and use it to verify a fitImage ?
>>>
>>> Technically doable. But compared with the current approach that reuse
>>> ROM public API, Using crypto driver in SPL will be complicated and
>>> code size larger without calling ROM API.
>>>
>>> I do not understand the problem the SPL loading NXP i.MX8 container
>> format.
>>> SPL should only support raw and fit format? vendor format is not
>>> allowed and not accepted?
>>
>> The problem I have is with the duplication of functionality -- the iMX8 custom
>> format does exactly the same as fitImage, except differently and with smaller
>> user base, thus with less users and reviewers and thus with less potential
>> bugfixes, which I think in crypto code is important.
> 
> The change to spl mmc common code is as below:
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index bf53a1dadf..6320af055b 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -79,6 +79,16 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
>                 load.bl_len = mmc->read_bl_len;
>                 load.read = h_spl_load_read;
>                 ret = spl_load_simple_fit(spl_image, &load, sector, header);
> +       } else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> +               struct spl_load_info load;
> +
> +               load.dev = mmc;
> +               load.priv = NULL;
> +               load.filename = NULL;
> +               load.bl_len = mmc->read_bl_len;
> +               load.read = h_spl_load_read;
> +
> +               ret = spl_load_imx_container(spl_image, &load, sector);
>         } else {
> 
> If IMX_CONTAINER is not preferred

I never implied that.

>, I'll change it to CONFIG_SPL_LOAD_VENDOR_FORMAT.
> In this way, only i.MX users need to take care container format, non i.MX users no need
> to care about that.

That would make it even worse, since if we follow this course of
development, I suspect iMX9 will have another different container format
and the list will grow on.

> It is not duplication of FIT. Container support the similar function of FIT image, but it is not
> only that.

So what is it ?

I don't think I get it. Why would I, as an iMX8 user, want to pick
custom new vendor-specific format over years-proven generic fitImage?
What is the selling point here ?

-- 
Best regards,
Marek Vasut