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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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
>