[PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Rasmus,

On Tue, 26 Oct 2021 at 02:08, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 26/10/2021 03.28, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 28 Sept 2021 at 02:57, Rasmus Villemoes
> > <rasmus.villemoes at prevas.dk> wrote:
> >>
> >> The build system already automatically looks for and includes an
> >> in-tree *-u-boot.dtsi when building the control .dtb. However, there
> >> are some things that are awkward to maintain in such an in-tree file,
> >> most notably the metadata associated to public keys used for verified
> >> boot.
> >>
> >> The only "official" API to get that metadata into the .dtb is via
> >> mkimage, as a side effect of building an actual signed image. But
> >> there are multiple problems with that. First of all, the final U-Boot
> >> (be it U-Boot proper or an SPL) image is built based on a binary
> >> image, the .dtb, and possibly some other binary artifacts. So
> >> modifying the .dtb after the build requires the meta-buildsystem
> >> (Yocto, buildroot, whatnot) to know about and repeat some of the steps
> >> that are already known to and handled by U-Boot's build system,
> >> resulting in needless duplication of code. It's also somewhat annoying
> >> and inconsistent to have a .dtb file in the build folder which is not
> >> generated by the command listed in the corresponding .cmd file (that
> >> of course applies to any generated file).
> >>
> >> So the contents of the /signature node really needs to be baked into
> >> the .dtb file when it is first created, which means providing the
> >> relevant data in the form of a .dtsi file. One could in theory put
> >> that data into the *-u-boot.dtsi file, but it's more convenient to be
> >> able to provide it externally: For example, when developing for a
> >> customer, it's common to use a set of dummy keys for development,
> >> while the consultants do not (and should not) have access to the
> >> actual keys used in production. For such a setup, it's easier if the
> >> keys used are chosen via the meta-buildsystem and the path(s) patched
> >> in during the configure step. And of course, nothing prevents anybody
> >> from having DEVICE_TREE_INCLUDES point at files maintained in git, or
> >> for that matter from including the public key metadata in the
> >> *-u-boot.dtsi directly and ignore this feature.
> >>
> >> There are other uses for this, e.g. in combination with ENV_IMPORT_FDT
> >> it can be used for providing the contents of the /config/environment
> >> node, so I don't want to tie this exclusively to use for verified
> >> boot.
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> >> ---
> >>
> >> Getting the public key metadata into .dtsi form can be done with a
> >> little scripting (roughly 'mkimage -K' of a dummy image followed by
> >> 'dtc -I dtb -O dts'). I have a script that does that, along with
> >> options to set 'required' and 'required-mode' properties, include
> >> u-boot,dm-spl properties in case one wants to check U-Boot proper from
> >> SPL with the verified boot mechanism, etc. I'm happy to share that
> >> script if this gets accepted, but it's moot if this is rejected.
> >>
> >> I have previously tried to get an fdt_add_pubkey tool accepted [1,2]
> >> to disentangle the kernel and u-boot builds (or u-boot and SPL builds
> >> for that matter!), but as I've since realized, that isn't quite enough
> >> - the above points re modifying the .dtb after it is created but
> >> before that is used to create further build artifacts still
> >> stand. However, such a tool could still be useful for creating the
> >> .dtsi info without the private keys being present, and my key2dtsi.sh
> >> script could easily be modified to use a tool like that should it ever
> >> appear.
> >>
> >> [1] https://lore.kernel.org/u-boot/20200211094818.14219-3-rasmus.villemoes@prevas.dk/
> >> [2] https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205feeabe@kaspersky.com/
> >>
> >>  dts/Kconfig          | 9 +++++++++
> >>  scripts/Makefile.lib | 2 ++
> >>  2 files changed, 11 insertions(+)
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > I suggest adding this to the documentation somewhere in doc/develop/devicetree/
>
> Will do.
>
> > Getting the key into the U-Boot .dtb is normally done with mkimage, as
> > you say. I don't really understand why this approach is easier.
>
> I think I explained that in the commit message, but let me try with a
> more concrete example. I'm building, using Yocto as the umbrella build
> system, for a SOC that needs an SPL + U-Boot proper.
>
> So I have a U-Boot recipe that handles the configuration and
> compilation. That's mostly a matter of "make foo_defconfig" followed by
> "make ${UBOOT_TARGETS}" where UBOOT_TARGETS is something set in the
> machine config. That results in two artifacts, say SPL and u-boot.itb
> (the names don't really matter) that are almost ready to be written to
> whatever storage is used for them. Most likely, the SPL binary is built
> from a u-boot-spl-nodtb.bin and a spl.dtb and perhaps there's some
> SOC-specific header in front of it all that the hardware/firmware needs,
> those details are hidden by U-Boot's build system invoking the right
> flavour of mkimage with the right parameters. If I really want to know,
> I can look in spl/.SPL.cmd to see just how it was made [unless it's
> binman creating a flash.bin, because then it's just black magic]. But I
> usually don't need to.
>
> Enter verified boot. SPL needs to verify U-Boot, and U-Boot must in turn
> verify the kernel. I can easily, as a post-compile step, create a signed
> version of u-boot.itb. But the -K option is rather useless here, because
> SPL has already been built. So if I were to only add the corresponding
> public key to SPL's dtb after/while signing U-Boot proper, I'd have to
> manually repeat the step(s) needed to create SPL in the first place. Not
> to mention that the .dtb living inside u-boot.itb doesn't have the
> public key needed for verifying the kernel, so I'd _actually_ first have
> to repeat whatever steps were done to create u-boot.itb, after using
> mkimage to sign some dummy image just to use the -K option to modify
> u-boot.dtb. It's really cumbersome.
>
> So part of the problem is this "you can only get the public keys in the
> form required inside the .dtb as a side-effect of signing an image". I
> believe that's a fundamental design mistake, hence my attempt at
> creating the fdt_add_pubkey tool. But even with that available, adding
> the pubkey info into an already-compiled .dtb isn't really enough,
> because the .dtb gets built as part of a normal "make". Hence the need
> for a way to ensure the pubkey info gets baked into that .dtb during the
> build.
>
> Yes, I then need to prepare proper .dtsi files. But since key generation
> is a mostly one-time/manual thing, that easily goes along with the
> instructions (or script) that generates those, and for every foo.crt,
> I'll just have that directory also contain a foo.dtsi, which I can then
> point at during do_configure.
>
> > Also, is there any interest in using binman?
>
> Not really. I mean, it's fine if U-Boot internally uses that, and I can
> just take the final output and use that. But as long as binman doesn't
> play nice with Kbuild and e.g. tells the commands that were used to
> create a given binary, and pollutes the build dir with stuff that's not
> removed by "make clean", I'm not very enthusiastic about adding more
> uses myself.
>
> Also, this:
>
>   BINMAN  all
> Image 'main-section' is missing external blobs and is non-functional:
> blob-ext at 3
>
> Some images are invalid
> $ echo $?
> 0
>
> Really? When building manually, perhaps the developer sees that warning
> (unless there were other non-BINMAN targets that make decided to build
> later, making the above scroll away...), but it's not very useful in
> some CI scenario where artifacts get deployed automatically to a test
> system after a successful build. Recovering boards with a broken
> bootloader easily costs many man-hours, plus the time to figure out
> what's wrong.

I still think binman is the best solution here. The points you are
raising are annoyances that can be resolved.

We could return a non-zero value from binman when external blobs are
missing; we'd just need to use a special value (we use 101 for
buildman) and update the CI to detect and ignore that.

Normally people use a separate output directory from the source
directory, which is why the 'pollution' is not normally a big problem.
But it is possible to resolve that problem and it has been discussed
on the mailing list. It's just that no one has done it yet.

In what way does binman play badly with Kbuild?

I'd really suggest changing the mindset to separating build from
packaging, perhaps by having a separate yocto package/recipe that puts
the firmware together, adds the signature, etc. Images are only
getting more complicated. The idea that you'll be able to build
everyone in the U-Boot tree and it won't need any post-processing is
not going to survive very long IMO, so you may as well invest in it
now :-)

Regards,
Simon