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

[Heinrich Schuchardt]

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.

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