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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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