[PATCH 1/3 v2] efi_capsule: Move signature from DTB to .rodata

[Ilias Apalodimas]

On Thu, 22 Jul 2021 at 16:30, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 22 Jul 2021 at 07:28, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 21 Jul 2021 at 00:42, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, 20 Jul 2021 at 20:42, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 20 Jul 2021 at 07:32, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Tue, 20 Jul 2021 at 18:20, Ilias Apalodimas <ilias.apalodimas at linaro.org> wrote:
> > > > >>
> > > > >> Hi Simon,
> > > > >> On Tue, 20 Jul 2021 at 15:33, Simon Glass <sjg at chromium.org> wrote:
> > > > >> >
> > > > >> > Hi Ilias,
> > > > >> >
> > > > >> > On Sat, 17 Jul 2021 at 08:27, Ilias Apalodimas
> > > > >> > <ilias.apalodimas at linaro.org> wrote:
> > > > >> > >
> > > > >> > > The capsule signature is now part of our DTB.  This is problematic when a
> > > > >> > > user is allowed to change/fixup that DTB from U-Boots command line since he
> > > > >> > > can overwrite the signature as well.
> > > > >> >
> > > > >> > Just to repeat my question since it looks like I didn't get a response
> > > > >> > on the last patch:
> > > > >> >
> > > > >> > Do you mean with the 'fdt' command?
> > > > >> > >
> > > > >> > If you mean the FDT fixups, they happen to a different DT, the one
> > > > >> > being passed to Linux.
> > > > >>
> > > > >> In some platforms the key is derived from the relocated DTB, which we
> > > > >> can overwrite. But I'll let Sughosh who figured it out explain the
> > > > >> details.
> > > > >
> > > > >
> > > > > On platforms where the dtb is concatenated with the u-boot image, using CONFIG_OF_SEPARATE, the fdt is also getting relocated to the main memory. We retrieve the public key from this dtb. By default, the fdtcontroladdr env variable is getting set to this relocated dtb address -- this address can also be accessed using the bdinfo command. Thus the public key can be modified before attempting the capsule update. Which is the reason why Ilias is moving the public key to the embedded rodata section.
> > > >
> > > > You should be clearer about what problem you are trying to solve. Are
> > > > you worried about a script changing the DT? Or just it being writable
> > > > in general?
> > >
> > > Being writable in general is my main concern. Doing fixup internally
> > > from U-Boot might be something we'll always need but the ability to
> > > completely change it doesn't play well security.
> > >
> > > >
> > > > U-Boot itself is relocated also, including the rodata. So are you
> > > > using the public key from the original location? What if that is not
> > > > accessible after relocation?
> > >
> > > We are accessing he key from the relocated address.
> >
> > Then in what way are you protecting it? This is so confusing. Are you
> > saying that you are protecting the relocated address? If so, protect
> > the relocated devicetree too!
> >

How? DTBs if fixed up and there's a protocol proposal from Heinrich,
which allows fixups from GRUB2.  So how exactly are you going to put
it in r/o memory (which is what .rodata is supposed to achieve).
A big portion of the DTBs we build today are horribly outdated
compared to the current upstream.  Since nowdays there's a spec
describing what can and can't go in a DTB, I'd much rather prefer we
stick to that and make a potential update easier.

Regards
/Ilias

> > >
> > > >
> > > > There is also the 'fdt addr -c' command to find the control DT. It is
> > > > not expected to be written to though. So just protect the memory to
> > > > which it is relocated, or relocate it to a place that you can protect.
> > >
> > > Can you define 'protect'? The mmu support in U-Boot is kind of limited
> > > from what I can see.  In order to protect anything we'd have to switch
> > > the pages ro R-- or RX-.  Someone please shout if I  am wrong, but I
> > > couldn't find code doing that in U-Boot.
> > >
> > > >
> > > > If the DT is writable it will affect U-Boot's operation, since that is
> > > > where all the config is stored. There is no point in pretending that
> > > > pulling one thing out of it and protecting it will result in any sort
> > > > of improvement. This needs to be done properly.
> > >
> > > Tbh I thought the relocation was done properly and the .rodata section
> > > was either merged with .text (and was RX-) or R--.
> > > If we fix the relocation properly, the .rodata will be read only,
> > > while the DTB should still be on RW memory.   I understand that you
> > > are using the DTB for some configuration on U-Boot ,but arguably the
> > > public key of the EFI firmware you need to authenticate hardly
> > > classifies as configuration.
> >
> > What is it then?
>
> Please just do this properly. Add an API to protect memory, implement
> it for your chosen platform and then we will actually fix the problem
> you are worried about. Until then, work-arounds and hacks are
> pointless and just confuse the issue. Most haste, less speed.
>
> >
> > > Keep in mind that even if we fix the page tables permissions, on a
> > > secure platform the command line should be disabled, since anyone
> > > could load an arbitrary application and modify them.
> >
> > Yes we brought in CONFIG_CMDLINE for that reason about five years ago :-)
>
> Regards,
> SImon