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

[Simon Glass]

 +U-Boot Mailing List (sorry for somehow dropping it)

On Wed, 25 Aug 2021 at 03:23, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> > > > >
> > > > > Please stop trying to create false impressions. On the initial revert you
> > > > > sent, you said something along the lines of "this patch was applied in
> > > > > spite of my objections" and I responded the same thing back then.
> > > > >
> > > > > The patch was merged on v2, after it has been reviewed on the mailing list.  It
> > > > > wasn't sneaked in, or whatever you are trying to imply here.  Despite the
> > > > > fact that I *still* think it's far better than the proposed alternatives, I
> > > > > am very willing to discuss it.  What really happened is that you noticed
> > > > > the patch *after* the PR from Heinrich was merged.  The patch was reviewed,
> > > > > it has documentation on how to use it and it was tested on 3 different
> > > > > platforms.  So I am failing to see the "proper processes" you keep repeating.
> > > >
> > > > I am referring to:
> > > >
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20210717142648.26588-1-ilias.apalodimas@linaro.org/
> > > >
> > > > There is no indication that it was applied. In fact our discussion
> > > > apparently continued long after it was applied. I don't think even you
> > > > were aware?
> > >
> > > I think I am pretty much aware of what I respond to emails. I am also aware
> > > of the dates and things I am pointing out.
> > >
> > > The patches were merged here: https://lore.kernel.org/u-boot/f55c615b-6f94-2828-3e5e-5a7cfaaab7ea@gmx.de/
> > > and you responded 2 days after the merge. I even pointed out the fact in
> > > IRC and email, but you kept ignoring it.
> >
> > This does not match with my understanding, which perhaps explains part
> > of the disconnect. Let me explain how I saw it:
> >
> > That is a pull request, not an 'Applied to EFI' response to the patch,
> > which is the normal procedure. The pull request was not even copied to
> > me, so I'm not sure how I should have known, even if I did have time
> > to dig through it and look for patches still under discussion I had no
> > idea this had been merged and that should be obvious from the fact
> > that you had to tell me a week later.
> >
> > If you really want to dig in, the original patch was here:
> > https://patchwork.ozlabs.org/project/uboot/patch/20210715170030.97758-1-ilias.apalodimas@linaro.org/
> >
> > I queried your comment about the fdt command and got a partial
> > response from you which did not address my question properly. Before I
> > had time to follow up, a v2 patch was sent on the 17th. Apparently it
> > was applied the next day without the 'Applied to' response. Presumably
> > Heinrich did not know at this point that there was an open question. I
> > found the v2 patch on the 20th and asked my question again, not
> > knowing it was applied.
> >
> > So can we put that point to bed?
>
> As long as you understand this wasn't some effort to merge something you
> objected to, sure.

I'm not suggesting that, just that it was done too soon.

>
> >
> > >
> > > >
> > > > 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...

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

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 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.

>
> > 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.

>
> >
> > >
> > > > 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.

>
> > 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.

Do you have a link for Herinrich's concerns?

Regards,
SImon