[PATCH] introduce CONFIG_DEVICE_TREE_INCLUDES

[Simon Glass]

Hi Rasmus,

On Mon, 24 Jan 2022 at 03:42, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 14/01/2022 17.51, Simon Glass wrote:
> > 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.
>
> Eh, does make(1) exit with a code that depends on the exit code from a
> failing target? What if multiple targets failed to build? Unless one can
> quote chapter-and-verse from make documentation, I don't think that's a
> particularly robust solution.

Make is not the issue here. The basic problem is that we want to have
three results:

- build failed, e.g. a compiler error
- build succeeded but some binary blobs or tools are missing so the
image won't work
- build succeeded, image is valid

>
> > Normally people use a separate output directory from the source
> > directory, which is why the 'pollution' is not normally a big problem.
>
> Well, build systems like Yocto do set up a separate build dir, but
> that's not really important, as I rarely look inside ${S} nor ${B} - but
> when doing my own tinkering, I almost always build in the source tree,
> simply because that's a lot faster for quick experiments. So I'd wish
> all binman's temporary files were stowed away in some .binman_tmp dir in
> the top build dir (whether that's the source dir or an out-of-tree dir).
> Commits like 824204e42 are a strong indication that the current state of
> things is broken and doesn't scale.

Well that commit was really just rearranging the deck chairs. I didn't
even see it.

The topic has come up before and I have suggested how it could be
done. There is even a long-standing TODO in the binman readme about
it.

>
> > 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?
>
> My main grievance is that when binman is merely a thin wrapper around
> some external tool (often mkimage or one of its derivates), there's no
> .<target>.cmd file generated allowing me to see just what command was
> run to generate <target>. Another thing is that I can't do "make <target>".

You can enable logging with BINMAN_VERBOSE. See also the bintool
series (which I should apply soon) which provides a single place for
running these tools. So we could perhaps improve things in this area.

>
> > 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.
>
> Oh, I couldn't agree more, and in fact I've already done that for all
> the kernels I build; Yocto's kernel-fitimage.bbclass is simply too
> inflexible to use directly from the kernel recipe, so I just build an
> Image (or Image.gz) in the kernel recipe, then have a separate
> kernel-fit.bb recipe for actually assembling the different FIT images I
> need (often one for normal use and another for bootstrapping, but
> possibly more).
>
> But I really can't see how binman helps in any way or form - in fact, it
> obscures the process of creating such a separate recipe, exactly because
> I can't extract the necessary magic invocation of mkimage that is needed
> to wrap SPL in the header format expected by the hardware.

Why are you wanting to do that? You can just run binman again, can't you?

>
> And the thing about "adding the signature" - yes, indeed, _signing_ can
> and should be done after building. But that is not at all what this
> started with, this is about embedding the metadata that U-Boot (or SPL)
> will need for _verifying_ during the build itself - when the private key
> may not even be available. Again, I think that it's a fundamental design
> bug that generating and adding that metadata in the form needed by
> U-Boot can only be done as a side effect of signing some unrelated image.

It is a side effect of signing *the same* image, i.e. the image that
holds the signature and the public key. There is only one image, the
firmware image produced by binman.

Regards,
Simon