Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues

[Thomas Huth]

On 28/04/2023 10.12, Daniel P. Berrangé wrote:

On Thu, Apr 27, 2023 at 05:10:06PM -0400, Alexander Bulekov wrote:

Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA.  The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:

1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case

These issues have led to problems such as stack-exhaustion and
use-after-frees.

Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Resolves: CVE-2023-0330

Signed-off-by: Alexander Bulekov <alx...@bu.edu>
Reviewed-by: Thomas Huth <th...@redhat.com>
---
  include/exec/memory.h  |  5 +++++
  include/hw/qdev-core.h |  7 +++++++
  softmmu/memory.c       | 16 ++++++++++++++++
  3 files changed, 28 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 15ade918ba..e45ce6061f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -767,6 +767,8 @@ struct MemoryRegion {
      bool is_iommu;
      RAMBlock *ram_block;
      Object *owner;
+    /* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath 
*/
+    DeviceState *dev;

const MemoryRegionOps *ops;

      void *opaque;
@@ -791,6 +793,9 @@ struct MemoryRegion {
      unsigned ioeventfd_nb;
      MemoryRegionIoeventfd *ioeventfds;
      RamDiscardManager *rdm; /* Only for RAM */
+
+    /* For devices designed to perform re-entrant IO into their own IO MRs */
+    bool disable_reentrancy_guard;
  };

struct IOMMUMemoryRegion {

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
      QLIST_ENTRY(NamedClockList) node;
  };

+typedef struct {

+    bool engaged_in_io;
+} MemReentrancyGuard;
+
  /**
   * DeviceState:
   * @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
      int alias_required_for_version;
      ResettableState reset;
      GSList *unplug_blockers;
+
+    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+    MemReentrancyGuard mem_reentrancy_guard;
  };

struct DeviceListener {

diff --git a/softmmu/memory.c b/softmmu/memory.c
index b1a6cae6f5..fe23f0e5ce 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
          access_size_max = 4;
      }

+ /* Do not allow more than one simultaneous access to a device's IO Regions */

+    if (mr->dev && !mr->disable_reentrancy_guard &&
+        !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+        if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
+            warn_report("Blocked re-entrant IO on "
+                    "MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
+                    memory_region_name(mr), addr);
+            return MEMTX_ACCESS_ERROR;

If we issue this warn_report on every invalid memory access, is this
going to become a denial of service by flooding logs, or is the
return MEMTX_ACCESS_ERROR, sufficient to ensure this is only printed
*once* in the lifetime of the QEMU process ?

Maybe it's better to use warn_report_once() here instead?

 Thomas