[RFC] efi_loader: improve firmware capsule authentication

[RFC] efi_loader: improve firmware capsule authentication

[AKASHI Takahiro]

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

[RFC] efi_loader: improve firmware capsule authentication

[Sughosh Ganu]

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
>

[RFC] efi_loader: improve firmware capsule authentication

[AKASHI Takahiro]

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

[RFC] efi_loader: improve firmware capsule authentication

[Ilias Apalodimas]

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

[RFC] efi_loader: improve firmware capsule authentication

[AKASHI Takahiro]

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

[RFC] efi_loader: improve firmware capsule authentication

[Ilias Apalodimas]

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

[RFC] efi_loader: improve firmware capsule authentication

[Sughosh Ganu]

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

[RFC] efi_loader: improve firmware capsule authentication

[AKASHI Takahiro]

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

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

[RFC] efi_loader: improve firmware capsule authentication

[Heinrich Schuchardt]

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

[RFC] efi_loader: improve firmware capsule authentication

[AKASHI Takahiro]

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