Re: [PATCH 1/7] capsule: authenticate: Embed capsule public key in platform's dtb

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Simon,

[...]

> > > > > > +
> > > > > > +gen_capsule_signature_file signature.$$.dts > /dev/null 2>&1
> > > > > > +$CPP $dtc_cpp_flags -x assembler-with-cpp -o signature.$$.tmp signature.$$.dts > /dev/null 2>&1
> > > > > > +dtc -@ -O dtb -o signature.$$.dtbo signature.$$.tmp > /dev/null 2>&1
> > > > > > +fdtoverlay -i $1 -o temp.$$.dtb -v signature.$$.dtbo > /dev/null 2>&1
> > > > > > +mv temp.$$.dtb $1 > /dev/null 2>&1
> > > > > > +rm -f signature.$$.* > /dev/null 2>&1
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > >
> > > > > Can you please add this to binman instead?
> > > >
> > > > I had looked at using binman for this work earlier because I very much
> > > > expected this comment from you :). Having said that, I am very much
> > > > open to using binman instead if it turns out to be the better way of
> > > > achieving this. What this patch does is that, with capsule
> > > > authentication enabled, it embeds the public key esl file into the
> > > > dtb's as they get built. As per my understanding, binman gets called
> > > > at the end of the u-boot build, once the constituent images( e..g
> > > > u-boot.bin = u-boot-no-dtb.bin + dtb) have been generated. So, if we
> > > > call binman _after_ the requisite image(s) have been generated, we
> > > > would need to 1) identify the dtb's in which the esl needs to be
> > > > embedded, and then 2) generate the final image all over again. Don't
> > > > you think this is non optimal? Or is there a way of generating the
> > > > constituent images(including the dtb's) through binman instead?
> > >
> > > The best way to do that IMO is to generate a second file, .e.g.
> > > u-boot-capsule.bin
> >

This make no sense to me whatsoever.  Do we have an example in u-boot
generating multiple dtb versions for other reasons/subsystems?

> > That would break the scripts for platforms which might be using
> > u-boot.bin as the image to boot from. I know that the ST platform
> > which does enable capsule updates uses the u-boot-nodtb.bin as the
> > BL33 image and the u-boot.dtb as BL33_CFG. Hence my question, if we
> > have to use binman, is there a way to 1) modify the u-boot.dtb and
> > then 2) regenerate u-boot.bin image.
> >
> > I know this is software, and everything can be done in a hacky way.
> > But I was exploring using the u-boot node as a section entry, so that
> > the u-boot.dtb can be modified and then binman would
> > repackage/regenerate the u-boot.bin. But this is not working.
>
> NO, please do not do that.  You should create a new file, not modify
> u-boot.bin or u-boot.dtb. Please just don't mess around with this, it
> will lead to all sorts of confusion.
>
> I thought we already had this discussion a while back?

No we haven't.  In fact I am struggling to see the confusion part.  It's
fine for the u-boot dtb to include all the internal nodes DM needs, but
suddenly having the capsule signature is problematic?

In the past the .esl file was part of the U-Boot binary and things were
working perfectly fine.  In fact you could update/downgrade u-boot and the
signatures naturally followed along instead of having to update u-boot
*and* the dtb, which we have to do today. You could also build a capsule
way easier without injecting/removing signatures to the dtb.
You were the one that insisted on reverting that and instead adding it on
the dtb.  We explained most of the downsides back then, along with some
security concerns.  We also mentioned that the signature in the dtb makes
little sense since it's difference *per class of boards* and it's not
something we could include in static dtb files, but that lead nowhere...

As Sughosh already said there are platforms that use the generated u-boot
dtb and the raw binary to assemble a FIP image.  So why exactly adding the
capsule signature to the default dtb is wrong?  Why should we add an
*extra* .dtb with one additional node -- which is very much needed by the
design you proposed.  This will only lead to extra confusion.


Thanks
/Ilias
>
> >
> > >
> > > I don't think it is a good idea to add other junk to u-boot.bin. It
> > > should just be U-Boot + dtb.
> >
> > No junk is being added to u-boot.bin. Just that, as the platform
> > builds dtb's, the ESL file gets embedded into them as a property under
> > the signature node. There is no additional image being added to the
> > the u-boot.bin.
>
> This needs to be done in a separte file, as above.
>
> Regards,
> Simon