On Fri, May 07, 2021 at 08:47:28PM +0200, Heinrich Schuchardt wrote: > 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.
Why do you think so? Since this is a technical issue on the implementation, I believe it important to discuss *openly* among interested parties. We should not care who works for which company. I have never had "cross-reviews" inside Linaro for all my patches. -Takahiro Akashi > 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 > > > > > > > > > > > > >
