| Message ID | 20210512045753.62288-3-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: capsule: improve capsule authentication support | expand |
Akashi-san, On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: > As we discussed, "-K" and "-D" options have nothing to do with > creating a capsule file. The same result can be obtained by > using standard commands like: > === signature.dts === > /dts-v1/; > /plugin/; > > &{/} { > signature { > capsule-key = /incbin/("SIGNER.esl"); > }; > }; > === > $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo > > So just remove this feature. > (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support > for embedding public key in a dtb").) > > The same feature is implemented by a shell script (tools/fdtsig.sh). The only reason I can see to keep this, is if mkeficapsule gets included intro distro packages in the future. That would make end users life a bit easier, since they would need a single binary to create the whole CapsuleUpdate sequence. Regards /Ilias
Hi Ilias, 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > Akashi-san, > > On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: > > As we discussed, "-K" and "-D" options have nothing to do with > > creating a capsule file. The same result can be obtained by > > using standard commands like: > > === signature.dts === > > /dts-v1/; > > /plugin/; > > > > &{/} { > > signature { > > capsule-key = /incbin/("SIGNER.esl"); > > }; > > }; > > === > > $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > > $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo > > > > So just remove this feature. > > (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support > > for embedding public key in a dtb").) > > > > The same feature is implemented by a shell script (tools/fdtsig.sh). > > > The only reason I can see to keep this, is if mkeficapsule gets included > intro distro packages in the future. That would make end users life a bit > easier, since they would need a single binary to create the whole > CapsuleUpdate sequence. Hmm, I think it is better to write a manpage of mkeficapsule which also describes how to embed the key into dtb as in the above example if it is so short. Or, distros can package the above shell script with mkeficapsule. Embedding a key and signing a capsule are different operations but using the same tool may confuse users (at least me). Thank you, -- Masami Hiramatsu
On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: > Hi Ilias, > > 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > > > Akashi-san, > > > > On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: > > > As we discussed, "-K" and "-D" options have nothing to do with > > > creating a capsule file. The same result can be obtained by > > > using standard commands like: > > > === signature.dts === > > > /dts-v1/; > > > /plugin/; > > > > > > &{/} { > > > signature { > > > capsule-key = /incbin/("SIGNER.esl"); > > > }; > > > }; > > > === > > > $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > > > $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo > > > > > > So just remove this feature. > > > (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support > > > for embedding public key in a dtb").) > > > > > > The same feature is implemented by a shell script (tools/fdtsig.sh). > > > > > > The only reason I can see to keep this, is if mkeficapsule gets included > > intro distro packages in the future. That would make end users life a bit > > easier, since they would need a single binary to create the whole > > CapsuleUpdate sequence. > > Hmm, I think it is better to write a manpage of mkeficapsule which > also describes > how to embed the key into dtb as in the above example if it is so short. > Or, distros can package the above shell script with mkeficapsule. > > Embedding a key and signing a capsule are different operations but > using the same tool may confuse users (at least me). Sure fair enough. I am merely pointing out we need a way to explain all of those to users. Thanks! /Ilias > > Thank you, > > -- > Masami Hiramatsu
On 12.05.21 10:01, Ilias Apalodimas wrote: > On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: >> Hi Ilias, >> >> 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: >>> >>> Akashi-san, >>> >>> On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: >>>> As we discussed, "-K" and "-D" options have nothing to do with >>>> creating a capsule file. The same result can be obtained by >>>> using standard commands like: >>>> === signature.dts === >>>> /dts-v1/; >>>> /plugin/; >>>> >>>> &{/} { >>>> signature { >>>> capsule-key = /incbin/("SIGNER.esl"); >>>> }; >>>> }; >>>> === >>>> $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts >>>> $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo >>>> >>>> So just remove this feature. >>>> (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support >>>> for embedding public key in a dtb").) >>>> >>>> The same feature is implemented by a shell script (tools/fdtsig.sh). >>> >>> >>> The only reason I can see to keep this, is if mkeficapsule gets included >>> intro distro packages in the future. That would make end users life a bit >>> easier, since they would need a single binary to create the whole >>> CapsuleUpdate sequence. >> >> Hmm, I think it is better to write a manpage of mkeficapsule which >> also describes >> how to embed the key into dtb as in the above example if it is so short. >> Or, distros can package the above shell script with mkeficapsule. >> >> Embedding a key and signing a capsule are different operations but >> using the same tool may confuse users (at least me). > > Sure fair enough. I am merely pointing out we need a way to explain all of > those to users. This is currently our only documentation: https://u-boot.readthedocs.io/en/latest/board/emulation/qemu_capsule_update.html?highlight=mkeficapsule For mkimage we have a man-page ./doc/mkimage.1 that is packaged with Debians u-boot-tools package. Please, provide a similar man-page as ./doc/mkeficapsule.1. Best regards Heinrich
On Wed, May 12, 2021 at 12:01:32PM +0200, Heinrich Schuchardt wrote: > On 12.05.21 10:01, Ilias Apalodimas wrote: > > On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: > >> Hi Ilias, > >> > >> 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >>> > >>> Akashi-san, > >>> > >>> On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: > >>>> As we discussed, "-K" and "-D" options have nothing to do with > >>>> creating a capsule file. The same result can be obtained by > >>>> using standard commands like: > >>>> === signature.dts === > >>>> /dts-v1/; > >>>> /plugin/; > >>>> > >>>> &{/} { > >>>> signature { > >>>> capsule-key = /incbin/("SIGNER.esl"); > >>>> }; > >>>> }; > >>>> === > >>>> $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > >>>> $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo > >>>> > >>>> So just remove this feature. > >>>> (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support > >>>> for embedding public key in a dtb").) > >>>> > >>>> The same feature is implemented by a shell script (tools/fdtsig.sh). > >>> > >>> > >>> The only reason I can see to keep this, is if mkeficapsule gets included > >>> intro distro packages in the future. That would make end users life a bit > >>> easier, since they would need a single binary to create the whole > >>> CapsuleUpdate sequence. > >> > >> Hmm, I think it is better to write a manpage of mkeficapsule which > >> also describes > >> how to embed the key into dtb as in the above example if it is so short. > >> Or, distros can package the above shell script with mkeficapsule. > >> > >> Embedding a key and signing a capsule are different operations but > >> using the same tool may confuse users (at least me). > > > > Sure fair enough. I am merely pointing out we need a way to explain all of > > those to users. > > This is currently our only documentation: > > https://u-boot.readthedocs.io/en/latest/board/emulation/qemu_capsule_update.html?highlight=mkeficapsule As I mentioned several times (and TODO in the cover letter), this text must be reviewed, revised and generalized as a platform-independent document. It contains a couple of errors. > For mkimage we have a man-page ./doc/mkimage.1 that is packaged with > Debians u-boot-tools package. Please, provide a similar man-page as > ./doc/mkeficapsule.1. So after all do you agree to removing "-K/-D"? Otherwise, I cannot complete the man page. -Takahiro Akashi > Best regards > > Heinrich
On 5/13/21 4:33 AM, AKASHI Takahiro wrote: > On Wed, May 12, 2021 at 12:01:32PM +0200, Heinrich Schuchardt wrote: >> On 12.05.21 10:01, Ilias Apalodimas wrote: >>> On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: >>>> Hi Ilias, >>>> >>>> 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: >>>>> >>>>> Akashi-san, >>>>> >>>>> On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: >>>>>> As we discussed, "-K" and "-D" options have nothing to do with >>>>>> creating a capsule file. The same result can be obtained by >>>>>> using standard commands like: >>>>>> === signature.dts === >>>>>> /dts-v1/; >>>>>> /plugin/; >>>>>> >>>>>> &{/} { >>>>>> signature { >>>>>> capsule-key = /incbin/("SIGNER.esl"); >>>>>> }; >>>>>> }; >>>>>> === >>>>>> $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts >>>>>> $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo >>>>>> >>>>>> So just remove this feature. >>>>>> (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support >>>>>> for embedding public key in a dtb").) >>>>>> >>>>>> The same feature is implemented by a shell script (tools/fdtsig.sh). >>>>> >>>>> >>>>> The only reason I can see to keep this, is if mkeficapsule gets included >>>>> intro distro packages in the future. That would make end users life a bit >>>>> easier, since they would need a single binary to create the whole >>>>> CapsuleUpdate sequence. >>>> >>>> Hmm, I think it is better to write a manpage of mkeficapsule which >>>> also describes >>>> how to embed the key into dtb as in the above example if it is so short. >>>> Or, distros can package the above shell script with mkeficapsule. >>>> >>>> Embedding a key and signing a capsule are different operations but >>>> using the same tool may confuse users (at least me). >>> >>> Sure fair enough. I am merely pointing out we need a way to explain all of >>> those to users. >> >> This is currently our only documentation: >> >> https://u-boot.readthedocs.io/en/latest/board/emulation/qemu_capsule_update.html?highlight=mkeficapsule > > As I mentioned several times (and TODO in the cover letter), > this text must be reviewed, revised and generalized > as a platform-independent document. > It contains a couple of errors. > >> For mkimage we have a man-page ./doc/mkimage.1 that is packaged with >> Debians u-boot-tools package. Please, provide a similar man-page as >> ./doc/mkeficapsule.1. > > So after all do you agree to removing "-K/-D"? I see no need to replicate in U-Boot what is already in the device tree compiler package. In the current workflow the fdt command is used to load the public key. This is insecure and not usable for production. The public key used to verify the capsule must be built into the U-Boot binary. This will supplant the -K and -D options. Best regards Heinrich > Otherwise, I cannot complete the man page. > > -Takahiro Akashi > >> Best regards >> >> Heinrich
On Thu, May 13, 2021 at 07:08:12AM +0200, Heinrich Schuchardt wrote: > On 5/13/21 4:33 AM, AKASHI Takahiro wrote: > > On Wed, May 12, 2021 at 12:01:32PM +0200, Heinrich Schuchardt wrote: > > > On 12.05.21 10:01, Ilias Apalodimas wrote: > > > > On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: > > > > > Hi Ilias, > > > > > > > > > > 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > > > > > > > > > > > Akashi-san, > > > > > > > > > > > > On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: > > > > > > > As we discussed, "-K" and "-D" options have nothing to do with > > > > > > > creating a capsule file. The same result can be obtained by > > > > > > > using standard commands like: > > > > > > > === signature.dts === > > > > > > > /dts-v1/; > > > > > > > /plugin/; > > > > > > > > > > > > > > &{/} { > > > > > > > signature { > > > > > > > capsule-key = /incbin/("SIGNER.esl"); > > > > > > > }; > > > > > > > }; > > > > > > > === > > > > > > > $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > > > > > > > $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo > > > > > > > > > > > > > > So just remove this feature. > > > > > > > (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support > > > > > > > for embedding public key in a dtb").) > > > > > > > > > > > > > > The same feature is implemented by a shell script (tools/fdtsig.sh). > > > > > > > > > > > > > > > > > > The only reason I can see to keep this, is if mkeficapsule gets included > > > > > > intro distro packages in the future. That would make end users life a bit > > > > > > easier, since they would need a single binary to create the whole > > > > > > CapsuleUpdate sequence. > > > > > > > > > > Hmm, I think it is better to write a manpage of mkeficapsule which > > > > > also describes > > > > > how to embed the key into dtb as in the above example if it is so short. > > > > > Or, distros can package the above shell script with mkeficapsule. > > > > > > > > > > Embedding a key and signing a capsule are different operations but > > > > > using the same tool may confuse users (at least me). > > > > > > > > Sure fair enough. I am merely pointing out we need a way to explain all of > > > > those to users. > > > > > > This is currently our only documentation: > > > > > > https://u-boot.readthedocs.io/en/latest/board/emulation/qemu_capsule_update.html?highlight=mkeficapsule > > > > As I mentioned several times (and TODO in the cover letter), > > this text must be reviewed, revised and generalized > > as a platform-independent document. > > It contains a couple of errors. > > > > > For mkimage we have a man-page ./doc/mkimage.1 that is packaged with > > > Debians u-boot-tools package. Please, provide a similar man-page as > > > ./doc/mkeficapsule.1. > > > > So after all do you agree to removing "-K/-D"? > > I see no need to replicate in U-Boot what is already in the device tree > compiler package. This is another reason that we should remove Sughosh's change. > In the current workflow the fdt command is used to load the public key. > This is insecure and not usable for production. I totally disagree. Why is using fdt command (what do you mean by fdt command, dtc/fdtoverlay?) insecure? > The public key used to verify the capsule must be built into the U-Boot > binary. This will supplant the -K and -D options. I don't get your point. You don't understand my code. Even with Sughosh's original patch, the public key (as I said, it is not a public key but a X509 certificate in ESL format) is embedded in the U-Boot's "control device tree". Even after applying my patch, this is true. Or are you insisting that the key should not be in the device tree? -Takahiro Akashi > Best regards > > Heinrich > > > Otherwise, I cannot complete the man page. > > > > -Takahiro Akashi > > > > > Best regards > > > > > > Heinrich >
On 5/13/21 9:13 AM, AKASHI Takahiro wrote: > On Thu, May 13, 2021 at 07:08:12AM +0200, Heinrich Schuchardt wrote: >> On 5/13/21 4:33 AM, AKASHI Takahiro wrote: >>> On Wed, May 12, 2021 at 12:01:32PM +0200, Heinrich Schuchardt wrote: >>>> On 12.05.21 10:01, Ilias Apalodimas wrote: >>>>> On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: >>>>>> Hi Ilias, >>>>>> >>>>>> 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: >>>>>>> >>>>>>> Akashi-san, >>>>>>> >>>>>>> On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: >>>>>>>> As we discussed, "-K" and "-D" options have nothing to do with >>>>>>>> creating a capsule file. The same result can be obtained by >>>>>>>> using standard commands like: >>>>>>>> === signature.dts === >>>>>>>> /dts-v1/; >>>>>>>> /plugin/; >>>>>>>> >>>>>>>> &{/} { >>>>>>>> signature { >>>>>>>> capsule-key = /incbin/("SIGNER.esl"); >>>>>>>> }; >>>>>>>> }; >>>>>>>> === >>>>>>>> $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts >>>>>>>> $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo >>>>>>>> >>>>>>>> So just remove this feature. >>>>>>>> (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support >>>>>>>> for embedding public key in a dtb").) >>>>>>>> >>>>>>>> The same feature is implemented by a shell script (tools/fdtsig.sh). >>>>>>> >>>>>>> >>>>>>> The only reason I can see to keep this, is if mkeficapsule gets included >>>>>>> intro distro packages in the future. That would make end users life a bit >>>>>>> easier, since they would need a single binary to create the whole >>>>>>> CapsuleUpdate sequence. >>>>>> >>>>>> Hmm, I think it is better to write a manpage of mkeficapsule which >>>>>> also describes >>>>>> how to embed the key into dtb as in the above example if it is so short. >>>>>> Or, distros can package the above shell script with mkeficapsule. >>>>>> >>>>>> Embedding a key and signing a capsule are different operations but >>>>>> using the same tool may confuse users (at least me). >>>>> >>>>> Sure fair enough. I am merely pointing out we need a way to explain all of >>>>> those to users. >>>> >>>> This is currently our only documentation: >>>> >>>> https://u-boot.readthedocs.io/en/latest/board/emulation/qemu_capsule_update.html?highlight=mkeficapsule >>> >>> As I mentioned several times (and TODO in the cover letter), >>> this text must be reviewed, revised and generalized >>> as a platform-independent document. >>> It contains a couple of errors. >>> >>>> For mkimage we have a man-page ./doc/mkimage.1 that is packaged with >>>> Debians u-boot-tools package. Please, provide a similar man-page as >>>> ./doc/mkeficapsule.1. >>> >>> So after all do you agree to removing "-K/-D"? >> >> I see no need to replicate in U-Boot what is already in the device tree >> compiler package. > > This is another reason that we should remove Sughosh's change. > >> In the current workflow the fdt command is used to load the public key. >> This is insecure and not usable for production. > > I totally disagree. > Why is using fdt command (what do you mean by fdt command, dtc/fdtoverlay?) > insecure? A user can load an insecure capsule. The fdt command is in /cmd/fdt.c and you are referring to it in board/emulation/qemu_capsule_update.rst. > >> The public key used to verify the capsule must be built into the U-Boot >> binary. This will supplant the -K and -D options. > > I don't get your point. You don't understand my code. > > Even with Sughosh's original patch, the public key (as I said, > it is not a public key but a X509 certificate in ESL format) is > embedded in the U-Boot's "control device tree". No, the ESL file it is not built into U-Boot's control device tree. A user is loading it and updating the control device tree. You shouldn't trust anything a user has loaded. You need at least the public key of the root CA built somewhere into U-Boot. The 'fdt resize' command may overwrite code. This is not what you want to do with the control device tree. If CONFIG_OF_LIVE=y, the active device tree is not at $fdtcontroladdr but in a hierarchical structure. You cannot update it via the fdt command. > > Even after applying my patch, this is true. > > Or are you insisting that the key should not be in the device tree? The public key of the root CA must not be in a place where it can be changed by a user while the device is in deployed mode. The device-tree based design is a good feasibility study but not suitable for production. Best regards Heinrich
Heinrich, You are discussing two different issues: 1. if we should remove "-K/-D" options from mkeficapsule 2. if it is safe or not to store a key in device tree It makes the discussion in this thread confusing. On Thu, May 13, 2021 at 07:42:23PM +0200, Heinrich Schuchardt wrote: > On 5/13/21 9:13 AM, AKASHI Takahiro wrote: > > On Thu, May 13, 2021 at 07:08:12AM +0200, Heinrich Schuchardt wrote: > > > On 5/13/21 4:33 AM, AKASHI Takahiro wrote: > > > > On Wed, May 12, 2021 at 12:01:32PM +0200, Heinrich Schuchardt wrote: > > > > > On 12.05.21 10:01, Ilias Apalodimas wrote: > > > > > > On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: > > > > > > > Hi Ilias, > > > > > > > > > > > > > > 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > > > > > > > > > > > > > > > Akashi-san, > > > > > > > > > > > > > > > > On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: > > > > > > > > > As we discussed, "-K" and "-D" options have nothing to do with > > > > > > > > > creating a capsule file. The same result can be obtained by > > > > > > > > > using standard commands like: > > > > > > > > > === signature.dts === > > > > > > > > > /dts-v1/; > > > > > > > > > /plugin/; > > > > > > > > > > > > > > > > > > &{/} { > > > > > > > > > signature { > > > > > > > > > capsule-key = /incbin/("SIGNER.esl"); > > > > > > > > > }; > > > > > > > > > }; > > > > > > > > > === > > > > > > > > > $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > > > > > > > > > $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo > > > > > > > > > > > > > > > > > > So just remove this feature. > > > > > > > > > (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support > > > > > > > > > for embedding public key in a dtb").) > > > > > > > > > > > > > > > > > > The same feature is implemented by a shell script (tools/fdtsig.sh). > > > > > > > > > > > > > > > > > > > > > > > > The only reason I can see to keep this, is if mkeficapsule gets included > > > > > > > > intro distro packages in the future. That would make end users life a bit > > > > > > > > easier, since they would need a single binary to create the whole > > > > > > > > CapsuleUpdate sequence. > > > > > > > > > > > > > > Hmm, I think it is better to write a manpage of mkeficapsule which > > > > > > > also describes > > > > > > > how to embed the key into dtb as in the above example if it is so short. > > > > > > > Or, distros can package the above shell script with mkeficapsule. > > > > > > > > > > > > > > Embedding a key and signing a capsule are different operations but > > > > > > > using the same tool may confuse users (at least me). > > > > > > > > > > > > Sure fair enough. I am merely pointing out we need a way to explain all of > > > > > > those to users. > > > > > > > > > > This is currently our only documentation: > > > > > > > > > > https://u-boot.readthedocs.io/en/latest/board/emulation/qemu_capsule_update.html?highlight=mkeficapsule > > > > > > > > As I mentioned several times (and TODO in the cover letter), > > > > this text must be reviewed, revised and generalized > > > > as a platform-independent document. > > > > It contains a couple of errors. > > > > > > > > > For mkimage we have a man-page ./doc/mkimage.1 that is packaged with > > > > > Debians u-boot-tools package. Please, provide a similar man-page as > > > > > ./doc/mkeficapsule.1. > > > > > > > > So after all do you agree to removing "-K/-D"? Regarding (1), you should clarify your opinion here first. > > > I see no need to replicate in U-Boot what is already in the device tree > > > compiler package. > > > > This is another reason that we should remove Sughosh's change. Hereafter, you are talking about (2). > > > In the current workflow the fdt command is used to load the public key. > > > This is insecure and not usable for production. > > > > I totally disagree. > > Why is using fdt command (what do you mean by fdt command, dtc/fdtoverlay?) > > insecure? > > A user can load an insecure capsule. > > The fdt command is in /cmd/fdt.c and you are referring to it in > board/emulation/qemu_capsule_update.rst. OK, you meant U-Boot's fdt command. > > > > > The public key used to verify the capsule must be built into the U-Boot > > > binary. This will supplant the -K and -D options. > > > > I don't get your point. You don't understand my code. > > > > Even with Sughosh's original patch, the public key (as I said, > > it is not a public key but a X509 certificate in ESL format) is > > embedded in the U-Boot's "control device tree". > > No, the ESL file it is not built into U-Boot's control device tree. What I meant by "control device tree" is the feature provided by OF_CONTROL+OF_EMBED with Sughosh's patch[1] which is also a prerequisite for my patch series. !config OF_EMBED ! bool "Embedded DTB for DT control" ! help ! If this option is enabled, the device tree will be picked up and ! built into the U-Boot image. > A user is loading it and updating the control device tree. > > You shouldn't trust anything a user has loaded. You need at least the > public key of the root CA built somewhere into U-Boot. > > The 'fdt resize' command may overwrite code. This is not what you want > to do with the control device tree. > > If CONFIG_OF_LIVE=y, the active device tree is not at $fdtcontroladdr > but in a hierarchical structure. You cannot update it via the fdt command. > > > > > Even after applying my patch, this is true. > > > > Or are you insisting that the key should not be in the device tree? > > The public key of the root CA must not be in a place where it can be > changed by a user while the device is in deployed mode. UEFI secure boot and capsule update are totally independent concepts as we discussed a long time ago. The notion of "deployed mode" is only valid for secure boot. > The device-tree based design is a good feasibility study but not > suitable for production. Nevertheless, I agree, even I have already mentioned the similar concern in [2], additionally saying we should turn off "fdt" command. [1] https://lists.denx.de/pipermail/u-boot/2021-April/447183.html [2] (oops, my message seems to have been lost.) -Takahiro Akashi > Best regards > > Heinrich
Hi Heinrich, 2021年5月14日(金) 2:42 Heinrich Schuchardt <xypron.glpk@gmx.de>: > > On 5/13/21 9:13 AM, AKASHI Takahiro wrote: > > On Thu, May 13, 2021 at 07:08:12AM +0200, Heinrich Schuchardt wrote: > >> On 5/13/21 4:33 AM, AKASHI Takahiro wrote: > >>> On Wed, May 12, 2021 at 12:01:32PM +0200, Heinrich Schuchardt wrote: > >>>> On 12.05.21 10:01, Ilias Apalodimas wrote: > >>>>> On Wed, May 12, 2021 at 04:49:02PM +0900, Masami Hiramatsu wrote: > >>>>>> Hi Ilias, > >>>>>> > >>>>>> 2021年5月12日(水) 16:21 Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >>>>>>> > >>>>>>> Akashi-san, > >>>>>>> > >>>>>>> On Wed, May 12, 2021 at 01:57:51PM +0900, AKASHI Takahiro wrote: > >>>>>>>> As we discussed, "-K" and "-D" options have nothing to do with > >>>>>>>> creating a capsule file. The same result can be obtained by > >>>>>>>> using standard commands like: > >>>>>>>> === signature.dts === > >>>>>>>> /dts-v1/; > >>>>>>>> /plugin/; > >>>>>>>> > >>>>>>>> &{/} { > >>>>>>>> signature { > >>>>>>>> capsule-key = /incbin/("SIGNER.esl"); > >>>>>>>> }; > >>>>>>>> }; > >>>>>>>> === > >>>>>>>> $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts > >>>>>>>> $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo > >>>>>>>> > >>>>>>>> So just remove this feature. > >>>>>>>> (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support > >>>>>>>> for embedding public key in a dtb").) > >>>>>>>> > >>>>>>>> The same feature is implemented by a shell script (tools/fdtsig.sh). > >>>>>>> > >>>>>>> > >>>>>>> The only reason I can see to keep this, is if mkeficapsule gets included > >>>>>>> intro distro packages in the future. That would make end users life a bit > >>>>>>> easier, since they would need a single binary to create the whole > >>>>>>> CapsuleUpdate sequence. > >>>>>> > >>>>>> Hmm, I think it is better to write a manpage of mkeficapsule which > >>>>>> also describes > >>>>>> how to embed the key into dtb as in the above example if it is so short. > >>>>>> Or, distros can package the above shell script with mkeficapsule. > >>>>>> > >>>>>> Embedding a key and signing a capsule are different operations but > >>>>>> using the same tool may confuse users (at least me). > >>>>> > >>>>> Sure fair enough. I am merely pointing out we need a way to explain all of > >>>>> those to users. > >>>> > >>>> This is currently our only documentation: > >>>> > >>>> https://u-boot.readthedocs.io/en/latest/board/emulation/qemu_capsule_update.html?highlight=mkeficapsule > >>> > >>> As I mentioned several times (and TODO in the cover letter), > >>> this text must be reviewed, revised and generalized > >>> as a platform-independent document. > >>> It contains a couple of errors. > >>> > >>>> For mkimage we have a man-page ./doc/mkimage.1 that is packaged with > >>>> Debians u-boot-tools package. Please, provide a similar man-page as > >>>> ./doc/mkeficapsule.1. > >>> > >>> So after all do you agree to removing "-K/-D"? > >> > >> I see no need to replicate in U-Boot what is already in the device tree > >> compiler package. > > > > This is another reason that we should remove Sughosh's change. > > > >> In the current workflow the fdt command is used to load the public key. > >> This is insecure and not usable for production. > > > > I totally disagree. > > Why is using fdt command (what do you mean by fdt command, dtc/fdtoverlay?) > > insecure? > > A user can load an insecure capsule. > > The fdt command is in /cmd/fdt.c and you are referring to it in > board/emulation/qemu_capsule_update.rst. Hmm, this seems like a testing manual. > > > > >> The public key used to verify the capsule must be built into the U-Boot > >> binary. This will supplant the -K and -D options. > > > > I don't get your point. You don't understand my code. > > > > Even with Sughosh's original patch, the public key (as I said, > > it is not a public key but a X509 certificate in ESL format) is > > embedded in the U-Boot's "control device tree". > > No, the ESL file it is not built into U-Boot's control device tree. > > A user is loading it and updating the control device tree. In my case (I've tested it on the DeveloperBox), the key is embedded in the U-Boot's control device tree. > > You shouldn't trust anything a user has loaded. You need at least the > public key of the root CA built somewhere into U-Boot. However, I agreed this point. If we use the public key in the device tree, it must be loaded as a part of U-Boot itself, and must not overwritten by the user given fdt. > > The 'fdt resize' command may overwrite code. This is not what you want > to do with the control device tree. > > If CONFIG_OF_LIVE=y, the active device tree is not at $fdtcontroladdr > but in a hierarchical structure. You cannot update it via the fdt command. > > > > > Even after applying my patch, this is true. > > > > Or are you insisting that the key should not be in the device tree? > > The public key of the root CA must not be in a place where it can be > changed by a user while the device is in deployed mode. > > The device-tree based design is a good feasibility study but not > suitable for production. Therefore, there is no reason mkeficapsule keeps -K/-D anymore, because embedding public key will be done in u-boot.bin build process (or embedded in the hardware via platform dependent interface.) Thank you, -- Masami Hiramatsu
diff --git a/Makefile b/Makefile index 63b1f0143220..9806464357e0 100644 --- a/Makefile +++ b/Makefile @@ -1015,9 +1015,8 @@ cmd_pad_cat = $(cmd_objcopy) && $(append) || { rm -f $@; false; } quiet_cmd_lzma = LZMA $@ cmd_lzma = lzma -c -z -k -9 $< > $@ -quiet_cmd_mkeficapsule = MKEFICAPSULE $@ -cmd_mkeficapsule = $(objtree)/tools/mkeficapsule -K $(CONFIG_EFI_PKEY_FILE) \ - -D $@ +quiet_cmd_fdtsig = FDTSIG $@ +cmd_fdtsig = $(srctree)/tools/fdtsig.sh $(CONFIG_EFI_PKEY_FILE) $@ cfg: u-boot.cfg @@ -1114,7 +1113,7 @@ dtbs: dts/dt.dtb ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE)$(CONFIG_EFI_PKEY_DTB_EMBED),yy) dts/dt.dtb: u-boot tools $(Q)$(MAKE) $(build)=dts dtbs - $(call cmd,mkeficapsule) + $(call cmd,fdtsig) else dts/dt.dtb: u-boot $(Q)$(MAKE) $(build)=dts dtbs diff --git a/tools/Makefile b/tools/Makefile index 02eae0286e20..71a52719620c 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -235,7 +235,6 @@ ifneq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),) HOSTLDLIBS_mkeficapsule += \ $(shell pkg-config --libs libssl libcrypto 2> /dev/null || echo "-lssl -lcrypto") endif -mkeficapsule-objs := mkeficapsule.o $(LIBFDT_OBJS) hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule # We build some files with extra pedantic flags to try to minimize things diff --git a/tools/fdtsig.sh b/tools/fdtsig.sh new file mode 100755 index 000000000000..aaa0a9190845 --- /dev/null +++ b/tools/fdtsig.sh @@ -0,0 +1,40 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0+ +# +# script to add a certificate (efi-signature-list) to dtb blob + +usage() { + if [ -n "$*" ]; then + echo "ERROR: $*" + fi + echo "Usage: "$(basename $0) " <esl file> <dtb file>" +} + +if [ "$#" -ne 2 ]; then + usage "Arguments missing" + exit 1 +fi + +ESL=$1 +DTB=$2 +NEW_DTB=$(basename $DTB)_tmp +SIG=signature + +cat << 'EOF' > $SIG.dts +/dts-v1/; +/plugin/; + +&{/} { + signature { + capsule-key = /incbin/("ESL"); + }; +}; +EOF + +sed -in "s/ESL/$ESL/" $SIG.dts + +dtc -@ -I dts -O dtb -o $SIG.dtbo $SIG.dts +fdtoverlay -i $DTB -o $NEW_DTB -v $SIG.dtbo +mv $NEW_DTB $DTB + +rm $SIG.dts $SIG.dtbo $NEW_DTB diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 34ff1bdd82eb..97ce68ec83ee 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -4,17 +4,13 @@ * Author: AKASHI Takahiro */ -#include <errno.h> #include <getopt.h> #include <malloc.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <unistd.h> #include <linux/types.h> - -#include <sys/mman.h> #include <sys/stat.h> #include <sys/types.h> @@ -28,8 +24,6 @@ #include <openssl/pkcs7.h> #endif -#include <linux/libfdt.h> - typedef __u8 u8; typedef __u16 u16; typedef __u32 u32; @@ -39,9 +33,6 @@ typedef __s32 s32; #define aligned_u64 __aligned_u64 -#define SIGNATURE_NODENAME "signature" -#define OVERLAY_NODENAME "__overlay__" - #ifndef __packed #define __packed __attribute__((packed)) #endif @@ -59,9 +50,9 @@ efi_guid_t efi_guid_image_type_uboot_raw = efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; #if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) -static const char *opts_short = "f:r:i:I:v:D:K:P:C:m:dOh"; +static const char *opts_short = "f:r:i:I:v:P:C:m:dh"; #else -static const char *opts_short = "f:r:i:I:v:D:K:Oh"; +static const char *opts_short = "f:r:i:I:v:h"; #endif static struct option options[] = { @@ -69,15 +60,12 @@ static struct option options[] = { {"raw", required_argument, NULL, 'r'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'}, - {"dtb", required_argument, NULL, 'D'}, - {"public key", required_argument, NULL, 'K'}, #if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) {"private-key", required_argument, NULL, 'P'}, {"certificate", required_argument, NULL, 'C'}, {"monotonic-count", required_argument, NULL, 'm'}, {"dump-sig", no_argument, NULL, 'd'}, #endif - {"overlay", no_argument, NULL, 'O'}, {"help", no_argument, NULL, 'h'}, {NULL, 0, NULL, 0}, }; @@ -104,180 +92,6 @@ static void print_usage(void) tool_name); } -static int fdt_add_pub_key_data(void *sptr, void *dptr, size_t key_size, - bool overlay) -{ - int parent; - int ov_node; - int frag_node; - int ret = 0; - - if (overlay) { - /* - * The signature would be stored in the - * first fragment node of the overlay - */ - frag_node = fdt_first_subnode(dptr, 0); - if (frag_node == -FDT_ERR_NOTFOUND) { - fprintf(stderr, - "Couldn't find the fragment node: %s\n", - fdt_strerror(frag_node)); - goto done; - } - - ov_node = fdt_subnode_offset(dptr, frag_node, OVERLAY_NODENAME); - if (ov_node == -FDT_ERR_NOTFOUND) { - fprintf(stderr, - "Couldn't find the __overlay__ node: %s\n", - fdt_strerror(ov_node)); - goto done; - } - } else { - ov_node = 0; - } - - parent = fdt_subnode_offset(dptr, ov_node, SIGNATURE_NODENAME); - if (parent == -FDT_ERR_NOTFOUND) { - parent = fdt_add_subnode(dptr, ov_node, SIGNATURE_NODENAME); - if (parent < 0) { - ret = parent; - if (ret != -FDT_ERR_NOSPACE) { - fprintf(stderr, - "Couldn't create signature node: %s\n", - fdt_strerror(parent)); - } - } - } - if (ret) - goto done; - - /* Write the key to the FDT node */ - ret = fdt_setprop(dptr, parent, "capsule-key", - sptr, key_size); - -done: - if (ret) - ret = ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO; - - return ret; -} - -static int add_public_key(const char *pkey_file, const char *dtb_file, - bool overlay) -{ - int ret; - int srcfd = -1; - int destfd = -1; - void *sptr = NULL; - void *dptr = NULL; - off_t src_size; - struct stat pub_key; - struct stat dtb; - - /* Find out the size of the public key */ - srcfd = open(pkey_file, O_RDONLY); - if (srcfd == -1) { - fprintf(stderr, "%s: Can't open %s: %s\n", - __func__, pkey_file, strerror(errno)); - ret = -1; - goto err; - } - - ret = fstat(srcfd, &pub_key); - if (ret == -1) { - fprintf(stderr, "%s: Can't stat %s: %s\n", - __func__, pkey_file, strerror(errno)); - ret = -1; - goto err; - } - - src_size = pub_key.st_size; - - /* mmap the public key esl file */ - sptr = mmap(0, src_size, PROT_READ, MAP_SHARED, srcfd, 0); - if (sptr == MAP_FAILED) { - fprintf(stderr, "%s: Failed to mmap %s:%s\n", - __func__, pkey_file, strerror(errno)); - ret = -1; - goto err; - } - - /* Open the dest FDT */ - destfd = open(dtb_file, O_RDWR); - if (destfd == -1) { - fprintf(stderr, "%s: Can't open %s: %s\n", - __func__, dtb_file, strerror(errno)); - ret = -1; - goto err; - } - - ret = fstat(destfd, &dtb); - if (ret == -1) { - fprintf(stderr, "%s: Can't stat %s: %s\n", - __func__, dtb_file, strerror(errno)); - goto err; - } - - dtb.st_size += src_size + 0x30; - if (ftruncate(destfd, dtb.st_size)) { - fprintf(stderr, "%s: Can't expand %s: %s\n", - __func__, dtb_file, strerror(errno)); - ret = -1; - goto err; - } - - errno = 0; - /* mmap the dtb file */ - dptr = mmap(0, dtb.st_size, PROT_READ | PROT_WRITE, MAP_SHARED, - destfd, 0); - if (dptr == MAP_FAILED) { - fprintf(stderr, "%s: Failed to mmap %s:%s\n", - __func__, dtb_file, strerror(errno)); - ret = -1; - goto err; - } - - if (fdt_check_header(dptr)) { - fprintf(stderr, "%s: Invalid FDT header\n", __func__); - ret = -1; - goto err; - } - - ret = fdt_open_into(dptr, dptr, dtb.st_size); - if (ret) { - fprintf(stderr, "%s: Cannot expand FDT: %s\n", - __func__, fdt_strerror(ret)); - ret = -1; - goto err; - } - - /* Copy the esl file to the expanded FDT */ - ret = fdt_add_pub_key_data(sptr, dptr, src_size, overlay); - if (ret < 0) { - fprintf(stderr, "%s: Unable to add public key to the FDT\n", - __func__); - ret = -1; - goto err; - } - - ret = 0; - -err: - if (sptr) - munmap(sptr, src_size); - - if (dptr) - munmap(dptr, dtb.st_size); - - if (srcfd != -1) - close(srcfd); - - if (destfd != -1) - close(destfd); - - return ret; -} - struct auth_context { char *key_file; char *cert_file; @@ -608,19 +422,13 @@ err_1: int main(int argc, char **argv) { char *file; - char *pkey_file; - char *dtb_file; efi_guid_t *guid; unsigned long index, instance; uint64_t mcount; char *privkey_file, *cert_file; int c, idx; - int ret; - bool overlay = false; file = NULL; - pkey_file = NULL; - dtb_file = NULL; guid = NULL; index = 0; instance = 0; @@ -656,20 +464,6 @@ int main(int argc, char **argv) case 'I': instance = strtoul(optarg, NULL, 0); break; - case 'K': - if (pkey_file) { - printf("Public Key already specified\n"); - return -1; - } - pkey_file = optarg; - break; - case 'D': - if (dtb_file) { - printf("DTB file already specified\n"); - return -1; - } - dtb_file = optarg; - break; #if IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) case 'P': if (privkey_file) { @@ -692,9 +486,6 @@ int main(int argc, char **argv) dump_sig = 1; break; #endif - case 'O': - overlay = true; - break; case 'h': print_usage(); return 0; @@ -702,27 +493,15 @@ int main(int argc, char **argv) } /* check necessary parameters */ - if ((file && (!(optind < argc) || - (privkey_file && !cert_file) || - (!privkey_file && cert_file))) || - ((pkey_file && !dtb_file) || - (!pkey_file && dtb_file))) { + if (file && (!(optind < argc) || + (privkey_file && !cert_file) || + (!privkey_file && cert_file))) { print_usage(); exit(EXIT_FAILURE); } - if (pkey_file && dtb_file) { - ret = add_public_key(pkey_file, dtb_file, overlay); - if (ret == -1) { - printf("Adding public key to the dtb failed\n"); - exit(EXIT_FAILURE); - } - } - - if (optind < argc && - create_fwbin(argv[optind], file, guid, index, instance, - mcount, privkey_file, cert_file) - < 0) { + if (create_fwbin(argv[optind], file, guid, index, instance, + mcount, privkey_file, cert_file) < 0) { printf("Creating firmware capsule failed\n"); exit(EXIT_FAILURE); }
As we discussed, "-K" and "-D" options have nothing to do with creating a capsule file. The same result can be obtained by using standard commands like: === signature.dts === /dts-v1/; /plugin/; &{/} { signature { capsule-key = /incbin/("SIGNER.esl"); }; }; === $ dtc -@ -I dts -O dtb -o signature.dtbo signature.dts $ fdtoverlay -i test.dtb -o test_sig.dtb -v signature.dtbo So just remove this feature. (Effectively revert the commit 322c813f4bec ("mkeficapsule: Add support for embedding public key in a dtb").) The same feature is implemented by a shell script (tools/fdtsig.sh). Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- Makefile | 7 +- tools/Makefile | 1 - tools/fdtsig.sh | 40 ++++++++ tools/mkeficapsule.c | 235 ++----------------------------------------- 4 files changed, 50 insertions(+), 233 deletions(-) create mode 100755 tools/fdtsig.sh -- 2.31.0