Re: Pull request for efi-2021-10-rc2-2

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

[...] 
> >
> > >
> > > >
> > > > >
> > > > > Look I'm sorry if this all seems a bit much. My initial request was
> > > > > rebuffed, other emails have been ignored and a large number of
> > > > > objections have been raised. It's just too hard. As far as I can
> > > > > remember, I've not come across anything like this in my time
> > > > > contributing to U-Boot.
> > > >
> > > > That's because putting in the DTB makes no sense whatsoever. On the
> > > > contrary due to the different options of the control DTB origin, it
> > > > overcomplicates the security of the key.
> > >
> > > I think you imagine that the DTB needs to be created in another
> > > project and then not modified through the build system. But that is
> > > not the intent.
> >
> > But it might as well be a reality, with TF-A.
> 
> Of course there are always exceptions.
> 
> >
> > > The DTB is created from source but then other things
> > > can be added to it. For example, mkimage adds a public key and so did
> > > the EFI implementation until your patch series.
> >
> > Not exactly. You had to configure U-Boot with OF_SEPARATE, fixup the dtb
> > manually with mkeficapsule application (which is what Akashi proposed to
> > revert) and the concatenate u-boot-nodtb.bin and the edited dtb
> 
> Well of course if the DT holds the key and you want to add the key
> after the build, you have to modify the DT. You make it sounds like a
> huge deal...

Having to edit the DT and concat it is not a huge deal.  Limiting the
ability to provide authenticated capsule updates, based on a config option
which defines how we supply he DTB is (at least for me).  Especially since
the current approach doesn't have such limitations. 

> 
> OF_SEPARATE is required, actually. We try to use OF_EMBED just for
> testing and development.

And that's what I've been trying to get an answer on a few mails. 
It's now quite clear, any board that's not compiled with OF_SEPARATE won't
support authenticated capsule updates (e.g rpi4 on the defconfig). 

> 
> If mkeficapsule is how you want to write to the DT, that is fine. You
> could also add support for this to binman, which might be easier to
> maintain.
> 
> Concatenating is handled by binman.

That's what Akashi patches were fixing. IT doesn't make too much sesne to
have it in mkeficapsule, so I'll leave it up to him.

> 
> >
> > > That is easy to do
> > > with a DTB but of course a real pain to do with an executable binary.
> > >
> > > What really complicates things is building the key into the source
> > > code of U-Boot. Whose key is it?
> >
> > The public portion of the organization that creates the capsule.
> >
> > > What if someone wants their own key?
> >
> > I am not sure I am following
> 
> They have to add the key into an environment variable and rebuild
> U-Boot from source. That's my point. You should not have to rebuild
> something from source to add the public key.
> 

"They" have to compile uboot anyway since they are offering the firmware to
begin with. I get that's t has some limitations, but it doesn't sound that
bad to me.

> >
> > > Will people really accept that another project has to sign their
> > > U-Boot? Or does everyone have to fiddle with the build system to make
> > > it produce suitable output. It's just not a good idea.
> >
> > No one *signs* U-Boot. The capsule is signed not u-boot (but contains
> > u-boot).  This key is a placeholder to authenticate the incoming capsule
> > update (which might update more than u-boot). But since
> > U-Boot *is* the EFI payload, it's responsible for having the key somewhere.
> 
> I just mean that U-Boot has the public key. I call it signing because
> signing produces two things:
> 
> - certificate or public key wrapper for checking signature (in U-Boot)
> - signature (in the thing you sign)
> 
> I think it is easier to think of signing as a single operation
> although of course you may split it in some build systems.
> 
> >
> > >
> > > >
> > > > > Someone has to hold the line on important design decisions and I am
> > > > > frustrated that there has been so much push-back on something that
> > > > > should have been a simple 'oh sorry, didn't know'. I wrote this patch
> > > > > somewhat in response:
> > > > >
> > > > > https://patchwork.ozlabs.org/project/uboot/patch/20210725164400.468319-3-sjg@chromium.org/
> > > > >
> > > > > >
> > > > > > Furthermore, on the entire thread, the responses I keep getting is "we just
> > > > > > need to move it on the dtb", although as I repeated a bunch of times up to
> > > > > > now, it's creating problems we don't have to deal with in the first place
> > > > > > and no one cares to argue about them.  But I'll agree it's taking way more
> > > > > > time that it has to.  For the record I am fine with whatever Heinrich
> > > > > > decides and I'll pick it up from there.
> > > > >
> > > > > It was already in DT, as I understand it. There was no good reason to
> > > > > change it, just some things that looked attractive on the surface.
> > > >
> > > > I won't go back explaining the entire thing again. It's not attractive "on
> > > > the surface", the only thing missing is u-boot doing a proper mapping of
> > > > sections during relocation.
> > >
> > > Which you can easily do with DT as well. Just copy the key somewhere
> > > and protect it...turn off CONFIG_CMLINE so commands cannot be used
> > > (call the overlay code directly!), this is just rehashing the same
> > > arguments. We have been using the current approach for years and it
> > > works well. This was all covered in:
> > >
> > > https://patchwork.ozlabs.org/project/uboot/patch/20210717142648.26588-1-ilias.apalodimas@linaro.org/
> > >
> > > I think perhaps the core of the confusion is that qemu passes the DT
> > > to U-Boot which passes it to linux and there is not a separate DT for
> > > U-Boot. But that is just a detail to figure out, not a reason to throw
> > > everything away.
> >
> > And this might be a reality for other boards as well (e.g RISC-V).
> 
> As I say, that is just a detail.

I don't know much about RISC-V and how the DTB is handled.  But if it
relies on CONFIG_OF_PRIOR_STAGE (which effectively means the feature won't
be supported), that's far away from a detail .

> 
> >
> > >
> > > >
> > > > > It would set a bad precedent that would lead to crazy-town with all sorts
> > > > > of strange things being compiled into U-Boot and update tools to
> > > > > update them.
> > > >
> > > > I've explicitly said that this is not a case. No one will update the key
> > > > using imaginary tools or whatever. In a secure setup that binary is
> > > > checked by a previous stage bootloader, so changing *anything* would mean
> > > > bricking the board.
> > >
> > > See my point above, re multiple keys. But here I am actually referring
> > > to the next feature that comes along where people 'oh, EFI builds xxx
> > > into the U-Boot binary, so we'll just do the same'. It sets a bad
> > > precedent and it one reason why I am so upset that this happened.
> > >
> > > >
> > > > > It is similar to the CONFIG_OF_EMBED thing in some ways.
> > > > > We separate the build from the packaging for a reason:
> > > > >
> > > > > https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#introduction
> > > > > https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#motivation
> > > > >
> > > > > More generally on EFI I think we should clean up the support to use
> > > > > driver model properly, to make it more maintainable, particularly with
> > > > > the DM migrations getting closer. I have some ideas on that which we
> > > > > could perhaps discuss in a future call.
> > > > >
> > > > > Regards,
> > > > > SImon
> > > >
> > > > Anyway, there are people far more suited to decide than me, so I'll
> > > > follow up after whatever is decided.
> > >
> > > Did you see the above docs?
> > >
> >
> > Yes and they explicitly refer to limitations with CONFIG_OF_PRIOR_STAGE etc,
> > which I've been asking for a couple of mails.
> 
> I suggested this when we spoke, but I still think a design doc would
> help a lot here. There seem to be a lot of hidden assumptions.
> 

The logic is quite straightforward tbh.  I didnt want DTB menuconfig options to
limit EFI functionality.  I'll go poke RISC-V, since there's limited
support in u-boot and find more.

> >
> > > If we revert the patches we can be back to the original approach,
> > > which fits with how signing works in U-Boot and is consistent with the
> > > build/packaging split that is necessary for anyone trying to put this
> > > into a production environment. Signing should not be added as part of
> > > building the source code unless there is only one key in the world, as
> > > it requires everyone to build from source.
> > >
> > > You have lost nothing in functionality or security. We can set up a
> > > proper API for protecting memory, move the key there and the fdt
> > > command has nothing to do with it.
> >
> > You need substantially more code and effort to secure the key, instead of
> > just marking some pages as RO during relocation, but I never said it's not
> > doable. In fact we discussed bit the .dtb and .rodata approaches internally
> > before posting the patch.
> >
> > Heinrich just sent similar concerns for RISC-V and CONFIG_OF_PRIOR_STAGE.
> > As I already said I dont mind reverting this, as long as Heinrich agrees.
> > I can fix QEMU breakages if we move the key back, but honestly I'd much
> > prefer putting the public key on an authenticated variable instead of the
> > DTB
> 
> I don't know much about OF_PRIOR_STAGE or how or why it is used. It
> seems like a special case to me at present. However from U-Boot's POV,
> so long as it can get the config somehow, then all is well.
> 

Which doesn't seem to be the case.  If u-boot had a way of saying "here's
the config dtb and here's the system dtb", I wouldn't mind putting the key
in the config one. 

Regards
/Ilias
> Do you have a link for Herinrich's concerns?
> 
> Regards,
> SImon