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

[Simon Glass <sjg@chromium.org>]

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