[RFC] efi_loader: improve firmware capsule authentication

[Heinrich Schuchardt]

On 5/7/21 6:29 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Mon, Apr 26, 2021 at 11:44:59AM +0900, AKASHI Takahiro wrote:
>> Heinrich,
>>
>> Do you have any comments?
>> # not only on this issue, but also other issues that I pointed out
>> # in the initial RFC.
>
> Ping?
>
> -Takahiro Akashi

I would prefer if you could clarify inside Linaro which direction you
want to go and cross-review your patches.

Best regards

Heinrich

>
>> On Fri, Apr 23, 2021 at 02:38:09PM +0530, Sughosh Ganu wrote:
>>> Takahiro,
>>>
>>> On Fri, 23 Apr 2021 at 12:30, AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> wrote:
>>>
>>>> Sughosh,
>>>>
>>>> On Fri, Apr 23, 2021 at 11:55:04AM +0530, Sughosh Ganu wrote:
>>>>> Takahiro,
>>>>>
>>>>> On Fri, 23 Apr 2021 at 11:17, AKASHI Takahiro <
>>>> takahiro.akashi at linaro.org>
>>>>> wrote:
>>>>>
>>>>>> Heinrich,
>>>>>>
>>>>>> I'm currently thinking of improving capsule authentication
>>>>>> that Sughosh has made, particularly around mkeficapsule command:
>>>>>>
>>>>>> 1) Add a signing feature to the command
>>>>>>     This will allow us to create a *signed* capsule file solely
>>>>>>     with mkeficapsule. We will no longer rely on EDK2's script.
>>>>>> 2) Delete "-K" and "-D" option
>>>>>>     Specifically, revert 322c813f4bec ("mkeficapsule: Add support
>>>>>>     for embedding public key in a dtb")
>>>>>>     As I said, this feature doesn't have anything to do with
>>>>>>     creating a capsule file. Above all, we can do the same thing
>>>>>>     with the existing commands (dtc and fdtoverlay).
>>>>>>
>>>>>
>>>>> I would vote against this particular revert that you are suggesting. I
>>>> have
>>>>> already submitted a patchset which is under review[1], which is adding
>>>>> support for embedding the key in the platform's dtb, using the above
>>>>> functionality in mkeficapsule.
>>>>
>>>> That is why I insisted "(2) should be done in 2021.04"
>>>> as we should stop it being merged immediately.
>>>>
>>>>> I don't see any reason why we should be
>>>>> adding this logic in another utility,
>>>>
>>>> ?
>>>> I never tried to add anything about this issue. Just remove.
>>>> FYI, we can get the exact same result with:
>>>> === pubkey.dts ===
>>>> /dts-v1/;
>>>> /plugin/;
>>>>
>>>> &{/} {
>>>>          signature {
>>>>                  capsule-key = /incbin/("CRT.esl");
>>>>          };
>>>> };
>>>> ===
>>>> $ dtc -@ -I dts -O dtb -o pubkey.dtbo pubkey.dts
>>>> $ fdtoverlay -i test.dtb -o test_pubkey.dtb -v pubkey.dtbo
>>>>
>>>> No "C" code needed here. You also re-invented the almost same function
>>>> as fdt_overlay_apply() in mkeficapsule, and yet your function is
>>>> incompatible with dtc/fdtoverlay commands in terms of overlay syntax.
>>>>
>>>> I have already confirmed the capsule file signed by my mkeficapsule
>>>> + above dtb work perfectly with efi_capsule_authenticate()
>>>> in my pytest with sandbox.
>>>>
>>>> And again, the feature has nothing to do with generating a capsule file.
>>>> It is simply to perform fdt overlay which is already supported by standard
>>>> commands.
>>>>
>>>> Those are the reasons why we should revert the patch.
>>>>
>>>
>>> I am sure that the method you have shown above would work for embedding the
>>> key into the dtb. But having the logic in mkeficapsule also does not hurt.
>>> I would say that a patch should be reverted in the scenario that it causes
>>> some regression and there is no easy or obvious fix available. This is
>>> adding some logic to a host tool, and not breaking any existing
>>> functionality. Also, this code being part of a host tool, there is no case
>>> of it causing any increase to the u-boot size. If you think that there are
>>> some bugs, or certain things can be improved in the code, I am open to
>>> making changes and fixing stuff. But I am still of the opinion that a
>>> revert in a host tool, and that too when it is not breaking any stuff is
>>> not needed.
>>
>> Instead of fixing bugs, just remove it as it is not necessary.
>> Regarding to your recent patch, use the command sequence I gave above.
>> It's enough. You don't have to maintain the code that has nothing
>> to do with capsule files.
>>
>>>
>>>>> and cannot use the mkeficapsule
>>>>> utility for embedding the public key in the platform's dtb.
>>>>
>>>> ?
>>>> No need to use mkeficapsule any more.
>>>>
>>>
>>> ? When did I say that. I said that there is no reason why mkeficapsule
>>> utility cannot be used for embedding the public key in the platform's dtb.
>>
>> My previous reply gave you lots of reasons why we should remove
>> the feature. I don't want to repeat them.
>>
>> -Takahiro Akashi
>>
>>
>>> -sughosh
>>>
>>>
>>>>
>>>> -Takahiro Akashi
>>>>
>>>>> The
>>>>> mkeficapsule utility can be extended to add the authentication
>>>> information
>>>>> that you plan to submit.
>>>>>
>>>>> -sughosh
>>>>>
>>>>> [1] - https://lists.denx.de/pipermail/u-boot/2021-April/447183.html
>>>>>
>>>>>
>>>>>> 3) Add pytest for capsule authentication with sandbox
>>>>>>
>>>>>> Now I have done all of them above although some cleanup work is
>>>>>> still needed. I think that (2) should be done in 2021.04.
>>>>>>
>>>>>> I plan to send patches for 1-3 (and maybe 5 and 7 below) if you agree.
>>>>>>
>>>>>> Other concerns:
>>>>>> 4) Documentation
>>>>>>     Currently, "doc/board/emulation/qemu_capsule_update.rst" is
>>>>>>     the only document about the usage of UEFI capsule on U-Boot.
>>>>>>     Unfortunately, it contains some errors and more importantly,
>>>>>>     most of the content are generic, not qemu-specific.
>>>>>>
>>>>>> 5) Certificate (public key) in dtb
>>>>>>     That's fine, but again "board/emulation/common/qemu_capsule.c"
>>>>>>     is naturally generic. It should be available for other platforms
>>>>>>     with a new Kconfig option.
>>>>>>
>>>>>>     # IMHO, I don't understand why the data in dtb needs be in
>>>>>>     efi-signature-list structure. A single key (cert) would be enough.
>>>>>>
>>>>>> 6) "capsule_authentication_enabled"
>>>>>>     I think that we have agreed with deleting this variable.
>>>>>>     But I don't see any patch.
>>>>>>     Moreover, capsule authentication must be enforced only
>>>>>>     if the attribute, IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED,
>>>>>>     is set. But there is no code to check the flag.
>>>>>>
>>>>>> 7) Pytest is broken
>>>>>>     Due to your and Ilias' recent patches, existing pytests for
>>>>>>     secure boot as well as capsule update are now broken.
>>>>>>     I'm not sure why you have not yet noticed.
>>>>>>     Presumably, Travis CI now skips those tests?
>>>>>>
>>>>>> If I have some time in the future, I will address them.
>>>>>> But Sughosh can do as well.
>>>>>>
>>>>>> -Takahiro Akashi
>>>>>>
>>>>