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

[Daniele Alessandrelli]

On Wed, 2020-05-27 at 20:59 +0200, Arnd Bergmann wrote:
> On Wed, May 27, 2020 at 7:43 PM Daniele Alessandrelli
> <daniele.alessandrelli@xxxxxxxxxxxxxxx> wrote:
> > 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:
> > > 
> > > 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.
> 
> I think the first approach is more common.
> 
> > However, I think that in the second case the driver must be
> > builtin.
> > Would that be okay?
> 
> It's better to avoid that.
> 
> > 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?
> 
> I'm not sure if memblock_free() works after early memory
> initialization
> is complete, but I think there is some way to do it later. Maybe try
> memblock_free() first, and then look for something else if it doesn't
> work.
> 

Brilliant. I will create a new patch using the 1st approach and see if
memblock_free() works; if not, I will look for something else.

Thanks a lot for your valuable feedback and advice.

> > > 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?
> 
> I think you should try to fix the driver rather than making an
> exception for it.

Yes, we'll look into that.

> Hot-pluggable drivers are a much more interesting
> case I think, because on the one hand you have no idea what
> users might want to plug in legitimately, but on the other hand
> those are also the most likely attack vectors (driver bugs for
> random USB drivers overwriting kernel memory when faced with
> malicious hardware) that this feature is trying to prevent.
> 
> I also wonder whether we should do something in the normal
> iommu code that prevents one from mapping a page that the
> kernel would consider as protected (kernel .text, freed memory,
> ...) into the iommu in the first place.

Sounds like an iteresting security feature; expecially because it would
apply to any hardware.

> 
>         Arnd