Re: [PATCH V5 08/12] iot2050: Add script for signing artifacts

[Simon Glass <sjg@chromium.org>]

Hi Jan,

On Sun, 12 Feb 2023 at 22:33, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 13.02.23 05:26, Simon Glass wrote:
> > Hi Jan,
> >
> > On Tue, 7 Feb 2023 at 11:39, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Jan,
> >>
> >> On Tue, 7 Feb 2023 at 09:45, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>
> >>> On 07.02.23 16:28, Simon Glass wrote:
> >>>> Hi Jan,
> >>>>
> >>>> On Mon, 6 Feb 2023 at 04:57, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>>
> >>>>> On 06.02.23 11:47, Jan Kiszka wrote:
> >>>>>> On 04.02.23 23:26, Simon Glass wrote:
> >>>>>>> Hi Jan,
> >>>>>>>
> >>>>>>> On Fri, 3 Feb 2023 at 23:35, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>>>>>
> >>>>>>>> On 03.02.23 19:51, Tom Rini wrote:
> >>>>>>>>> On Fri, Feb 03, 2023 at 01:26:37PM +0100, Jan Kiszka wrote:
> >>>>>>>>>
> >>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>>>
> >>>>>>>>>> There are many ways to get a signed firmware for the IOT2050 devices,
> >>>>>>>>>> namely for the parts under user-control. This script documents one way
> >>>>>>>>>> of doing it, given a signing key. Augment the board documentation with
> >>>>>>>>>> the required procedure around it.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>> [snip]
> >>>>>>>>>> +# currently broken in upstream
> >>>>>>>>>> +#source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit fit@0x380000
> >>>>>>>>>> +dd if=fit@0x380000.fit of=flash.bin bs=$((0x1000)) seek=$((0x380000/0x1000)) conv=notrunc
> >>>>>>>>>
> >>>>>>>>> Is that still a true comment?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> binman: Node '/fit@0x380000/images/u-boot': Offset 0x0 (0) size 0xb8870
> >>>>>>>> (755824) is outside the section '/fit@0x380000' starting at 0x0 (0) of
> >>>>>>>> size 0x400 (1024)
> >>>>>>>>
> >>>>>>>> And for the second call:
> >>>>>>>>
> >>>>>>>> binman: Node '/fit@0x380000': Replacing sections is not implemented yet
> >>>>>>>
> >>>>>>> I sent a patch to implement that.
> >>>>>>>
> >>>>>>> I'm not quite sure if this resolves the first problem, too. If not,
> >>>>>>> can you please provide a binman test for the case you need, or
> >>>>>>> instructions on how to cause the failure?
> >>>>>>
> >>>>>> Instructions to reproduce are basically
> >>>>>>  - apply this series
> >>>>>>  - build flash.bin according to doc/board/siemens/iot2050.rst
> >>>>>>  - drop the dd calls and activate binman in this signing script
> >>>>>>  - run it
> >>>>>>
> >>>>>> But I'll try your patch ASAP on my setup.
> >>>>>
> >>>>> Still left with
> >>>>>
> >>>>> binman: Node '/fit@0x380000/images/u-boot': Offset 0x0 (0) size 0xb8928
> >>>>> (756008) is outside the section '/fit@0x380000' starting at 0x0 (0) of
> >>>>> size 0x400 (1024)
> >>>>>
> >>>>> and
> >>>>>
> >>>>> binman: 'NoneType' object has no attribute 'props'
> >>>>>
> >>>>> That was for the second call of binman (source/tools/binman/binman
> >>>>> replace -i flash.bin -f fit@0x380000.fit fit@0x380000). The "not
> >>>>> implemented messages is gone.
> >>>>>
> >>>>> I've switched back to dd for the first call, but that did not work yet.
> >>>>> So the message above indicates a relevant error.
> >>>>>
> >>>>> Jan
> >>>>
> >>>> I thought I was able to follow all the steps (gosh, so many blobs) but
> >>>> I got something different from the first 'binman' call in your script.
> >>>>
> >>>> binman: Error 1 running 'mkimage -t -F
> >>>> /tmp/binman.l_xl69mi/fit@0x380000.fit': mkimage: verify_header failed
> >>>> for FIT Image support with exit code 1
> >>>>
> >>>> I will take a look at that...it is trying to rebuild the FIT and it
> >>>> should not. It is another case of rebuilding sections that I didn't
> >>>> think of.
> >>>>
> >>>> But actually, you need to create a new entry type for your signing
> >>>> scheme. It looks like the signature is created by openssl and (rather
> >>>> than putting it in a separate file) it creates a new file containing
> >>>> both the signature and the file contents. That is a bit of a pain, but
> >>>> can be made to work.
> >>>>
> >>>> Basically you need a new entry type (of which the FIT is a child) that
> >>>> gets the contents of its child, signs it and returns that as the
> >>>> contents. You can see vblock for an example, and
> >>>> collection_contents_to_file(). Let me know if you want me to create an
> >>>> example.
> >>>>
> >>>> The way it should work is that you run binman once (as part of the
> >>>> U-Boot build) and it produces a final image. No messing about with
> >>>> scripts, etc. In this case it looks like the key.pem file should be an
> >>>> input to your new entry type.
> >>>>
> >>>> Using binman replace to sign something later is fine, but it is not
> >>>> the intended use. Binman is supposed to be a single-pass image
> >>>> builder.
> >>>
> >>> I strongly suspect we will need split build/sign also in the future
> >>> because those steps are generally separate in corporate production envs.
> >>> Maybe even 3 steps: assemble, extract hashes that should be signed and
> >>> embed signatures of those in the end.
> >>
> >> Yes I'm sure you are right, that's what I would expect. There is a
> >> 'binman sign' command coming[1] so I hope we can use that to make it
> >> easier, too.
> >>
> >> Still, we must have a single-step build in U-Boot, so we do need a new
> >> entry type. Let me know if you want me to hack up something as a
> >> starting point.
> >
> > Please see here:
> >
> > https://github.com/sjg20/u-boot/tree/for-jan
> >
> > There is an entry type to create an x509 certificate, which I think it
> > part of what you are trying to do in your shell script.
> >
> > Please take a look and see what you think.
> >
>
> Generating the cert is one problem, and it is surely valuable to have
> such a feature in the end - BTW, also for TI's reference boards when
> U-Boot is the FSBL (see also board/ti/am65x/README). But the cert is
> opt-in for non-secure mode. It moves the payload up when present. That
> also needs to be modeled correctly.

Does this mean that the entry needs to be optional? How can we tell
binman what to do? One option is to use the -a entry argument to
provide the key. If that is not present it can mark the entry as zero
size perhaps, or drop it altogether?

>
> > The problem with the 'binman replace' command in the script is that it
> > seems to be overwriting the fdtmap. I am not sure why but will take a
> > look.
>
> Indeed! TIA.

I suppose you saw that this is fixed, or seems t obe.

>
> >
> > In any case, we should not be using the script, so let's try to get
> > the binman description complete for your board, so it contains all the
> > steps.
>
> I'm open for it, but the path seems longer. Meanwhile, I would
> appreciate if we could document to procedure with that script, maybe
> leaving a note that this is purely transitional.

The problem is that these things tend to stick around for a long time.
It took an unreasonable amount of effort for me to remove much of the
SPL_FIT_GENERATOR stuff, for example. It is better to do it correctly
the first time. Migrating things over is always difficult because
sometimes things break, people cannot test the board quickly, patches
languish around, etc.

Putting it another way, here is an exciting oppty to use a data-driven
firmware packer with tests and automatic tool installation!

Regards,
Simon