[PATCH v2 0/3] Fix dma-reentrancy issues

[PATCH v2 0/3] Fix dma-reentrancy issues

[Alexander Bulekov]

A shot at fixing dma-reentrancy issues.

Patch 1 adds a flag to track device IO activity to DeviceState.
Patch 2 Checks/sets the flag prior to invoking MemoryRegion handlers to
prevent the mmio->dma->mmio case
Patch 3 Sets the flag in dma-related calls to prevent the bh->dma->mmio
case

The related issues are tracked here: https://gitlab.com/qemu-project/qemu/-/issues/556
There is also a related whitepaper: https://qiuhao.org/Matryoshka_Trap.pdf

Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Mauro Matteo Cascella <mcascell@redhat.com>
Cc: Qiuhao Li <Qiuhao.Li@outlook.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Li Qiang <liq3ea@gmail.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: Bandan Das <bsd@redhat.com>
Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Cc: Darren Kenny <darren.kenny@oracle.com>
Cc: Bin Meng <bin.meng@windriver.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>

Alexander Bulekov (3):
  memory: Track whether a Device is engaged in IO
  memory: fix PIO/MMIO-initiated dma-reentracy issues
  memory: fix bh-initiated dma-reentracy issues

 include/hw/pci/pci.h   | 13 +++++++++++--
 include/hw/qdev-core.h |  3 +++
 softmmu/dma-helpers.c  | 12 ++++++++++++
 softmmu/memory.c       | 15 +++++++++++++++
 softmmu/trace-events   |  1 +
 5 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Alexander Bulekov]

Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag should be 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 DMA reentrancy issues. E.g.:
sdhci pio -> dma write -> sdhci mmio
nvme bh -> dma write -> nvme mmio

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

Assumptions:
 * Devices do not interact with their own PIO/MMIO memory-regions using
   DMA.

 * There is now way for there to be multiple simultaneous accesses to a
   device's PIO/MMIO memory-regions, or for multiple threads to perform
   DMA accesses simultaneously on behalf of a single device.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 include/hw/qdev-core.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 92c3d65208..6474dc51fa 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -193,6 +193,9 @@ struct DeviceState {
     int instance_id_alias;
     int alias_required_for_version;
     ResettableState reset;
+
+    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+    int engaged_in_io;
 };
 
 struct DeviceListener {
-- 
2.33.0

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Darren Kenny]

Hi Alex,

I don't know this code well enough to be certain, but is a flag
sufficient here given the intent, or should it be using a more
thread-safe method like a rwlock or condition variable?

Maybe the device state structure is already protected at some level
with a mutex - just not obvious to me from these changes...

Thanks,

Darren.

On Friday, 2022-05-27 at 12:19:35 -04, Alexander Bulekov wrote:
> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> This flag should be 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 DMA reentrancy issues. E.g.:
> sdhci pio -> dma write -> sdhci mmio
> nvme bh -> dma write -> nvme mmio
>
> These issues have led to problems such as stack-exhaustion and
> use-after-frees.
>
> Assumptions:
>  * Devices do not interact with their own PIO/MMIO memory-regions using
>    DMA.
>
>  * There is now way for there to be multiple simultaneous accesses to a
>    device's PIO/MMIO memory-regions, or for multiple threads to perform
>    DMA accesses simultaneously on behalf of a single device.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  include/hw/qdev-core.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..6474dc51fa 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -193,6 +193,9 @@ struct DeviceState {
>      int instance_id_alias;
>      int alias_required_for_version;
>      ResettableState reset;
> +
> +    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
> +    int engaged_in_io;
>  };
>  
>  struct DeviceListener {
> -- 
> 2.33.0

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Peter Maydell]

On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote:
>
> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> This flag should be 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 DMA reentrancy issues. E.g.:
> sdhci pio -> dma write -> sdhci mmio
> nvme bh -> dma write -> nvme mmio
>
> These issues have led to problems such as stack-exhaustion and
> use-after-frees.
>
> Assumptions:
>  * Devices do not interact with their own PIO/MMIO memory-regions using
>    DMA.

If you're trying to protect against malicious guest-controlled
DMA operations, you can't assume that. The guest can program
a DMA controller to DMA to its own MMIO register bank if it likes.

>  * There is now way for there to be multiple simultaneous accesses to a
>    device's PIO/MMIO memory-regions, or for multiple threads to perform
>    DMA accesses simultaneously on behalf of a single device.

This one is generally true because device code runs with
the iothread lock held.

-- PMM

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Alexander Bulekov]

On 220530 1219, Peter Maydell wrote:
> On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote:
> >
> > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > This flag should be 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 DMA reentrancy issues. E.g.:
> > sdhci pio -> dma write -> sdhci mmio
> > nvme bh -> dma write -> nvme mmio
> >
> > These issues have led to problems such as stack-exhaustion and
> > use-after-frees.
> >
> > Assumptions:
> >  * Devices do not interact with their own PIO/MMIO memory-regions using
> >    DMA.
> 
> If you're trying to protect against malicious guest-controlled
> DMA operations, you can't assume that. The guest can program
> a DMA controller to DMA to its own MMIO register bank if it likes.

If this is the case, then it seems the only way to fix this class of
problems is to rewrite device code so that it is safe for re-entrancy.
That seems to require significant upfront work to support behavior that
is often not even specified.
Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous,
incomplete solution.

Can we disable re-entracy by default, to fix the security issues, and
allow device code to "opt-in" to re-entrancy?

-Alex

> 
> >  * There is now way for there to be multiple simultaneous accesses to a
> >    device's PIO/MMIO memory-regions, or for multiple threads to perform
> >    DMA accesses simultaneously on behalf of a single device.
> 
> This one is generally true because device code runs with
> the iothread lock held.
> 
> -- PMM

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Peter Maydell]

On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote:
>
> On 220530 1219, Peter Maydell wrote:
> > On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote:
> > >
> > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > > This flag should be 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 DMA reentrancy issues. E.g.:
> > > sdhci pio -> dma write -> sdhci mmio
> > > nvme bh -> dma write -> nvme mmio
> > >
> > > These issues have led to problems such as stack-exhaustion and
> > > use-after-frees.
> > >
> > > Assumptions:
> > >  * Devices do not interact with their own PIO/MMIO memory-regions using
> > >    DMA.
> >
> > If you're trying to protect against malicious guest-controlled
> > DMA operations, you can't assume that. The guest can program
> > a DMA controller to DMA to its own MMIO register bank if it likes.
>
> If this is the case, then it seems the only way to fix this class of
> problems is to rewrite device code so that it is safe for re-entrancy.
> That seems to require significant upfront work to support behavior that
> is often not even specified.
> Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous,
> incomplete solution.
>
> Can we disable re-entracy by default, to fix the security issues, and
> allow device code to "opt-in" to re-entrancy?

That's a different question, ie "are there legitimate cases where
devices try to DMA to themselves?". I don't know the answer, but
I suspect not.

-- PMM

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Philippe Mathieu-Daudé via]

On 30/5/22 15:28, Peter Maydell wrote:
> On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote:
>>
>> On 220530 1219, Peter Maydell wrote:
>>> On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote:
>>>>
>>>> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
>>>> This flag should be 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 DMA reentrancy issues. E.g.:
>>>> sdhci pio -> dma write -> sdhci mmio
>>>> nvme bh -> dma write -> nvme mmio
>>>>
>>>> These issues have led to problems such as stack-exhaustion and
>>>> use-after-frees.
>>>>
>>>> Assumptions:
>>>>   * Devices do not interact with their own PIO/MMIO memory-regions using
>>>>     DMA.
>>>
>>> If you're trying to protect against malicious guest-controlled
>>> DMA operations, you can't assume that. The guest can program
>>> a DMA controller to DMA to its own MMIO register bank if it likes.
>>
>> If this is the case, then it seems the only way to fix this class of
>> problems is to rewrite device code so that it is safe for re-entrancy.
>> That seems to require significant upfront work to support behavior that
>> is often not even specified.
>> Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous,
>> incomplete solution.
>>
>> Can we disable re-entracy by default, to fix the security issues, and
>> allow device code to "opt-in" to re-entrancy?
> 
> That's a different question, ie "are there legitimate cases where
> devices try to DMA to themselves?". I don't know the answer, but
> I suspect not.

There is a niche where it might not be legitimate, but it is (ab)used
and Paolo wants to keep such cases working. I already responded to
Alexander here:
https://lore.kernel.org/qemu-devel/380ea0e5-a006-c570-4ec8-d67e837547ee@redhat.com/

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Alexander Bulekov]

On 220530 1539, Philippe Mathieu-Daudé wrote:
> On 30/5/22 15:28, Peter Maydell wrote:
> > On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote:
> > > 
> > > On 220530 1219, Peter Maydell wrote:
> > > > On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote:
> > > > > 
> > > > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > > > > This flag should be 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 DMA reentrancy issues. E.g.:
> > > > > sdhci pio -> dma write -> sdhci mmio
> > > > > nvme bh -> dma write -> nvme mmio
> > > > > 
> > > > > These issues have led to problems such as stack-exhaustion and
> > > > > use-after-frees.
> > > > > 
> > > > > Assumptions:
> > > > >   * Devices do not interact with their own PIO/MMIO memory-regions using
> > > > >     DMA.
> > > > 
> > > > If you're trying to protect against malicious guest-controlled
> > > > DMA operations, you can't assume that. The guest can program
> > > > a DMA controller to DMA to its own MMIO register bank if it likes.
> > > 
> > > If this is the case, then it seems the only way to fix this class of
> > > problems is to rewrite device code so that it is safe for re-entrancy.
> > > That seems to require significant upfront work to support behavior that
> > > is often not even specified.
> > > Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous,
> > > incomplete solution.
> > > 
> > > Can we disable re-entracy by default, to fix the security issues, and
> > > allow device code to "opt-in" to re-entrancy?
> > 
> > That's a different question, ie "are there legitimate cases where
> > devices try to DMA to themselves?". I don't know the answer, but
> > I suspect not.
> 
> There is a niche where it might not be legitimate, but it is (ab)used
> and Paolo wants to keep such cases working. I already responded to
> Alexander here:
> https://lore.kernel.org/qemu-devel/380ea0e5-a006-c570-4ec8-d67e837547ee@redhat.com/

I'm not sure we confirmed that this is actually an example of a device
performing DMA to its own MMIO. Unless I am missing something, the BLOAD
example simply performs repeated writes to VRAM?

That said video-related devices seem like possible candidates where such
behavior is conceivable. But even in those cases, the memory regions
would likely be ram/rom devices (which are excluded from the re-entrancy
check). 

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[Alexander Bulekov]

On 220530 1428, Peter Maydell wrote:
> On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote:
> >
> > On 220530 1219, Peter Maydell wrote:
> > > On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote:
> > > >
> > > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > > > This flag should be 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 DMA reentrancy issues. E.g.:
> > > > sdhci pio -> dma write -> sdhci mmio
> > > > nvme bh -> dma write -> nvme mmio
> > > >
> > > > These issues have led to problems such as stack-exhaustion and
> > > > use-after-frees.
> > > >
> > > > Assumptions:
> > > >  * Devices do not interact with their own PIO/MMIO memory-regions using
> > > >    DMA.
> > >
> > > If you're trying to protect against malicious guest-controlled
> > > DMA operations, you can't assume that. The guest can program
> > > a DMA controller to DMA to its own MMIO register bank if it likes.
> >
> > If this is the case, then it seems the only way to fix this class of
> > problems is to rewrite device code so that it is safe for re-entrancy.
> > That seems to require significant upfront work to support behavior that
> > is often not even specified.
> > Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous,
> > incomplete solution.
> >
> > Can we disable re-entracy by default, to fix the security issues, and
> > allow device code to "opt-in" to re-entrancy?
> 
> That's a different question, ie "are there legitimate cases where
> devices try to DMA to themselves?". I don't know the answer, but
> I suspect not.

Ah, I worded the assumption incorrectly. Should be
 * There is no valid use-case for a device to interact with its own
   PIO/MMIO memory-regions using DMA.

> 
> -- PMM

Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO

[David Hildenbrand]

On 27.05.22 18:19, Alexander Bulekov wrote:
> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> This flag should be 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 DMA reentrancy issues. E.g.:
> sdhci pio -> dma write -> sdhci mmio
> nvme bh -> dma write -> nvme mmio
> 
> These issues have led to problems such as stack-exhaustion and
> use-after-frees.
> 
> Assumptions:
>  * Devices do not interact with their own PIO/MMIO memory-regions using
>    DMA.
> 
>  * There is now way for there to be multiple simultaneous accesses to a
>    device's PIO/MMIO memory-regions, or for multiple threads to perform
>    DMA accesses simultaneously on behalf of a single device.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

I think this patch should be squashed into the other ones, it doesn't
make particular sense without any actual users.


-- 
Thanks,

David / dhildenb

[PATCH v2 2/3] memory: fix PIO/MMIO-initiated dma-reentracy issues

[Alexander Bulekov]

Set/check the engaged_in_io DeviceState flag, prior to calling
MemoryRegion handlers.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 softmmu/memory.c     | 15 +++++++++++++++
 softmmu/trace-events |  1 +
 2 files changed, 16 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 7ba2048836..44a14bb4f5 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -532,6 +532,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     uint64_t access_mask;
     unsigned access_size;
     unsigned i;
+    DeviceState *dev = NULL;
     MemTxResult r = MEMTX_OK;
 
     if (!access_size_min) {
@@ -541,6 +542,17 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
         access_size_max = 4;
     }
 
+    /* Do not allow more than one simultanous access to a device's IO Regions */
+    if (mr->owner &&
+        !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+        dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
+        if (dev->engaged_in_io) {
+            trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+            return MEMTX_ERROR;
+        }
+        dev->engaged_in_io = true;
+    }
+
     /* FIXME: support unaligned access? */
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -555,6 +567,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
                         access_mask, attrs);
         }
     }
+    if (dev) {
+        dev->engaged_in_io = false;
+    }
     return r;
 }
 
diff --git a/softmmu/trace-events b/softmmu/trace-events
index 9c88887b3c..d7228316db 100644
--- a/softmmu/trace-events
+++ b/softmmu/trace-events
@@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, u
 memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u name '%s'"
 memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u"
+memory_region_reentrant_io(int cpu_index, void *mr, uint64_t offset, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" size %u"
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)"
-- 
2.33.0

[PATCH v2 3/3] memory: fix bh-initiated dma-reentracy issues

[Alexander Bulekov]

This patch leverages the DeviceState engaged_in_io flag to prevent
issues due to accesses similar to bh -> dma -> mmio.
e.g.  CVE-2021-3929

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 include/hw/pci/pci.h  | 13 +++++++++++--
 softmmu/dma-helpers.c | 12 ++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 44dacfa224..ab1ad0f7a8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -834,8 +834,17 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
                                      void *buf, dma_addr_t len,
                                      DMADirection dir, MemTxAttrs attrs)
 {
-    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
-                         dir, attrs);
+    bool prior_engaged_state;
+    MemTxResult result;
+
+    prior_engaged_state = dev->qdev.engaged_in_io;
+
+    dev->qdev.engaged_in_io = true;
+    result = dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
+                           dir, attrs);
+    dev->qdev.engaged_in_io = prior_engaged_state;
+
+    return result;
 }
 
 /**
diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
index 7820fec54c..7a4f1fb9b3 100644
--- a/softmmu/dma-helpers.c
+++ b/softmmu/dma-helpers.c
@@ -288,8 +288,16 @@ static MemTxResult dma_buf_rw(void *buf, dma_addr_t len, dma_addr_t *residual,
     uint8_t *ptr = buf;
     dma_addr_t xresidual;
     int sg_cur_index;
+    DeviceState *dev;
+    bool prior_engaged_state;
     MemTxResult res = MEMTX_OK;
 
+    dev = sg->dev;
+    if (dev) {
+        prior_engaged_state = dev->engaged_in_io;
+        dev->engaged_in_io = true;
+    }
+
     xresidual = sg->size;
     sg_cur_index = 0;
     len = MIN(len, xresidual);
@@ -302,6 +310,10 @@ static MemTxResult dma_buf_rw(void *buf, dma_addr_t len, dma_addr_t *residual,
         xresidual -= xfer;
     }
 
+    if (dev) {
+        dev->engaged_in_io = prior_engaged_state;
+    }
+
     if (residual) {
         *residual = xresidual;
     }
-- 
2.33.0