Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1587485099.git.daniele.alessandrelli@intel.com>
 <13ca92165fab2827b6d439661e75f5b91ef083c2.1587485099.git.daniele.alessandrelli@intel.com>
 <20200501081002.GA1055721@kroah.com> <f60aece195cd0700728fc38b0398949a82b72fc3.camel@linux.intel.com>
 <20200524212851.GG1192@bug> <CAK8P3a225pqBfzQ19e6Gt0s_tYBp29xLb8EG==hhz=1wc7aVCA@mail.gmail.com>
 <ac0534138facc25c4cbcbbff68fc0ba3c2de87b6.camel@linux.intel.com>
 <CAK8P3a2MzqF3P8nY3hAUaAXhTV8ZGQd187UDbNV1GBdu+_z5-g@mail.gmail.com> <561ea2aabf919ef332ec0165351791f6148e8597.camel@linux.intel.com>
In-Reply-To: <561ea2aabf919ef332ec0165351791f6148e8597.camel@linux.intel.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Wed, 27 May 2020 20:59:01 +0200
Message-ID: <CAK8P3a0OK+BRF2t=8V6Pa95b6Ldcfn3AP1VM+GSsruGRVH=MXQ@mail.gmail.com>
Subject: Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver
To:     Daniele Alessandrelli <daniele.alessandrelli@linux.intel.com>
Cc:     "pavel@ucw.cz" <pavel@ucw.cz>, "robh@kernel.org" <robh@kernel.org>,
        "Murphy, Paul J" <paul.j.murphy@intel.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "Shevchenko, Andriy" <andriy.shevchenko@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 27, 2020 at 7:43 PM Daniele Alessandrelli
<daniele.alessandrelli@linux.intel.com> 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@intel.com> 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.

> > 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. 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.

        Arnd