* [PATCH v2 0/3] Fix dma-reentrancy issues
@ 2022-05-27 16:19 Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 1/3] memory: Track whether a Device is engaged in IO Alexander Bulekov
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Alexander Bulekov @ 2022-05-27 16:19 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Peter Maydell, Li Qiang,
Thomas Huth, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-27 16:19 [PATCH v2 0/3] Fix dma-reentrancy issues Alexander Bulekov
@ 2022-05-27 16:19 ` Alexander Bulekov
2022-05-30 9:58 ` Darren Kenny
` (2 more replies)
2022-05-27 16:19 ` [PATCH v2 2/3] memory: fix PIO/MMIO-initiated dma-reentracy issues Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 3/3] memory: fix bh-initiated " Alexander Bulekov
2 siblings, 3 replies; 12+ messages in thread
From: Alexander Bulekov @ 2022-05-27 16:19 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Peter Maydell, Li Qiang,
Thomas Huth, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Stefan Hajnoczi,
Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] memory: fix PIO/MMIO-initiated dma-reentracy issues
2022-05-27 16:19 [PATCH v2 0/3] Fix dma-reentrancy issues Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 1/3] memory: Track whether a Device is engaged in IO Alexander Bulekov
@ 2022-05-27 16:19 ` Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 3/3] memory: fix bh-initiated " Alexander Bulekov
2 siblings, 0 replies; 12+ messages in thread
From: Alexander Bulekov @ 2022-05-27 16:19 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Peter Maydell, Li Qiang,
Thomas Huth, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] memory: fix bh-initiated dma-reentracy issues
2022-05-27 16:19 [PATCH v2 0/3] Fix dma-reentrancy issues Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 1/3] memory: Track whether a Device is engaged in IO Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 2/3] memory: fix PIO/MMIO-initiated dma-reentracy issues Alexander Bulekov
@ 2022-05-27 16:19 ` Alexander Bulekov
2 siblings, 0 replies; 12+ messages in thread
From: Alexander Bulekov @ 2022-05-27 16:19 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Peter Maydell, Li Qiang,
Thomas Huth, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Stefan Hajnoczi,
Michael S. Tsirkin, Marcel Apfelbaum
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-27 16:19 ` [PATCH v2 1/3] memory: Track whether a Device is engaged in IO Alexander Bulekov
@ 2022-05-30 9:58 ` Darren Kenny
2022-05-30 11:19 ` Peter Maydell
2022-05-30 12:13 ` David Hildenbrand
2 siblings, 0 replies; 12+ messages in thread
From: Darren Kenny @ 2022-05-30 9:58 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel
Cc: Alexander Bulekov, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Peter Maydell, Li Qiang,
Thomas Huth, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Bin Meng, Paolo Bonzini, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-27 16:19 ` [PATCH v2 1/3] memory: Track whether a Device is engaged in IO Alexander Bulekov
2022-05-30 9:58 ` Darren Kenny
@ 2022-05-30 11:19 ` Peter Maydell
2022-05-30 13:09 ` Alexander Bulekov
2022-05-30 12:13 ` David Hildenbrand
2 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2022-05-30 11:19 UTC (permalink / raw)
To: Alexander Bulekov
Cc: qemu-devel, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Li Qiang, Thomas Huth,
Laurent Vivier, Bandan Das, Edgar E . Iglesias, Darren Kenny,
Bin Meng, Paolo Bonzini, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-27 16:19 ` [PATCH v2 1/3] memory: Track whether a Device is engaged in IO Alexander Bulekov
2022-05-30 9:58 ` Darren Kenny
2022-05-30 11:19 ` Peter Maydell
@ 2022-05-30 12:13 ` David Hildenbrand
2 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2022-05-30 12:13 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel
Cc: Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
Gerd Hoffmann, Peter Maydell, Li Qiang, Thomas Huth,
Laurent Vivier, Bandan Das, Edgar E . Iglesias, Darren Kenny,
Bin Meng, Paolo Bonzini, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-30 11:19 ` Peter Maydell
@ 2022-05-30 13:09 ` Alexander Bulekov
2022-05-30 13:28 ` Peter Maydell
0 siblings, 1 reply; 12+ messages in thread
From: Alexander Bulekov @ 2022-05-30 13:09 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Li Qiang, Thomas Huth,
Laurent Vivier, Bandan Das, Edgar E . Iglesias, Darren Kenny,
Bin Meng, Paolo Bonzini, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-30 13:09 ` Alexander Bulekov
@ 2022-05-30 13:28 ` Peter Maydell
2022-05-30 13:39 ` Philippe Mathieu-Daudé via
2022-05-30 13:41 ` Alexander Bulekov
0 siblings, 2 replies; 12+ messages in thread
From: Peter Maydell @ 2022-05-30 13:28 UTC (permalink / raw)
To: Alexander Bulekov
Cc: qemu-devel, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Li Qiang, Thomas Huth,
Laurent Vivier, Bandan Das, Edgar E . Iglesias, Darren Kenny,
Bin Meng, Paolo Bonzini, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-30 13:28 ` Peter Maydell
@ 2022-05-30 13:39 ` Philippe Mathieu-Daudé via
2022-05-30 14:04 ` Alexander Bulekov
2022-05-30 13:41 ` Alexander Bulekov
1 sibling, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-05-30 13:39 UTC (permalink / raw)
To: Peter Maydell, Alexander Bulekov, Paolo Bonzini
Cc: qemu-devel, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Li Qiang, Thomas Huth,
Laurent Vivier, Bandan Das, Edgar E . Iglesias, Darren Kenny,
Bin Meng, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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/
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-30 13:28 ` Peter Maydell
2022-05-30 13:39 ` Philippe Mathieu-Daudé via
@ 2022-05-30 13:41 ` Alexander Bulekov
1 sibling, 0 replies; 12+ messages in thread
From: Alexander Bulekov @ 2022-05-30 13:41 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Li Qiang, Thomas Huth,
Laurent Vivier, Bandan Das, Edgar E . Iglesias, Darren Kenny,
Bin Meng, Paolo Bonzini, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] memory: Track whether a Device is engaged in IO
2022-05-30 13:39 ` Philippe Mathieu-Daudé via
@ 2022-05-30 14:04 ` Alexander Bulekov
0 siblings, 0 replies; 12+ messages in thread
From: Alexander Bulekov @ 2022-05-30 14:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Paolo Bonzini, qemu-devel,
Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Qiuhao Li, Peter Xu, Jason Wang,
David Hildenbrand, Gerd Hoffmann, Li Qiang, Thomas Huth,
Laurent Vivier, Bandan Das, Edgar E . Iglesias, Darren Kenny,
Bin Meng, Stefan Hajnoczi, Daniel P. Berrangé,
Eduardo Habkost
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).
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-30 14:19 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-27 16:19 [PATCH v2 0/3] Fix dma-reentrancy issues Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 1/3] memory: Track whether a Device is engaged in IO Alexander Bulekov
2022-05-30 9:58 ` Darren Kenny
2022-05-30 11:19 ` Peter Maydell
2022-05-30 13:09 ` Alexander Bulekov
2022-05-30 13:28 ` Peter Maydell
2022-05-30 13:39 ` Philippe Mathieu-Daudé via
2022-05-30 14:04 ` Alexander Bulekov
2022-05-30 13:41 ` Alexander Bulekov
2022-05-30 12:13 ` David Hildenbrand
2022-05-27 16:19 ` [PATCH v2 2/3] memory: fix PIO/MMIO-initiated dma-reentracy issues Alexander Bulekov
2022-05-27 16:19 ` [PATCH v2 3/3] memory: fix bh-initiated " Alexander Bulekov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.