Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument

From: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "David Hildenbrand" <david@redhat.com>,
	"Jason Wang" <jasowang@redhat.com>, "Li Qiang" <liq3ea@gmail.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Peter Xu" <peterx@redhat.com>,
	"Qiuhao Li" <Qiuhao.Li@outlook.com>,
	"Alexander Bulekov" <alxndr@bu.edu>,
	qemu-arm <qemu-arm@nongnu.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [RFC PATCH v2 0/5] physmem: Have flaview API check bus permission from MemTxAttrs argument
Date: Tue, 24 Aug 2021 11:25:49 +0200	[thread overview]
Message-ID: <20210824092549.GK3586016@toto> (raw)
In-Reply-To: <CAFEAcA9_=6n4w86gD3MHhiLeCiui9_+ePT7E2sSHdSRufUJdyA@mail.gmail.com>

On Mon, Aug 23, 2021 at 08:10:50PM +0100, Peter Maydell wrote:
> On Mon, 23 Aug 2021 at 17:42, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > This series aim to kill a recent class of bug, the infamous
> > "DMA reentrancy" issues found by Alexander while fuzzing.
> >
> > Introduce the 'bus_perm' field in MemTxAttrs, defining 3 bits:
> >
> > - MEMTXPERM_UNSPECIFIED (current default, unchanged behavior)
> > - MEMTXPERM_UNRESTRICTED (allow list approach)
> > - MEMTXPERM_RAM_DEVICE (example of deny list approach)
> >
> > If a transaction permission is not allowed (for example access
> > to non-RAM device), we return the specific MEMTX_BUS_ERROR.
> >
> > Permissions are checked in after the flatview is resolved, and
> > before the access is done, in a new function: flatview_access_allowed().
> 
> So I'm not going to say 'no' to this, because we have a real
> recursive-device-handling problem and I don't have a better
> idea to hand, but the thing about this is that we end up with
> behaviour which is not what the real hardware does. I'm not
> aware of any DMA device which has this kind of "can only DMA
> to/from RAM, and aborts on access to a device" behaviour...
>

Yes, I agree.

Having said that, There are DMA devices that do indicate to the
interconnect and peripherals if they are targeting "normal" memory
or device (together with cacheability, buffering and ordering
attributes). Accessing registers of a device with "normal" memory
and cache attributes is not a good idea and may lead to all kinds
of weirdness on real HW.

IMO, it would be better to model something like those attributes
rather than permissions. And it's probably a good idea to not
call it MEMTXPERM_RAM_DEVICE since in the AMBA documentation
it's Normal Memory vs Device (calling it RAM_DEVICE is confusing
to me at least).

Adding a "memory" attribute to MemTxAttrs may be enough?
If it's set, the access would only target memories. If targeting a device
it could perhaps be logged and dropped rather than aborted?
If not set (the default), accesses would target anything...

Cheers,
Edgar