[PATCH 2/4] tools: mkeficapsule: remove device-tree related operation

[Masami Hiramatsu]

Hi Heinrich,

2021年5月14日(金) 2:42 Heinrich Schuchardt <xypron.glpk at 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 at 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