This series aim to kill a recent class of bug, the infamous
"DMA reentrancy" issues found by Alexander while fuzzing.

Introduce the 'memory' field in MemTxAttrs, allowing to restrict
a controller to memories (and not devices).

If a transaction permission is not allowed (for example access
to 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().

Since v2 [1]:
- Addressed review comments:
  - reword arm_gicv3 description (pm215)
  - merged patches 3/4/5 (peterx & dhildenb)
  - simplify flatview_access_allowed() logic (stefanha)
  - drop MEMTXPERM enum and reword following AMBA terminology (edgar)

Since v1 ("hw: Forbid DMA write accesses to MMIO regions") [2]:
- rewrite based on Peter / Stefan feedbacks

Based on "hw: Let the DMA API take a MemTxAttrs argument" [3].

Based-on: <20210702092439.989969-1-philmd@redhat.com>

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg831168.html
[2] https://www.mail-archive.com/qemu-block@nongnu.org/msg72924.html
[3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg820359.html

Philippe Mathieu-Daudé (3):
  hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR
  softmmu/physmem: Simplify flatview_write and
    address_space_access_valid
  softmmu/physmem: Introduce MemTxAttrs::memory field and
    MEMTX_BUS_ERROR

 include/exec/memattrs.h    |  9 +++++++
 hw/intc/arm_gicv3_redist.c |  4 +--
 softmmu/physmem.c          | 54 +++++++++++++++++++++++++++++++-------
 3 files changed, 55 insertions(+), 12 deletions(-)

-- 
2.33.1

On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote:
> This series aim to kill a recent class of bug, the infamous
> "DMA reentrancy" issues found by Alexander while fuzzing.

I took a look at how to protect DMA transactions in VIRTIO devices. It
will require setting the MemTxAttrs for address_space_ld/st_le/be_cached
calls. Errors on write (store) can be ignored. Errors on read (load) are
a bit more questionable since the device performs some operation based
on the loaded value, but at this point the driver has already caused the
device to do something no correct driver does (as of today, it could
change in the future...) so undefined device behavior might be okay.

It would be easier to be confident if there was a single place to
disable DMA re-entrancy for a device. The currently proposed API
requires per-device code audits and fixes. It leaves decisions to the
developer of each device. This will be a lot of work to fix and we
cannot be confident that everything has been covered since this is an
opt-in mechanism.

For these reasons it seems likely that DMA re-entrancy issues will
continue to creep in. I think the only way to rule out this class of
bugs is to implement a centralized change that doesn't involve fixing
every DMA access in QEMU.

Thoughts?

Stefan

On 220124 1630, Stefan Hajnoczi wrote:
> On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote:
> > This series aim to kill a recent class of bug, the infamous
> > "DMA reentrancy" issues found by Alexander while fuzzing.
> 
> I took a look at how to protect DMA transactions in VIRTIO devices. It
> will require setting the MemTxAttrs for address_space_ld/st_le/be_cached
> calls. Errors on write (store) can be ignored. Errors on read (load) are
> a bit more questionable since the device performs some operation based
> on the loaded value, but at this point the driver has already caused the
> device to do something no correct driver does (as of today, it could
> change in the future...) so undefined device behavior might be okay.
> 
> It would be easier to be confident if there was a single place to
> disable DMA re-entrancy for a device. The currently proposed API
> requires per-device code audits and fixes. It leaves decisions to the
> developer of each device. This will be a lot of work to fix and we
> cannot be confident that everything has been covered since this is an
> opt-in mechanism.
> 
> For these reasons it seems likely that DMA re-entrancy issues will
> continue to creep in. I think the only way to rule out this class of
> bugs is to implement a centralized change that doesn't involve fixing
> every DMA access in QEMU.
> 
> Thoughts?

Hi Stefan,
Do you have some ideas about how to do this centrally?
There were at least two attempts to do this in a centralized way, but it
seems there is some worry that edge cases will break. However, I'm
not sure there were any concrete examples of such breakages.

[1] https://lore.kernel.org/all/20210824120153.altqys6jjiuxh35p@sirius.home.kraxel.org/
[2] https://lore.kernel.org/all/20211217030858.834822-1-alxndr@bu.edu/
(AFAIK Neither handles the BH->DMA->MMIO case, at the moment)

-Alex

> 
> Stefan