On 14/01/2022 17.51, Simon Glass wrote:
> Hi Rasmus,
>
> On Tue, 26 Oct 2021 at 02:08, Rasmus Villemoes
> <rasmus.villem...@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.villem...@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.villem...@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.villem...@prevas.dk/
>>>> [2]
>>>> https://lore.kernel.org/u-boot/2184f1e6dd6247e48133c76205fee...@kaspersky.com/
>>>>
>>>> dts/Kconfig | 9 +++++++++
>>>> scripts/Makefile.lib | 2 ++
>>>> 2 files changed, 11 insertions(+)
>>>
>>> Reviewed-by: Simon Glass <s...@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@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.
> 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.
> 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>".
> 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.
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.
Rasmus
