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

[Simon Glass <sjg@chromium.org>]

+U-Boot Mailing List which I seemed to drop, sorry


On Tue, 24 Aug 2021 at 10:19, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Tue, 24 Aug 2021 at 00:05, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > > > > > >>
> >
> > [...]
> >
> > > > > > >> I don't think we should wait any longer for this as we are already at rc2.
> > > > > > >>
> > > > > > >> I'd like to see my revert patches (or something similar) applied ASAP
> > > > > > >> please. We can then figure out a better solution.
> > > > > > >
> > > > > > > Heinrich feel free to revert those, but keep in mind that will break
> > > > > > > authenticated capsules for anything apart from qemu (and also U-Boot
> > > > > > > compilation if that feature is selected).  We'll also have to carry the
> > > > > > > mkeficapsule code, until we document Akashi's changes for injecting the key
> > > > > > > in the dtb, without using that application.
> > > > > > >
> > > > > > > Alternatively we could just put the key in an authenticated variable
> > > > > > > (perhaps with it's own GUID?).  I'll go back and check EDK2, but I think
> > > > > > > that's what they've implemented.
> > > > > > >
> > > > > > > Regards
> > > > > > > /Ilias
> > > > > >
> > > > > > Simon,
> > > > > >
> > > > > > as Ilias pointed out you series "efi: Minimal revert to rodata change "
> > > > > > https://patchwork.ozlabs.org/project/uboot/list/?series=256272 is
> > > > > > breaking capsule updates on non-QEMU boards.
> > > > >
> > > > > Isn't that what was just implemented? I am happy for you to apply a
> > > > > different revert. I was just responding to what Ilias said needed to
> > > > > be reverted.
> > > > >
> > > > > >
> > > > > > Furthermove your proposal does not work securitywise: The fdt command is
> > > > > > needed to apply overlays and cannot be removed from all boards using
> > > > > > capsule updates. Putting validation keys into the fdt would require to
> > > > > > disable that the fdt command can change the capsule verification key.
> > > > >
> > > > > That is one of the many points Ilias and I discussed in the call.
> > > > > There are many solutions to that and many other problems to solve
> > > > > (e.g. the mw command and adding memory protection). These issues are
> > > > > not going to be solved immediately, particularly on a very new
> > > > > feature.
> > > > >
> > > > > >
> > > > > > So it seems you series is not in a state for being merged.
> > > > >
> > > > > A revert takes us back to where we were so that the path forward can
> > > > > be determined. A revert does not, by its nature, reimplement a
> > > > > feature. It is a revert.
> > > > >
> > > > > Please can you accept the revert or tell me what you will accept, or
> > > > > come up with another plan to revert this. This is taking far too much
> > > > > energy and, as I said, should never have happened if proper processes
> > > > > were followed.
> > > > >
> > > >
> > > > 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?
>
> >
> > >
> > > 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. 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. 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? What if someone wants their own 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.
>
> >
> > > 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.
>
> >
> > > 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?
>
> 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.
>
> Regards,
> Simon