From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Thu, 13 May 2021 19:42:23 +0200 Subject: [PATCH 2/4] tools: mkeficapsule: remove device-tree related operation In-Reply-To: <20210513071326.GH16848@laputa> References: <20210512045753.62288-1-takahiro.akashi@linaro.org> <20210512045753.62288-3-takahiro.akashi@linaro.org> <8b2f7c9b-bcac-566a-b085-54f5d5a62dba@gmx.de> <20210513023306.GB16848@laputa> <20210513071326.GH16848@laputa> Message-ID: <755f30f4-f0f2-5c40-751f-fd393c6e2671@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.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 : >>>>>>> >>>>>>> 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