From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Fri, 23 Apr 2021 16:00:36 +0900
Subject: [RFC] efi_loader: improve firmware capsule authentication
In-Reply-To: <CADg8p95tR25-uCumUbuyNrF0tk_e21Qg=P6_+xBykKcWRs2b_A@mail.gmail.com>
References: <20210423054731.GA23309@laputa>
 <CADg8p95tR25-uCumUbuyNrF0tk_e21Qg=P6_+xBykKcWRs2b_A@mail.gmail.com>
Message-ID: <20210423070036.GB23309@laputa>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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

> and cannot use the mkeficapsule
> utility for embedding the public key in the platform's dtb.

?
No need to use mkeficapsule any more.

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