[RFC] efi_loader: improve firmware capsule authentication

[AKASHI Takahiro]

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

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