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

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

[Simon Glass]

+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

Re: 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@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

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

[Ilias Apalodimas]

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

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

[Simon Glass]

Hi Ilias,

On Wed, 25 Aug 2021 at 23:51, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> [...]
> > >
> > > >
> > > > >
> > > > > >
> > > > > > 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.

Heinrich asked the same question. I went as far as sending a patch
with docs on how it should work.

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

I don't see why...where does the DT come from on that board?

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

I am not sure about mkeficapsule, but we already have the tool and the
approach you have sent does not fit without how U-Boot should work.

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

Let's just agree to disagree on that.

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

See my docs patch on how that should be handled. However I am not sure
what the prior stage is, in the RISC-V case. Are you just referring to
QEMU? If so, I addressed that in my patch.

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

+Heinrich Schuchardt

There seem to be multiple cases with different approaches of supplying
the device tree. Heinrich was so confused he asked me to write some
docs about it all. I had to do some digging to understand it even as
well as I do. So it isn't obvious nor simple.

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

OK I addressed that in my doc patch too. It leads to crazy town...

http://patchwork.ozlabs.org/project/uboot/patch/20210828164630.81050-4-sjg@chromium.org/

Regards,
Simon

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

[Ilias Apalodimas]

Hi Simon,

> > >
> > > 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.
> 
> Heinrich asked the same question. I went as far as sending a patch
> with docs on how it should work.
> 

I've commented on that patch as well wrt DTB origin.

> >
> > >
> > > 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).
> 
> I don't see why...where does the DT come from on that board?

From a previous bootloader. It's address is usually is one of the cpu
registers.  The most cases I've seen in u-boot is boards using their
version of board_fdt_blob_setup().

> 
> >
> > >
> > > 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.
> 
> I am not sure about mkeficapsule, but we already have the tool and the
> approach you have sent does not fit without how U-Boot should work.
> 
> >
> > >
> > > >
> > > > > 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.
> 
> Let's just agree to disagree on that.
> 
> >
> > > >
> > > > > 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 .
> 
> See my docs patch on how that should be handled. However I am not sure
> what the prior stage is, in the RISC-V case. Are you just referring to
> QEMU? If so, I addressed that in my patch.

If I don't read U-Boot wrong, there's more boards than QEMU (an even more
to come). What about boards using board_fdt_blob_setup() I've just mentioned?

> 
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > 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.
> 
> +Heinrich Schuchardt
> 
> There seem to be multiple cases with different approaches of supplying
> the device tree. Heinrich was so confused he asked me to write some
> docs about it all. I had to do some digging to understand it even as
> well as I do. So it isn't obvious nor simple.
> 
> >
> > > >
> > > > > 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.
> 
> OK I addressed that in my doc patch too. It leads to crazy town...
> 
> http://patchwork.ozlabs.org/project/uboot/patch/20210828164630.81050-4-sjg@chromium.org/
> 
> Regards,
> Simon


Regards
/Ilias

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

[Simon Glass]

Hi Ilias,

On Mon, 6 Sept 2021 at 05:59, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> > > >
> > > > 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.
> >
> > Heinrich asked the same question. I went as far as sending a patch
> > with docs on how it should work.
> >
>
> I've commented on that patch as well wrt DTB origin.

Yes I'll take that up there. I'm on holiday for two more weeks so
things are a bit quiet.

>
> > >
> > > >
> > > > 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).
> >
> > I don't see why...where does the DT come from on that board?
>
> From a previous bootloader. It's address is usually is one of the cpu
> registers.  The most cases I've seen in u-boot is boards using their
> version of board_fdt_blob_setup().
>
> >
> > >
> > > >
> > > > 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.
> >
> > I am not sure about mkeficapsule, but we already have the tool and the
> > approach you have sent does not fit without how U-Boot should work.
> >
> > >
> > > >
> > > > >
> > > > > > 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.
> >
> > Let's just agree to disagree on that.
> >
> > >
> > > > >
> > > > > > 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 .
> >
> > See my docs patch on how that should be handled. However I am not sure
> > what the prior stage is, in the RISC-V case. Are you just referring to
> > QEMU? If so, I addressed that in my patch.
>
> If I don't read U-Boot wrong, there's more boards than QEMU (an even more
> to come). What about boards using board_fdt_blob_setup() I've just mentioned?

I'm going to suggest that we pass a bloblist to U-Boot in this case,
with the DT being in there somewhere. I think that makes sense as a
standard approach, since it is extensible for the future.

[..]

Regards,
Simon

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

On Sun, Aug 15, 2021 at 06:28:54PM +0200, Heinrich Schuchardt wrote:

> Dear Tom,
> 
> The following changes since commit 85ccbf666e549f0b06c29d565b9e4fdd87cf6600:
> 
>   Merge https://source.denx.de/u-boot/custodians/u-boot-x86 (2021-08-13
> 08:37:47 -0400)
> 
> are available in the Git repository at:
> 
>   https://source.denx.de/u-boot/custodians/u-boot-efi.git
> tags/efi-2021-10-rc2-2
> 
> for you to fetch changes up to 61ee780352e054df587d8781f23b323c967b5d2a:
> 
>   efi_loader: refactor efi_append_scrtm_version() (2021-08-14 20:54:41
> +0200)
> 
> Gitlab CI showed not problems:
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/8711
> 

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Heinrich Schuchardt]

Dear Tom,

The following changes since commit 85ccbf666e549f0b06c29d565b9e4fdd87cf6600:

   Merge https://source.denx.de/u-boot/custodians/u-boot-x86 (2021-08-13
08:37:47 -0400)

are available in the Git repository at:

   https://source.denx.de/u-boot/custodians/u-boot-efi.git
tags/efi-2021-10-rc2-2

for you to fetch changes up to 61ee780352e054df587d8781f23b323c967b5d2a:

   efi_loader: refactor efi_append_scrtm_version() (2021-08-14 20:54:41
+0200)

Gitlab CI showed not problems:
https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/8711

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

Documentation:

* Require Sphinx >= 2.4.4 for 'make htmldocs'
* Move devicetree documentation to restructured text and update it
* Document stm32mp1 devicetree bindings

UEFI

* Extend measurement to UEFI variables and ExitBootServices()
* Support Uri() devicetree node in text protocol
* Add Linux magic token to RISC-V EFI test binaries

----------------------------------------------------------------
Heinrich Schuchardt (5):
       doc: add pkg-config to the build dependencies
       doc: require Sphinx 2.4.4
       doc: fix Latex margins
       efi_loader: Uri() device path node
       efi_loader: add Linux magic to RISC-V crt0

Masahisa Kojima (4):
       efi_loader: add secure boot variable measurement
       efi_loader: add boot variable measurement
       efi_loader: add ExitBootServices() measurement
       efi_loader: refactor efi_append_scrtm_version()

Patrick Delaunay (1):
       doc: stm32mp1: add page for device tree bindings

Simon Glass (3):
       doc: Move devicetree control doc to rST
       doc: Update devicedocs including how to add tweaks
       doc: Add a note about why devicetree is used

  arch/riscv/lib/crt0_riscv_efi.S                    |   7 +-
  doc/README.fdt-control                             | 230 -------------
  doc/board/st/index.rst                             |   1 +
  doc/board/st/st.rst                                |  68 ++++
  doc/build/gcc.rst                                  |   4 +-
  doc/conf.py                                        | 108 ++-----
  doc/develop/devicetree/control.rst                 | 251 +++++++++++++++
  doc/develop/devicetree/index.rst                   |  13 +
  doc/develop/devicetree/intro.rst                   |  44 +++
  doc/develop/index.rst                              |   1 +
  doc/device-tree-bindings/adc/st,stm32-adc.txt      | 141 --------
  doc/device-tree-bindings/clock/st,stm32-rcc.txt    |  95 ------
  doc/device-tree-bindings/clock/st,stm32h7-rcc.txt  | 152 ---------
  doc/device-tree-bindings/i2c/i2c-stm32.txt         |  30 --
  .../memory-controllers/st,stm32-fmc.txt            |  58 ----
  doc/device-tree-bindings/mtd/stm32-fmc2-nand.txt   |  61 ----
  doc/device-tree-bindings/phy/phy-stm32-usbphyc.txt |  75 -----
  .../pinctrl/st,stm32-pinctrl.txt                   | 208 ------------
  .../regulator/st,stm32-vrefbuf.txt                 |  23 --
  doc/device-tree-bindings/reset/st,stm32-rcc.txt    |   6 -
  doc/device-tree-bindings/spi/spi-stm32-qspi.txt    |  44 ---
  doc/sphinx/kerneldoc.py                            |  26 +-
  include/efi_api.h                                  |   6 +
  include/efi_loader.h                               |   5 +
  include/efi_tcg2.h                                 |  20 ++
  include/tpm-v2.h                                   |  18 +-
  lib/efi_loader/efi_boottime.c                      |  25 ++
  lib/efi_loader/efi_device_path_to_text.c           |  13 +
  lib/efi_loader/efi_tcg2.c                          | 356
++++++++++++++++++++-
  29 files changed, 845 insertions(+), 1244 deletions(-)
  delete mode 100644 doc/README.fdt-control
  create mode 100644 doc/board/st/st.rst
  create mode 100644 doc/develop/devicetree/control.rst
  create mode 100644 doc/develop/devicetree/index.rst
  create mode 100644 doc/develop/devicetree/intro.rst
  delete mode 100644 doc/device-tree-bindings/adc/st,stm32-adc.txt
  delete mode 100644 doc/device-tree-bindings/clock/st,stm32-rcc.txt
  delete mode 100644 doc/device-tree-bindings/clock/st,stm32h7-rcc.txt
  delete mode 100644 doc/device-tree-bindings/i2c/i2c-stm32.txt
  delete mode 100644
doc/device-tree-bindings/memory-controllers/st,stm32-fmc.txt
  delete mode 100644 doc/device-tree-bindings/mtd/stm32-fmc2-nand.txt
  delete mode 100644 doc/device-tree-bindings/phy/phy-stm32-usbphyc.txt
  delete mode 100644 doc/device-tree-bindings/pinctrl/st,stm32-pinctrl.txt
  delete mode 100644 doc/device-tree-bindings/regulator/st,stm32-vrefbuf.txt
  delete mode 100644 doc/device-tree-bindings/reset/st,stm32-rcc.txt
  delete mode 100644 doc/device-tree-bindings/spi/spi-stm32-qspi.txt