* [RFC] efi_loader: improve firmware capsule authentication
@ 2021-04-23 5:47 AKASHI Takahiro
2021-04-23 6:25 ` Sughosh Ganu
2021-04-23 7:21 ` Ilias Apalodimas
0 siblings, 2 replies; 11+ messages in thread
From: AKASHI Takahiro @ 2021-04-23 5:47 UTC (permalink / raw)
To: u-boot
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).
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [RFC] efi_loader: improve firmware capsule authentication 2021-04-23 5:47 [RFC] efi_loader: improve firmware capsule authentication AKASHI Takahiro @ 2021-04-23 6:25 ` Sughosh Ganu 2021-04-23 7:00 ` AKASHI Takahiro 2021-04-23 7:21 ` Ilias Apalodimas 1 sibling, 1 reply; 11+ messages in thread From: Sughosh Ganu @ 2021-04-23 6:25 UTC (permalink / raw) To: u-boot 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. I don't see any reason why we should be adding this logic in another utility, and cannot use the mkeficapsule utility for embedding the public key in the platform's dtb. 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-04-23 6:25 ` Sughosh Ganu @ 2021-04-23 7:00 ` AKASHI Takahiro 2021-04-23 9:08 ` Sughosh Ganu 0 siblings, 1 reply; 11+ messages in thread From: AKASHI Takahiro @ 2021-04-23 7:00 UTC (permalink / raw) To: u-boot 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 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-04-23 7:00 ` AKASHI Takahiro @ 2021-04-23 9:08 ` Sughosh Ganu 2021-04-26 2:44 ` AKASHI Takahiro 0 siblings, 1 reply; 11+ messages in thread From: Sughosh Ganu @ 2021-04-23 9:08 UTC (permalink / raw) To: u-boot Takahiro, On Fri, 23 Apr 2021 at 12:30, AKASHI Takahiro <takahiro.akashi@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. > > 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. -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 > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-04-23 9:08 ` Sughosh Ganu @ 2021-04-26 2:44 ` AKASHI Takahiro 2021-05-07 4:29 ` AKASHI Takahiro 0 siblings, 1 reply; 11+ messages in thread From: AKASHI Takahiro @ 2021-04-26 2:44 UTC (permalink / raw) To: u-boot Heinrich, Do you have any comments? # not only on this issue, but also other issues that I pointed out # in the initial RFC. 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@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 > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-04-26 2:44 ` AKASHI Takahiro @ 2021-05-07 4:29 ` AKASHI Takahiro 2021-05-07 18:47 ` Heinrich Schuchardt 0 siblings, 1 reply; 11+ messages in thread From: AKASHI Takahiro @ 2021-05-07 4:29 UTC (permalink / raw) To: u-boot 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@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 > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-05-07 4:29 ` AKASHI Takahiro @ 2021-05-07 18:47 ` Heinrich Schuchardt 2021-05-10 0:31 ` AKASHI Takahiro 0 siblings, 1 reply; 11+ messages in thread From: Heinrich Schuchardt @ 2021-05-07 18:47 UTC (permalink / raw) To: u-boot 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@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 >>>>>> >>>> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-05-07 18:47 ` Heinrich Schuchardt @ 2021-05-10 0:31 ` AKASHI Takahiro 0 siblings, 0 replies; 11+ messages in thread From: AKASHI Takahiro @ 2021-05-10 0:31 UTC (permalink / raw) To: u-boot 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 <takahiro.akashi@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 > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-04-23 5:47 [RFC] efi_loader: improve firmware capsule authentication AKASHI Takahiro 2021-04-23 6:25 ` Sughosh Ganu @ 2021-04-23 7:21 ` Ilias Apalodimas 2021-04-23 7:50 ` AKASHI Takahiro 1 sibling, 1 reply; 11+ messages in thread From: Ilias Apalodimas @ 2021-04-23 7:21 UTC (permalink / raw) To: u-boot Akashi-san [...] > 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? I can have a look on that. Any idea which part is broken exactly? the efidebug command just added an option for 'efidebug boot add'. Did I forget to change any of the python scripts? Thanks /Ilias > > If I have some time in the future, I will address them. > But Sughosh can do as well. > > -Takahiro Akashi ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-04-23 7:21 ` Ilias Apalodimas @ 2021-04-23 7:50 ` AKASHI Takahiro 2021-04-23 8:03 ` Ilias Apalodimas 0 siblings, 1 reply; 11+ messages in thread From: AKASHI Takahiro @ 2021-04-23 7:50 UTC (permalink / raw) To: u-boot On Fri, Apr 23, 2021 at 10:21:52AM +0300, Ilias Apalodimas wrote: > Akashi-san > > [...] > > 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? > > I can have a look on that. Any idea which part is broken exactly? > the efidebug command just added an option for 'efidebug boot add'. Did I > forget to change any of the python scripts? Yes, you did :) You introduced "-s" for possible options to "efidebug boot add". So any of executions of this command in either secure boot or capsule update test fails now. -Takahiro Akashi > Thanks > /Ilias > > > > If I have some time in the future, I will address them. > > But Sughosh can do as well. > > > > -Takahiro Akashi ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] efi_loader: improve firmware capsule authentication 2021-04-23 7:50 ` AKASHI Takahiro @ 2021-04-23 8:03 ` Ilias Apalodimas 0 siblings, 0 replies; 11+ messages in thread From: Ilias Apalodimas @ 2021-04-23 8:03 UTC (permalink / raw) To: u-boot On Fri, Apr 23, 2021 at 04:50:21PM +0900, AKASHI Takahiro wrote: > On Fri, Apr 23, 2021 at 10:21:52AM +0300, Ilias Apalodimas wrote: > > Akashi-san > > > > [...] > > > 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? > > > > I can have a look on that. Any idea which part is broken exactly? > > the efidebug command just added an option for 'efidebug boot add'. Did I > > forget to change any of the python scripts? > > Yes, you did :) > Ah great my grep failed me then! > You introduced "-s" for possible options to "efidebug boot add". > So any of executions of this command in either secure boot or > capsule update test fails now. Indeed -b was added for thge boot options and -s for the optional data. You don't have to look at this, I'll reproduce it and send whatever patches are needed. Thanks! /Ilias > > -Takahiro Akashi > > > Thanks > > /Ilias > > > > > > If I have some time in the future, I will address them. > > > But Sughosh can do as well. > > > > > > -Takahiro Akashi ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-05-10 0:31 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-23 5:47 [RFC] efi_loader: improve firmware capsule authentication AKASHI Takahiro 2021-04-23 6:25 ` Sughosh Ganu 2021-04-23 7:00 ` AKASHI Takahiro 2021-04-23 9:08 ` Sughosh Ganu 2021-04-26 2:44 ` AKASHI Takahiro 2021-05-07 4:29 ` AKASHI Takahiro 2021-05-07 18:47 ` Heinrich Schuchardt 2021-05-10 0:31 ` AKASHI Takahiro 2021-04-23 7:21 ` Ilias Apalodimas 2021-04-23 7:50 ` AKASHI Takahiro 2021-04-23 8:03 ` Ilias Apalodimas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.