Re: Pull request for efi-2021-10-rc2-2
+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-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?
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...
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.
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#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?
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
[...]
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...
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.
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#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?
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
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...
> > 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.
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#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?
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@c...
Regards, Simon
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... > > > > > 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.
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#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?
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@c...
Regards, Simon
Regards /Ilias
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... > > > > > > > > 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.
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
participants (2)
-
Ilias Apalodimas -
Simon Glass