On 06/05/2023 11.25, Song Gao wrote:
  Hi Alexander

在 2023/4/28 下午5:14, Thomas Huth 写道:
On 28/04/2023 11.11, Alexander Bulekov wrote:
On 230428 1015, Thomas Huth wrote:
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?

Sounds good - should I respin the series to change this?

Not necessary, I've got v10 already queued, I'll fix it up there

 Thomas

This patch causes the loongarch virtual machine to fail to start the slave cpu.

     ./build/qemu-system-loongarch64 -machine virt -m 8G -cpu la464 \
              -smp 4 -bios QEMU_EFI.fd -kernel vmlinuz.efi -initrd ramdisk   \
                -serial stdio   -monitor telnet:localhost:4495,server,nowait  \
               -append "root=/dev/ram rdinit=/sbin/init console=ttyS0,115200"   --nographic


....
qemu-system-loongarch64: warning: Blocked re-entrant IO on MemoryRegion: loongarch_ipi_iocsr at addr: 0x24

Oh, another spot that needs special handling ... I see Alexander already sent a patch (thanks!), but anyway, this is a good indication that we're missing some test coverage in the CI.

Are there any loongarch kernel images available for public download somewhere? If so, we really should add an avocado regression test for this - since as far as I can see, we don't have any tests for loongarch in tests/avocado yet?

 Thomas


Reply via email to