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 <[email protected]>
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 <
[email protected]>
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
