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

[Simon Glass]

Hi,

On Tue, 27 Jun 2023 at 10:55, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, 26 Jun 2023 at 14:19, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Mon, 26 Jun 2023 at 10:53, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > 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.
> >
> > 1. I thought a capsule update was going to be a separate file, e.g.
> > u-boot-capture.bin ?
>
> Yes the capsule itself is a different file and I dont think there's
> any disagreement on how to generate that.
> On the u-boot.bin you need to flash on the board though, you need to
> include the public key you authenticate the incoming capsule against.
> That's what Sughosh wants to inject.
>
> >
> > 2. You can't put the signature into U-Boot. It needs to be in the
> > capsule update so that U-Boot can check it. If you are talking about
> > the public key, then yes that needs to be in U-Boot
>
> See above,
>
> >
> > 3. Where does the public key come from? With the normal verified boot
> > flow it is created (and added to the dtb) as a separate signing step
> > after U-Boot is built.
>
> It's per class of hardware or whatever the vendor decides it should be.
>
> >
> > 4. Off topic, but re FIP, can someone just kill that off and use FIT?
> > It is such a shame that a new format was invented...
>
> You'd have to ask the TrustedFirmware-A code owners, but I doubt it.
> FIP is what TF-A uses for the first stage bootloader packaging and
> authentication of subsequent boot stages.

Sughosh do you happen to be at EOSS so we could talk this through?
Otherwise I'll reply later on.

Regards,
Simon