+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-ilia...
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-ilia...
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...
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-ilia...
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#introduc... https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#motivati...
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