Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

[Daniele Alessandrelli]

On Wed, 2020-05-27 at 16:33 +0200, Arnd Bergmann wrote:
> On Wed, May 27, 2020 at 3:31 PM Alessandrelli, Daniele
> <daniele.alessandrelli@xxxxxxxxx> wrote:
> > > > Alternatively, take that memory off the "memory available"
> > > > maps,
> > > > and only re-add it once
> > > > it is usable.
> > > > 
> > > > Anything else is dangerous.
> > 
> > That sounds like an interesting solution, thanks!
> > 
> > Do you know any code that I can use as a reference?
> > 
> > Since, the protected memory is just a few megabytes large, I guess
> > that
> > in theory we could just have U-Boot mark it as reserved memory and
> > forget about it; but if there's a way to re-add it back once in
> > Linux
> > that would be great.
> 
> Adding it back later on with a loadable device driver should
> not be a problem, as this is not a violation of the boot protocol.

Cool, I'll try to do that then, thanks!

I see two ways to do that though:

1. Create a device driver that gets a reference to the memory region
from its DT node and then re-adds the memory to the pool of available
memory.

2. Use a special "compatible" string for my memory region and create a
driver to handle it.

However, I think that in the second case the driver must be builtin.
Would that be okay?

Also, from a quick look, it seems that I can re-add that memory back by
calling memblock_free() (or a similar memblock_* function). Am I on the
right track?

> 
> > > Agreed, this sounds like an incompatible extension of the boot
> > > protocol that we should otherwise not merge.
> > > 
> > > However, there is also a lot of missing information here, and it
> > > is
> > > always possible they are trying to something for a good reason.
> > > As long as the problem that the bootloader is trying to solve is
> > > explained well enough in the changelog, we can discuss it to see
> > > how it should be done properly.
> > 
> > Apologies, I should have provided more information. Here it is :)
> > 
> > Basically, at boot time U-Boot code and core memory (.text, .data,
> > .bss, etc.) is protected by this Isolated Memory Region (IMR) which
> > prevents any device or processing units other than the ARM CPU to
> > access/modify the memory.
> > 
> > This is done for security reasons, to reduce the risks that a
> > potential
> > attacker can use "hijacked" HW devices to interfere with the boot
> > process (and break the secure boot flow in place).
> > 
> > Before booting the Kernel, U-Boot sets up a new IMR to protect the
> > Kernel image (so that the kernel can benefit of a similar
> > protection)
> > and then starts the kernel, delegating to the kernel the task of
> > switching off the U-Boot IMR.
> > 
> > U-Boot doesn't turn off its own IMR because doing so would leave a
> > (tiny) window in which the boot execution flow is not protected.
> > 
> > If you have any additional questions or feedback, just let me know.
> 
> Thank you for the detailed explanation. I've never seen this done
> in the Linux boot flow, but I can see how it helps prevent certain
> kinds of attacks.
> 
> It seems that just reserving the u-boot area and optionally
> freeing it later from a driver solves most of your problem.
> I have one related question though: if the kernel itself is
> protected, does that mean that any driver that does a DMA
> to statically allocated memory inside of the kernel is broken
> as well? I think this is something that a couple of USB drivers
> do, though it is fairly rare. Does u-boot protect both only
> the executable sections of the kernel or also data, and does
> the hardware block both read and write on the IMR, or just
> writes?

Yes, you're very right. Drivers that do DMA transfers to statically
allocated memory inside the kernel will be broken.

We are currently seeing this with our eMMC driver.

Current solution is to add the eMMC controller to the list of allowed
"agents" for the IMR. This will reduce the level of protection, but
since we expect to have only a few of these exceptions (since, as you
pointed out, drivers that do DMA to static kernel memory seem to be
quite rare), we think that there is still value in having the Kernel
IMR.

Do you see any issue with this?

Regards,
Daniele