[RFC 0/3] try to solve the DMA to MMIO issue

[RFC 0/3] try to solve the DMA to MMIO issue

[Li Qiang]

The qemu device fuzzer has found several DMA to MMIO issue.
These issues is caused by the guest driver programs the DMA
address, then in the device MMIO handler it trigger the DMA
and as the DMA address is MMIO it will trigger another dispatch
and reenter the MMIO handler again. However most of the device
is not reentrant.

DMA to MMIO will cause issues depend by the device emulator,
mostly it will crash the qemu. Following is three classic 
DMA to MMIO issue.

e1000e: https://bugs.launchpad.net/qemu/+bug/1886362
xhci: https://bugs.launchpad.net/qemu/+bug/1891354
virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606

The DMA to MMIO issue I think can be classified as following:
1. DMA to the device itself
2. device A DMA to device B and to device C
3. device A DMA to device B and to device A

The first case of course should not be allowed.
The second case I think it ok as the device IO handler has no
assumption about the IO data came from no matter it come from
device or other device. This is for P2P DMA.
The third case I think it also should not be allowed.

So our issue has been reduced by one case: not allowed the
device's IO handler reenter.

Paolo suggested that we can refactor the device emulation with
BH. However it is a lot of work.
I have thought several propose to address this, also discuss
this with Jason Wang in private email.

I have can solve this issue in core framework or in specific device.
After try several methods I choose address it in per-device for
following reason:
1. If we address it in core framwork we have to recored and check the 
device or MR info in MR dispatch write function. Unfortunally we have
no these info in core framework.
2. The performance will also be decrease largely
3. Only the device itself know its IO

The (most of the) device emulation is protected by BQL one time only
a device emulation code can be run. We can add a flag to indicate the
IO is running. The first two patches does this. For simplicity at the
RFC stage I just set it while enter the IO callback and clear it exit
the IO callback. It should be check/set/clean according the per-device's
IO emulation.
The second issue which itself suffers a race condition so I uses a
atomic.




Li Qiang (3):
  e1000e: make the IO handler reentrant
  xhci: make the IO handler reentrant
  virtio-gpu: make the IO handler reentrant

 hw/display/virtio-gpu.c        | 10 ++++++
 hw/net/e1000e.c                | 35 +++++++++++++++++++-
 hw/usb/hcd-xhci.c              | 60 ++++++++++++++++++++++++++++++++++
 hw/usb/hcd-xhci.h              |  1 +
 include/hw/virtio/virtio-gpu.h |  1 +
 5 files changed, 106 insertions(+), 1 deletion(-)

-- 
2.17.1

[RFC 1/3] e1000e: make the IO handler reentrant

[Li Qiang]

The guest can program the e1000e DMA address to its MMIO.
This will cause an UAF issue.

Following is the reproducer:

cat << EOF | ./i386-softmmu/qemu-system-i386 -M q35,accel=qtest \
-qtest stdio -nographic -monitor none -serial none
outl 0xcf8 0x80001010
outl 0xcfc 0xe1020000
outl 0xcf8 0x80001014
outl 0xcf8 0x80001004
outw 0xcfc 0x7
outl 0xcf8 0x800010a2
write 0xe102003b 0x1 0xff
write 0xe1020103 0x1e 0xffffff055c5e5c30be4511d084ffffffffffffffffffffffffffffffffff
write 0xe1020420 0x4 0xffffffff
write 0xe1020424 0x4 0xffffffff
write 0xe102042b 0x1 0xff
write 0xe1020430 0x4 0x055c5e5c
write 0x5c041 0x1 0x04
write 0x5c042 0x1 0x02
write 0x5c043 0x1 0xe1
write 0x5c048 0x1 0x8a
write 0x5c04a 0x1 0x31
write 0x5c04b 0x1 0xff
write 0xe1020403 0x1 0xff
EOF

This patch avoid this by adding a 'in_io' in E1000EState to indicate it is in IO
processing.

Buglink: https://bugs.launchpad.net/qemu/+bug/1886362

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/net/e1000e.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index fda34518c9..eb6b34b7f3 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -77,6 +77,8 @@ typedef struct E1000EState {
 
     bool disable_vnet;
 
+    bool in_io;
+
     E1000ECore core;
 
 } E1000EState;
@@ -98,7 +100,15 @@ static uint64_t
 e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size)
 {
     E1000EState *s = opaque;
-    return e1000e_core_read(&s->core, addr, size);
+    uint64_t ret;
+
+    if (s->in_io) {
+        return 0;
+    }
+    s->in_io = true;
+    ret = e1000e_core_read(&s->core, addr, size);
+    s->in_io = false;
+    return ret;
 }
 
 static void
@@ -106,7 +116,13 @@ e1000e_mmio_write(void *opaque, hwaddr addr,
                    uint64_t val, unsigned size)
 {
     E1000EState *s = opaque;
+
+    if (s->in_io) {
+        return;
+    }
+    s->in_io = true;
     e1000e_core_write(&s->core, addr, val, size);
+    s->in_io = false;
 }
 
 static bool
@@ -138,19 +154,28 @@ e1000e_io_read(void *opaque, hwaddr addr, unsigned size)
     uint32_t idx = 0;
     uint64_t val;
 
+    if (s->in_io) {
+            return 0;
+    }
+    s->in_io = true;
+
     switch (addr) {
     case E1000_IOADDR:
         trace_e1000e_io_read_addr(s->ioaddr);
+        s->in_io = false;
         return s->ioaddr;
     case E1000_IODATA:
         if (e1000e_io_get_reg_index(s, &idx)) {
             val = e1000e_core_read(&s->core, idx, sizeof(val));
             trace_e1000e_io_read_data(idx, val);
+            s->in_io = false;
             return val;
         }
+        s->in_io = false;
         return 0;
     default:
         trace_e1000e_wrn_io_read_unknown(addr);
+        s->in_io = false;
         return 0;
     }
 }
@@ -162,19 +187,27 @@ e1000e_io_write(void *opaque, hwaddr addr,
     E1000EState *s = opaque;
     uint32_t idx = 0;
 
+    if (s->in_io) {
+        return;
+    }
+    s->in_io = true;
+
     switch (addr) {
     case E1000_IOADDR:
         trace_e1000e_io_write_addr(val);
         s->ioaddr = (uint32_t) val;
+        s->in_io = false;
         return;
     case E1000_IODATA:
         if (e1000e_io_get_reg_index(s, &idx)) {
             trace_e1000e_io_write_data(idx, val);
             e1000e_core_write(&s->core, idx, val, sizeof(val));
         }
+        s->in_io = false;
         return;
     default:
         trace_e1000e_wrn_io_write_unknown(addr);
+        s->in_io = false;
         return;
     }
 }
-- 
2.17.1

[RFC 2/3] xhci: make the IO handler reentrant

[Li Qiang]

The guest can program the xhci DMA address to its MMIO.
This will cause an UAF issue.

Following is the reproducer:

cat << EOF | ./i386-softmmu/qemu-system-i386 -device nec-usb-xhci \
-trace usb\* -device usb-audio -device usb-storage,drive=mydrive \
-drive id=mydrive,file=null-co://,size=2M,format=raw,if=none \
-nodefaults -nographic -qtest stdio
outl 0xcf8 0x80001010
outl 0xcfc 0xc0202
outl 0xcf8 0x80001004
outl 0xcfc 0x1c77695e
writel 0xc0040 0xffffd855
writeq 0xc2000 0xff05140100000000
write 0x1d 0x1 0x27
write 0x2d 0x1 0x2e
write 0x17232 0x1 0x03
write 0x17254 0x1 0x05
write 0x17276 0x1 0x72
write 0x17278 0x1 0x02
write 0x3d 0x1 0x27
write 0x40 0x1 0x2e
write 0x41 0x1 0x72
write 0x42 0x1 0x01
write 0x4d 0x1 0x2e
write 0x4f 0x1 0x01
write 0x2007c 0x1 0xc7
writeq 0xc2000 0x5c05140100000000
write 0x20070 0x1 0x80
write 0x20078 0x1 0x08
write 0x2007c 0x1 0xfe
write 0x2007d 0x1 0x08
write 0x20081 0x1 0xff
write 0x20082 0x1 0x0b
write 0x20089 0x1 0x8c
write 0x2008d 0x1 0x04
write 0x2009d 0x1 0x10
writeq 0xc2000 0x2505ef019e092f00
EOF

This patch avoid this by adding a 'in_io' in XHCIState to indicate it is in IO
processing.

Buglink: https://bugs.launchpad.net/qemu/+bug/1891354

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/usb/hcd-xhci.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/usb/hcd-xhci.h |  1 +
 2 files changed, 61 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 46a2186d91..06cd235123 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2738,6 +2738,11 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size)
     XHCIState *xhci = ptr;
     uint32_t ret;
 
+    if (xhci->in_io) {
+        return 0;
+    }
+    xhci->in_io = true;
+
     switch (reg) {
     case 0x00: /* HCIVERSION, CAPLENGTH */
         ret = 0x01000000 | LEN_CAP;
@@ -2805,6 +2810,9 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr reg, unsigned size)
     }
 
     trace_usb_xhci_cap_read(reg, ret);
+
+    xhci->in_io = false;
+
     return ret;
 }
 
@@ -2813,6 +2821,11 @@ static uint64_t xhci_port_read(void *ptr, hwaddr reg, unsigned size)
     XHCIPort *port = ptr;
     uint32_t ret;
 
+    if (port->xhci->in_io) {
+        return 0;
+    }
+    port->xhci->in_io = true;
+
     switch (reg) {
     case 0x00: /* PORTSC */
         ret = port->portsc;
@@ -2828,6 +2841,9 @@ static uint64_t xhci_port_read(void *ptr, hwaddr reg, unsigned size)
     }
 
     trace_usb_xhci_port_read(port->portnr, reg, ret);
+
+    port->xhci->in_io = false;
+
     return ret;
 }
 
@@ -2837,6 +2853,11 @@ static void xhci_port_write(void *ptr, hwaddr reg,
     XHCIPort *port = ptr;
     uint32_t portsc, notify;
 
+    if (port->xhci->in_io) {
+        return;
+    }
+    port->xhci->in_io = true;
+
     trace_usb_xhci_port_write(port->portnr, reg, val);
 
     switch (reg) {
@@ -2896,6 +2917,7 @@ static void xhci_port_write(void *ptr, hwaddr reg,
     default:
         trace_usb_xhci_unimplemented("port write", reg);
     }
+    port->xhci->in_io = false;
 }
 
 static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size)
@@ -2903,6 +2925,11 @@ static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size)
     XHCIState *xhci = ptr;
     uint32_t ret;
 
+    if (xhci->in_io) {
+        return 0;
+    }
+    xhci->in_io = true;
+
     switch (reg) {
     case 0x00: /* USBCMD */
         ret = xhci->usbcmd;
@@ -2937,6 +2964,9 @@ static uint64_t xhci_oper_read(void *ptr, hwaddr reg, unsigned size)
     }
 
     trace_usb_xhci_oper_read(reg, ret);
+
+    xhci->in_io = false;
+
     return ret;
 }
 
@@ -2946,6 +2976,11 @@ static void xhci_oper_write(void *ptr, hwaddr reg,
     XHCIState *xhci = ptr;
     DeviceState *d = DEVICE(ptr);
 
+    if (xhci->in_io) {
+        return;
+    }
+    xhci->in_io = true;
+
     trace_usb_xhci_oper_write(reg, val);
 
     switch (reg) {
@@ -3008,6 +3043,7 @@ static void xhci_oper_write(void *ptr, hwaddr reg,
     default:
         trace_usb_xhci_unimplemented("oper write", reg);
     }
+    xhci->in_io = false;
 }
 
 static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
@@ -3016,6 +3052,11 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
     XHCIState *xhci = ptr;
     uint32_t ret = 0;
 
+    if (xhci->in_io) {
+        return 0;
+    }
+    xhci->in_io = true;
+
     if (reg < 0x20) {
         switch (reg) {
         case 0x00: /* MFINDEX */
@@ -3054,6 +3095,9 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
     }
 
     trace_usb_xhci_runtime_read(reg, ret);
+
+    xhci->in_io = false;
+
     return ret;
 }
 
@@ -3063,10 +3107,17 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
     XHCIState *xhci = ptr;
     int v = (reg - 0x20) / 0x20;
     XHCIInterrupter *intr = &xhci->intr[v];
+
+    if (xhci->in_io) {
+        return;
+    }
+    xhci->in_io = true;
+
     trace_usb_xhci_runtime_write(reg, val);
 
     if (reg < 0x20) {
         trace_usb_xhci_unimplemented("runtime write", reg);
+        xhci->in_io = false;
         return;
     }
 
@@ -3121,6 +3172,7 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
     default:
         trace_usb_xhci_unimplemented("oper write", reg);
     }
+    xhci->in_io = false;
 }
 
 static uint64_t xhci_doorbell_read(void *ptr, hwaddr reg,
@@ -3137,10 +3189,17 @@ static void xhci_doorbell_write(void *ptr, hwaddr reg,
     XHCIState *xhci = ptr;
     unsigned int epid, streamid;
 
+    if (xhci->in_io) {
+        return;
+    }
+
+    xhci->in_io = true;
+
     trace_usb_xhci_doorbell_write(reg, val);
 
     if (!xhci_running(xhci)) {
         DPRINTF("xhci: wrote doorbell while xHC stopped or paused\n");
+        xhci->in_io = false;
         return;
     }
 
@@ -3165,6 +3224,7 @@ static void xhci_doorbell_write(void *ptr, hwaddr reg,
             xhci_kick_ep(xhci, reg, epid, streamid);
         }
     }
+    xhci->in_io = false;
 }
 
 static void xhci_cap_write(void *opaque, hwaddr addr, uint64_t val,
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index 946af51fc2..ed16232c96 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -227,6 +227,7 @@ struct XHCIState {
     XHCIRing cmd_ring;
 
     bool nec_quirks;
+    bool in_io;
 };
 
 #endif
-- 
2.17.1

[RFC 3/3] virtio-gpu: make the IO handler reentrant

[Li Qiang]

The guest can program the virtio desc table. When the used ring
is programmed by virtio-gpu's MMIO it may cause an reentrant issue.

Following is the reproducer:

cat << EOF | ./i386-softmmu/qemu-system-i386 -nographic -M pc -nodefaults -m 512M -device virtio-vga -qtest stdio
outl 0xcf8 0x80001018
outl 0xcfc 0xe0800000
outl 0xcf8 0x80001020
outl 0xcf8 0x80001004
outw 0xcfc 0x7
writeq 0xe0801024 0x10646c00776c6cff
writeq 0xe080102d 0xe0801000320000
writeq 0xe0801015 0x12b2901ba000000
write 0x10646c02 0x1 0x2c
write 0x999 0x1 0x25
write 0x8 0x1 0x78
write 0x2c7 0x1 0x32
write 0x2cb 0x1 0xff
write 0x2cc 0x1 0x7e
writeq 0xe0803000 0xf2b8f0540ff83
EOF

This patch avoid this by adding a 'in_io' in VirtIOGPU to indicate it is in IO processing.
Notice this also address the race condition between 'virtio_gpu_process_cmdq' and
'virtio_gpu_reset' as the 'virtio_gpu_process_cmdq' is run in a BH which in the main thread
and 'virtio_gpu_reset' is run in the vcpu thread and both of them access the 'g->cmdq'.

Buglink: https://bugs.launchpad.net/qemu/+bug/1888606

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/display/virtio-gpu.c        | 10 ++++++++++
 include/hw/virtio/virtio-gpu.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5f0dd7c150..404b7dc174 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -809,6 +809,10 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
 {
     struct virtio_gpu_ctrl_command *cmd;
 
+    if (atomic_read(&g->in_io)) {
+        return;
+    }
+    atomic_set(&g->in_io, 1);
     while (!QTAILQ_EMPTY(&g->cmdq)) {
         cmd = QTAILQ_FIRST(&g->cmdq);
 
@@ -838,6 +842,7 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
             g_free(cmd);
         }
     }
+    atomic_set(&g->in_io, 0);
 }
 
 static void virtio_gpu_gl_unblock(VirtIOGPUBase *b)
@@ -1144,6 +1149,10 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
     struct virtio_gpu_simple_resource *res, *tmp;
     struct virtio_gpu_ctrl_command *cmd;
 
+    if (atomic_read(&g->in_io)) {
+        return;
+    }
+    atomic_set(&g->in_io, 1);
 #ifdef CONFIG_VIRGL
     if (g->parent_obj.use_virgl_renderer) {
         virtio_gpu_virgl_reset(g);
@@ -1179,6 +1188,7 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
 #endif
 
     virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
+    atomic_set(&g->in_io, 0);
 }
 
 static void
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 7517438e10..aadcf0e332 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -150,6 +150,7 @@ typedef struct VirtIOGPU {
 
     bool renderer_inited;
     bool renderer_reset;
+    bool in_io;
     QEMUTimer *fence_poll;
     QEMUTimer *print_stats;
 
-- 
2.17.1

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Jason Wang]

On 2020/9/3 上午12:22, Li Qiang wrote:
> The qemu device fuzzer has found several DMA to MMIO issue.
> These issues is caused by the guest driver programs the DMA
> address, then in the device MMIO handler it trigger the DMA
> and as the DMA address is MMIO it will trigger another dispatch
> and reenter the MMIO handler again. However most of the device
> is not reentrant.
>
> DMA to MMIO will cause issues depend by the device emulator,
> mostly it will crash the qemu. Following is three classic
> DMA to MMIO issue.
>
> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362
> xhci: https://bugs.launchpad.net/qemu/+bug/1891354
> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606
>
> The DMA to MMIO issue I think can be classified as following:
> 1. DMA to the device itself
> 2. device A DMA to device B and to device C
> 3. device A DMA to device B and to device A
>
> The first case of course should not be allowed.
> The second case I think it ok as the device IO handler has no
> assumption about the IO data came from no matter it come from
> device or other device. This is for P2P DMA.
> The third case I think it also should not be allowed.
>
> So our issue has been reduced by one case: not allowed the
> device's IO handler reenter.
>
> Paolo suggested that we can refactor the device emulation with
> BH. However it is a lot of work.
> I have thought several propose to address this, also discuss
> this with Jason Wang in private email.
>
> I have can solve this issue in core framework or in specific device.
> After try several methods I choose address it in per-device for
> following reason:
> 1. If we address it in core framwork we have to recored and check the
> device or MR info in MR dispatch write function. Unfortunally we have
> no these info in core framework.
> 2. The performance will also be decrease largely
> 3. Only the device itself know its IO


I think we still need to seek a way to address this issue completely.

How about adding a flag in MemoryRegionOps and detect the reentrancy 
through that flag?

Thanks


>
> The (most of the) device emulation is protected by BQL one time only
> a device emulation code can be run. We can add a flag to indicate the
> IO is running. The first two patches does this. For simplicity at the
> RFC stage I just set it while enter the IO callback and clear it exit
> the IO callback. It should be check/set/clean according the per-device's
> IO emulation.
> The second issue which itself suffers a race condition so I uses a
> atomic.
>
>
>
>
> Li Qiang (3):
>    e1000e: make the IO handler reentrant
>    xhci: make the IO handler reentrant
>    virtio-gpu: make the IO handler reentrant
>
>   hw/display/virtio-gpu.c        | 10 ++++++
>   hw/net/e1000e.c                | 35 +++++++++++++++++++-
>   hw/usb/hcd-xhci.c              | 60 ++++++++++++++++++++++++++++++++++
>   hw/usb/hcd-xhci.h              |  1 +
>   include/hw/virtio/virtio-gpu.h |  1 +
>   5 files changed, 106 insertions(+), 1 deletion(-)
>

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Alexander Bulekov]

On 200903 1154, Jason Wang wrote:
> 
> On 2020/9/3 上午12:22, Li Qiang wrote:
> > The qemu device fuzzer has found several DMA to MMIO issue.
> > These issues is caused by the guest driver programs the DMA
> > address, then in the device MMIO handler it trigger the DMA
> > and as the DMA address is MMIO it will trigger another dispatch
> > and reenter the MMIO handler again. However most of the device
> > is not reentrant.
> > 
> > DMA to MMIO will cause issues depend by the device emulator,
> > mostly it will crash the qemu. Following is three classic
> > DMA to MMIO issue.
> > 
> > e1000e: https://bugs.launchpad.net/qemu/+bug/1886362
> > xhci: https://bugs.launchpad.net/qemu/+bug/1891354
> > virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606
> > 
> > The DMA to MMIO issue I think can be classified as following:
> > 1. DMA to the device itself
> > 2. device A DMA to device B and to device C
> > 3. device A DMA to device B and to device A
> > 
> > The first case of course should not be allowed.
> > The second case I think it ok as the device IO handler has no
> > assumption about the IO data came from no matter it come from
> > device or other device. This is for P2P DMA.
> > The third case I think it also should not be allowed.
> > 
> > So our issue has been reduced by one case: not allowed the
> > device's IO handler reenter.
> > 
> > Paolo suggested that we can refactor the device emulation with
> > BH. However it is a lot of work.
> > I have thought several propose to address this, also discuss
> > this with Jason Wang in private email.
> > 
> > I have can solve this issue in core framework or in specific device.
> > After try several methods I choose address it in per-device for
> > following reason:
> > 1. If we address it in core framwork we have to recored and check the
> > device or MR info in MR dispatch write function. Unfortunally we have
> > no these info in core framework.
> > 2. The performance will also be decrease largely
> > 3. Only the device itself know its IO
> 
> 
> I think we still need to seek a way to address this issue completely.
> 
> How about adding a flag in MemoryRegionOps and detect the reentrancy through
> that flag?

What happens for devices with multiple MemoryRegions? Make all the
MemoryRegionOps share the same flag?

What about the virtio-gpu bug, where the problem happens in a bh->mmio
access rather than an mmio->mmio access?

-Alex

> Thanks
> 
> 
> > 
> > The (most of the) device emulation is protected by BQL one time only
> > a device emulation code can be run. We can add a flag to indicate the
> > IO is running. The first two patches does this. For simplicity at the
> > RFC stage I just set it while enter the IO callback and clear it exit
> > the IO callback. It should be check/set/clean according the per-device's
> > IO emulation.
> > The second issue which itself suffers a race condition so I uses a
> > atomic.
> > 
> > 
> > 
> > 
> > Li Qiang (3):
> >    e1000e: make the IO handler reentrant
> >    xhci: make the IO handler reentrant
> >    virtio-gpu: make the IO handler reentrant
> > 
> >   hw/display/virtio-gpu.c        | 10 ++++++
> >   hw/net/e1000e.c                | 35 +++++++++++++++++++-
> >   hw/usb/hcd-xhci.c              | 60 ++++++++++++++++++++++++++++++++++
> >   hw/usb/hcd-xhci.h              |  1 +
> >   include/hw/virtio/virtio-gpu.h |  1 +
> >   5 files changed, 106 insertions(+), 1 deletion(-)
> > 
> 

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Jason Wang]

On 2020/9/3 下午12:06, Alexander Bulekov wrote:
> On 200903 1154, Jason Wang wrote:
>> On 2020/9/3 上午12:22, Li Qiang wrote:
>>> The qemu device fuzzer has found several DMA to MMIO issue.
>>> These issues is caused by the guest driver programs the DMA
>>> address, then in the device MMIO handler it trigger the DMA
>>> and as the DMA address is MMIO it will trigger another dispatch
>>> and reenter the MMIO handler again. However most of the device
>>> is not reentrant.
>>>
>>> DMA to MMIO will cause issues depend by the device emulator,
>>> mostly it will crash the qemu. Following is three classic
>>> DMA to MMIO issue.
>>>
>>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362
>>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354
>>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606
>>>
>>> The DMA to MMIO issue I think can be classified as following:
>>> 1. DMA to the device itself
>>> 2. device A DMA to device B and to device C
>>> 3. device A DMA to device B and to device A
>>>
>>> The first case of course should not be allowed.
>>> The second case I think it ok as the device IO handler has no
>>> assumption about the IO data came from no matter it come from
>>> device or other device. This is for P2P DMA.
>>> The third case I think it also should not be allowed.
>>>
>>> So our issue has been reduced by one case: not allowed the
>>> device's IO handler reenter.
>>>
>>> Paolo suggested that we can refactor the device emulation with
>>> BH. However it is a lot of work.
>>> I have thought several propose to address this, also discuss
>>> this with Jason Wang in private email.
>>>
>>> I have can solve this issue in core framework or in specific device.
>>> After try several methods I choose address it in per-device for
>>> following reason:
>>> 1. If we address it in core framwork we have to recored and check the
>>> device or MR info in MR dispatch write function. Unfortunally we have
>>> no these info in core framework.
>>> 2. The performance will also be decrease largely
>>> 3. Only the device itself know its IO
>>
>> I think we still need to seek a way to address this issue completely.
>>
>> How about adding a flag in MemoryRegionOps and detect the reentrancy through
>> that flag?
> What happens for devices with multiple MemoryRegions? Make all the
> MemoryRegionOps share the same flag?


I think there could be two approaches:

1) record the device in MR as Qiang mentioned
2) Only forbid the reentrancy in MMIO handler and depends on the device 
to solve the multiple Memory Region issue, if the regions want to access 
the same data, it needs to be synchronized internally

But the point is still to try to solve it in the layer of memory 
regions. Otherwise we may still hit similar issues.


>
> What about the virtio-gpu bug, where the problem happens in a bh->mmio
> access rather than an mmio->mmio access?


Yes, it needs more thought, but as a first step, we can try to fix the 
MMIO handler issue and do bh fix on top.

Thanks


>
> -Alex
>
>> Thanks
>>
>>
>>> The (most of the) device emulation is protected by BQL one time only
>>> a device emulation code can be run. We can add a flag to indicate the
>>> IO is running. The first two patches does this. For simplicity at the
>>> RFC stage I just set it while enter the IO callback and clear it exit
>>> the IO callback. It should be check/set/clean according the per-device's
>>> IO emulation.
>>> The second issue which itself suffers a race condition so I uses a
>>> atomic.
>>>
>>>
>>>
>>>
>>> Li Qiang (3):
>>>     e1000e: make the IO handler reentrant
>>>     xhci: make the IO handler reentrant
>>>     virtio-gpu: make the IO handler reentrant
>>>
>>>    hw/display/virtio-gpu.c        | 10 ++++++
>>>    hw/net/e1000e.c                | 35 +++++++++++++++++++-
>>>    hw/usb/hcd-xhci.c              | 60 ++++++++++++++++++++++++++++++++++
>>>    hw/usb/hcd-xhci.h              |  1 +
>>>    include/hw/virtio/virtio-gpu.h |  1 +
>>>    5 files changed, 106 insertions(+), 1 deletion(-)
>>>

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Li Qiang]

Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午12:24写道：
>
>
> On 2020/9/3 下午12:06, Alexander Bulekov wrote:
> > On 200903 1154, Jason Wang wrote:
> >> On 2020/9/3 上午12:22, Li Qiang wrote:
> >>> The qemu device fuzzer has found several DMA to MMIO issue.
> >>> These issues is caused by the guest driver programs the DMA
> >>> address, then in the device MMIO handler it trigger the DMA
> >>> and as the DMA address is MMIO it will trigger another dispatch
> >>> and reenter the MMIO handler again. However most of the device
> >>> is not reentrant.
> >>>
> >>> DMA to MMIO will cause issues depend by the device emulator,
> >>> mostly it will crash the qemu. Following is three classic
> >>> DMA to MMIO issue.
> >>>
> >>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362
> >>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354
> >>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606
> >>>
> >>> The DMA to MMIO issue I think can be classified as following:
> >>> 1. DMA to the device itself
> >>> 2. device A DMA to device B and to device C
> >>> 3. device A DMA to device B and to device A
> >>>
> >>> The first case of course should not be allowed.
> >>> The second case I think it ok as the device IO handler has no
> >>> assumption about the IO data came from no matter it come from
> >>> device or other device. This is for P2P DMA.
> >>> The third case I think it also should not be allowed.
> >>>
> >>> So our issue has been reduced by one case: not allowed the
> >>> device's IO handler reenter.
> >>>
> >>> Paolo suggested that we can refactor the device emulation with
> >>> BH. However it is a lot of work.
> >>> I have thought several propose to address this, also discuss
> >>> this with Jason Wang in private email.
> >>>
> >>> I have can solve this issue in core framework or in specific device.
> >>> After try several methods I choose address it in per-device for
> >>> following reason:
> >>> 1. If we address it in core framwork we have to recored and check the
> >>> device or MR info in MR dispatch write function. Unfortunally we have
> >>> no these info in core framework.
> >>> 2. The performance will also be decrease largely
> >>> 3. Only the device itself know its IO
> >>
> >> I think we still need to seek a way to address this issue completely.
> >>
> >> How about adding a flag in MemoryRegionOps and detect the reentrancy through
> >> that flag?
> > What happens for devices with multiple MemoryRegions? Make all the
> > MemoryRegionOps share the same flag?
>
>
> I think there could be two approaches:
>
> 1) record the device in MR as Qiang mentioned

I have tried this as we discussed. But has following consideration:
1. The performance, we need to check/record/clean the MR in an array/hashtable.

2. The multiple MR and alias MR process in the memory layer. It is
complicated and performance effective.
So If we let the MR issue to the device itself, it is just as this
patch does-let the device address the reentrancy issue.f

Another solution. We connects a MR with the corresponding device. Now
the device often tight MR with an 'opaque' field.
Just uses it in the calling of MR callback. Then we add a flag in the
device and needs to modify the MR register interface.

So in the memory layer we can check/record/clean the MR->device->flag.
But this is can't address the DMA (in BH) to MMIO issue as the BH runs
in main thread.

Thanks,
Li Qiang



> 2) Only forbid the reentrancy in MMIO handler and depends on the device
> to solve the multiple Memory Region issue, if the regions want to access
> the same data, it needs to be synchronized internally
>
> But the point is still to try to solve it in the layer of memory
> regions. Otherwise we may still hit similar issues.
>
>
> >
> > What about the virtio-gpu bug, where the problem happens in a bh->mmio
> > access rather than an mmio->mmio access?
>
>
> Yes, it needs more thought, but as a first step, we can try to fix the
> MMIO handler issue and do bh fix on top.



>
> Thanks
>
>
> >
> > -Alex
> >
> >> Thanks
> >>
> >>
> >>> The (most of the) device emulation is protected by BQL one time only
> >>> a device emulation code can be run. We can add a flag to indicate the
> >>> IO is running. The first two patches does this. For simplicity at the
> >>> RFC stage I just set it while enter the IO callback and clear it exit
> >>> the IO callback. It should be check/set/clean according the per-device's
> >>> IO emulation.
> >>> The second issue which itself suffers a race condition so I uses a
> >>> atomic.
> >>>
> >>>
> >>>
> >>>
> >>> Li Qiang (3):
> >>>     e1000e: make the IO handler reentrant
> >>>     xhci: make the IO handler reentrant
> >>>     virtio-gpu: make the IO handler reentrant
> >>>
> >>>    hw/display/virtio-gpu.c        | 10 ++++++
> >>>    hw/net/e1000e.c                | 35 +++++++++++++++++++-
> >>>    hw/usb/hcd-xhci.c              | 60 ++++++++++++++++++++++++++++++++++
> >>>    hw/usb/hcd-xhci.h              |  1 +
> >>>    include/hw/virtio/virtio-gpu.h |  1 +
> >>>    5 files changed, 106 insertions(+), 1 deletion(-)
> >>>
>

Re: [RFC 3/3] virtio-gpu: make the IO handler reentrant

[Michael Tokarev]

02.09.2020 19:22, Li Qiang wrote:
..
> @@ -809,6 +809,10 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
>  {
>      struct virtio_gpu_ctrl_command *cmd;
>  
> +    if (atomic_read(&g->in_io)) {
> +        return;
> +    }
> +    atomic_set(&g->in_io, 1);

Can't we race in these two instructions? Both
threads atomic_reads at the same time, both see zero,
and both are trying to set it to 1, no?

Just asking really, b/c despite of the atomic_ prefix,
to me this look a bit unsafe..

Thanks,

/mjt

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Jason Wang]

On 2020/9/3 下午12:50, Li Qiang wrote:
> Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午12:24写道：
>>
>> On 2020/9/3 下午12:06, Alexander Bulekov wrote:
>>> On 200903 1154, Jason Wang wrote:
>>>> On 2020/9/3 上午12:22, Li Qiang wrote:
>>>>> The qemu device fuzzer has found several DMA to MMIO issue.
>>>>> These issues is caused by the guest driver programs the DMA
>>>>> address, then in the device MMIO handler it trigger the DMA
>>>>> and as the DMA address is MMIO it will trigger another dispatch
>>>>> and reenter the MMIO handler again. However most of the device
>>>>> is not reentrant.
>>>>>
>>>>> DMA to MMIO will cause issues depend by the device emulator,
>>>>> mostly it will crash the qemu. Following is three classic
>>>>> DMA to MMIO issue.
>>>>>
>>>>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362
>>>>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354
>>>>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606
>>>>>
>>>>> The DMA to MMIO issue I think can be classified as following:
>>>>> 1. DMA to the device itself
>>>>> 2. device A DMA to device B and to device C
>>>>> 3. device A DMA to device B and to device A
>>>>>
>>>>> The first case of course should not be allowed.
>>>>> The second case I think it ok as the device IO handler has no
>>>>> assumption about the IO data came from no matter it come from
>>>>> device or other device. This is for P2P DMA.
>>>>> The third case I think it also should not be allowed.
>>>>>
>>>>> So our issue has been reduced by one case: not allowed the
>>>>> device's IO handler reenter.
>>>>>
>>>>> Paolo suggested that we can refactor the device emulation with
>>>>> BH. However it is a lot of work.
>>>>> I have thought several propose to address this, also discuss
>>>>> this with Jason Wang in private email.
>>>>>
>>>>> I have can solve this issue in core framework or in specific device.
>>>>> After try several methods I choose address it in per-device for
>>>>> following reason:
>>>>> 1. If we address it in core framwork we have to recored and check the
>>>>> device or MR info in MR dispatch write function. Unfortunally we have
>>>>> no these info in core framework.
>>>>> 2. The performance will also be decrease largely
>>>>> 3. Only the device itself know its IO
>>>> I think we still need to seek a way to address this issue completely.
>>>>
>>>> How about adding a flag in MemoryRegionOps and detect the reentrancy through
>>>> that flag?
>>> What happens for devices with multiple MemoryRegions? Make all the
>>> MemoryRegionOps share the same flag?
>>
>> I think there could be two approaches:
>>
>> 1) record the device in MR as Qiang mentioned
> I have tried this as we discussed. But has following consideration:
> 1. The performance, we need to check/record/clean the MR in an array/hashtable.
>
> 2. The multiple MR and alias MR process in the memory layer. It is
> complicated and performance effective.
> So If we let the MR issue to the device itself, it is just as this
> patch does-let the device address the reentrancy issue.f
>
> Another solution. We connects a MR with the corresponding device. Now
> the device often tight MR with an 'opaque' field.
> Just uses it in the calling of MR callback. Then we add a flag in the
> device and needs to modify the MR register interface.
>
> So in the memory layer we can check/record/clean the MR->device->flag.
> But this is can't address the DMA (in BH) to MMIO issue as the BH runs
> in main thread.


This is probably good enough to start. To my point of view, there're two 
different issues:

1) re-entrant MMIO handler
2) MMIO hanlder sync with BH

For 1), we'd better solve it at core, For 2) it can only be solved in 
the device.

Thanks


>
> Thanks,
> Li Qiang
>
>
>
>> 2) Only forbid the reentrancy in MMIO handler and depends on the device
>> to solve the multiple Memory Region issue, if the regions want to access
>> the same data, it needs to be synchronized internally
>>
>> But the point is still to try to solve it in the layer of memory
>> regions. Otherwise we may still hit similar issues.
>>
>>
>>> What about the virtio-gpu bug, where the problem happens in a bh->mmio
>>> access rather than an mmio->mmio access?
>>
>> Yes, it needs more thought, but as a first step, we can try to fix the
>> MMIO handler issue and do bh fix on top.
>
>
>> Thanks
>>
>>
>>> -Alex
>>>
>>>> Thanks
>>>>
>>>>
>>>>> The (most of the) device emulation is protected by BQL one time only
>>>>> a device emulation code can be run. We can add a flag to indicate the
>>>>> IO is running. The first two patches does this. For simplicity at the
>>>>> RFC stage I just set it while enter the IO callback and clear it exit
>>>>> the IO callback. It should be check/set/clean according the per-device's
>>>>> IO emulation.
>>>>> The second issue which itself suffers a race condition so I uses a
>>>>> atomic.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Li Qiang (3):
>>>>>      e1000e: make the IO handler reentrant
>>>>>      xhci: make the IO handler reentrant
>>>>>      virtio-gpu: make the IO handler reentrant
>>>>>
>>>>>     hw/display/virtio-gpu.c        | 10 ++++++
>>>>>     hw/net/e1000e.c                | 35 +++++++++++++++++++-
>>>>>     hw/usb/hcd-xhci.c              | 60 ++++++++++++++++++++++++++++++++++
>>>>>     hw/usb/hcd-xhci.h              |  1 +
>>>>>     include/hw/virtio/virtio-gpu.h |  1 +
>>>>>     5 files changed, 106 insertions(+), 1 deletion(-)
>>>>>

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Li Qiang]

Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午2:16写道：
>
>
> On 2020/9/3 下午12:50, Li Qiang wrote:
> > Jason Wang <jasowang@redhat.com> 于2020年9月3日周四 下午12:24写道：
> >>
> >> On 2020/9/3 下午12:06, Alexander Bulekov wrote:
> >>> On 200903 1154, Jason Wang wrote:
> >>>> On 2020/9/3 上午12:22, Li Qiang wrote:
> >>>>> The qemu device fuzzer has found several DMA to MMIO issue.
> >>>>> These issues is caused by the guest driver programs the DMA
> >>>>> address, then in the device MMIO handler it trigger the DMA
> >>>>> and as the DMA address is MMIO it will trigger another dispatch
> >>>>> and reenter the MMIO handler again. However most of the device
> >>>>> is not reentrant.
> >>>>>
> >>>>> DMA to MMIO will cause issues depend by the device emulator,
> >>>>> mostly it will crash the qemu. Following is three classic
> >>>>> DMA to MMIO issue.
> >>>>>
> >>>>> e1000e: https://bugs.launchpad.net/qemu/+bug/1886362
> >>>>> xhci: https://bugs.launchpad.net/qemu/+bug/1891354
> >>>>> virtio-gpu: https://bugs.launchpad.net/qemu/+bug/1888606
> >>>>>
> >>>>> The DMA to MMIO issue I think can be classified as following:
> >>>>> 1. DMA to the device itself
> >>>>> 2. device A DMA to device B and to device C
> >>>>> 3. device A DMA to device B and to device A
> >>>>>
> >>>>> The first case of course should not be allowed.
> >>>>> The second case I think it ok as the device IO handler has no
> >>>>> assumption about the IO data came from no matter it come from
> >>>>> device or other device. This is for P2P DMA.
> >>>>> The third case I think it also should not be allowed.
> >>>>>
> >>>>> So our issue has been reduced by one case: not allowed the
> >>>>> device's IO handler reenter.
> >>>>>
> >>>>> Paolo suggested that we can refactor the device emulation with
> >>>>> BH. However it is a lot of work.
> >>>>> I have thought several propose to address this, also discuss
> >>>>> this with Jason Wang in private email.
> >>>>>
> >>>>> I have can solve this issue in core framework or in specific device.
> >>>>> After try several methods I choose address it in per-device for
> >>>>> following reason:
> >>>>> 1. If we address it in core framwork we have to recored and check the
> >>>>> device or MR info in MR dispatch write function. Unfortunally we have
> >>>>> no these info in core framework.
> >>>>> 2. The performance will also be decrease largely
> >>>>> 3. Only the device itself know its IO
> >>>> I think we still need to seek a way to address this issue completely.
> >>>>
> >>>> How about adding a flag in MemoryRegionOps and detect the reentrancy through
> >>>> that flag?
> >>> What happens for devices with multiple MemoryRegions? Make all the
> >>> MemoryRegionOps share the same flag?
> >>
> >> I think there could be two approaches:
> >>
> >> 1) record the device in MR as Qiang mentioned
> > I have tried this as we discussed. But has following consideration:
> > 1. The performance, we need to check/record/clean the MR in an array/hashtable.
> >
> > 2. The multiple MR and alias MR process in the memory layer. It is
> > complicated and performance effective.
> > So If we let the MR issue to the device itself, it is just as this
> > patch does-let the device address the reentrancy issue.f
> >
> > Another solution. We connects a MR with the corresponding device. Now
> > the device often tight MR with an 'opaque' field.
> > Just uses it in the calling of MR callback. Then we add a flag in the
> > device and needs to modify the MR register interface.
> >
> > So in the memory layer we can check/record/clean the MR->device->flag.
> > But this is can't address the DMA (in BH) to MMIO issue as the BH runs
> > in main thread.
>
>
> This is probably good enough to start. To my point of view, there're two
> different issues:
>
> 1) re-entrant MMIO handler
> 2) MMIO hanlder sync with BH
>

Agree, here I want to address these two kind of issue in a manner so
it just be left to the device itself.
I  will try to add a new memory register function
memory_region_init_io_with_device
to connect the MR and device. And solve it in the memory layer.


Thanks,
Li Qiang

> For 1), we'd better solve it at core, For 2) it can only be solved in
> the device.
>
> Thanks
>
>
> >
> > Thanks,
> > Li Qiang
> >
> >
> >
> >> 2) Only forbid the reentrancy in MMIO handler and depends on the device
> >> to solve the multiple Memory Region issue, if the regions want to access
> >> the same data, it needs to be synchronized internally
> >>
> >> But the point is still to try to solve it in the layer of memory
> >> regions. Otherwise we may still hit similar issues.
> >>
> >>
> >>> What about the virtio-gpu bug, where the problem happens in a bh->mmio
> >>> access rather than an mmio->mmio access?
> >>
> >> Yes, it needs more thought, but as a first step, we can try to fix the
> >> MMIO handler issue and do bh fix on top.
> >
> >
> >> Thanks
> >>
> >>
> >>> -Alex
> >>>
> >>>> Thanks
> >>>>
> >>>>
> >>>>> The (most of the) device emulation is protected by BQL one time only
> >>>>> a device emulation code can be run. We can add a flag to indicate the
> >>>>> IO is running. The first two patches does this. For simplicity at the
> >>>>> RFC stage I just set it while enter the IO callback and clear it exit
> >>>>> the IO callback. It should be check/set/clean according the per-device's
> >>>>> IO emulation.
> >>>>> The second issue which itself suffers a race condition so I uses a
> >>>>> atomic.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> Li Qiang (3):
> >>>>>      e1000e: make the IO handler reentrant
> >>>>>      xhci: make the IO handler reentrant
> >>>>>      virtio-gpu: make the IO handler reentrant
> >>>>>
> >>>>>     hw/display/virtio-gpu.c        | 10 ++++++
> >>>>>     hw/net/e1000e.c                | 35 +++++++++++++++++++-
> >>>>>     hw/usb/hcd-xhci.c              | 60 ++++++++++++++++++++++++++++++++++
> >>>>>     hw/usb/hcd-xhci.h              |  1 +
> >>>>>     include/hw/virtio/virtio-gpu.h |  1 +
> >>>>>     5 files changed, 106 insertions(+), 1 deletion(-)
> >>>>>
>

Re: [RFC 3/3] virtio-gpu: make the IO handler reentrant

[Li Qiang]

Michael Tokarev <mjt@tls.msk.ru> 于2020年9月3日周四 下午1:12写道：
>
> 02.09.2020 19:22, Li Qiang wrote:
> ..
> > @@ -809,6 +809,10 @@ void virtio_gpu_process_cmdq(VirtIOGPU *g)
> >  {
> >      struct virtio_gpu_ctrl_command *cmd;
> >
> > +    if (atomic_read(&g->in_io)) {
> > +        return;
> > +    }
> > +    atomic_set(&g->in_io, 1);
>
> Can't we race in these two instructions? Both
> threads atomic_reads at the same time, both see zero,
> and both are trying to set it to 1, no?
>
> Just asking really, b/c despite of the atomic_ prefix,
> to me this look a bit unsafe..

Yes you're right. My patch is wrong. Here I try to address race
condition and DMA to MMIO issue at the same time.

I will first focus the DMA to MMIO issue in the revision patch.

Thanks,
Li Qiang

>
> Thanks,
>
> /mjt

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Peter Maydell]

On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote:
> I think we still need to seek a way to address this issue completely.
>
> How about adding a flag in MemoryRegionOps and detect the reentrancy
> through that flag?

This won't catch everything. Consider this situation:
  Device A makes DMA access to device B
  Device B's write-handling causes it to raise an
   outbound qemu_irq signal
  The qemu_irq signal is connected to device A
  Now we have reentered into device A's code

That is to say, the problem is general to "device A does
something that affects device B" links of all kinds, which
can form loops. Self-DMA is just an easy way to find one
category of these with the fuzzer.

thanks
-- PMM

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Li Qiang]

Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道：
>
> On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote:
> > I think we still need to seek a way to address this issue completely.
> >
> > How about adding a flag in MemoryRegionOps and detect the reentrancy
> > through that flag?
>
> This won't catch everything. Consider this situation:
>   Device A makes DMA access to device B
>   Device B's write-handling causes it to raise an
>    outbound qemu_irq signal
>   The qemu_irq signal is connected to device A

Here mean device A is an interrupt controller?
This is special case I think.

>   Now we have reentered into device A's code
>
> That is to say, the problem is general to "device A does
> something that affects device B" links of all kinds, which

As the P2P is a normal behavior, we can't just prevent this.

Thanks,
Li Qiang
> can form loops. Self-DMA is just an easy way to find one
> category of these with the fuzzer.
>
> thanks
> -- PMM

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Peter Maydell]

On Thu, 3 Sep 2020 at 12:11, Li Qiang <liq3ea@gmail.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道：
> >
> > On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote:
> > > I think we still need to seek a way to address this issue completely.
> > >
> > > How about adding a flag in MemoryRegionOps and detect the reentrancy
> > > through that flag?
> >
> > This won't catch everything. Consider this situation:
> >   Device A makes DMA access to device B
> >   Device B's write-handling causes it to raise an
> >    outbound qemu_irq signal
> >   The qemu_irq signal is connected to device A
>
> Here mean device A is an interrupt controller?

No. Any device can have an inbound or outbound qemu_irq line.
We use them not just for actual IRQ lines but for any
situation where we need to pass an on-or-off signal from
one device to another.

> This is special case I think.

It's an example of why looking purely at MMIO is not
sufficient. We should prefer to see if we can come up with
a design principle that works for all between-device
coordination before we implement something that is specific
to MMIO.

thanks
-- PMM

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Li Qiang]

Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午7:19写道：
>
> On Thu, 3 Sep 2020 at 12:11, Li Qiang <liq3ea@gmail.com> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道：
> > >
> > > On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote:
> > > > I think we still need to seek a way to address this issue completely.
> > > >
> > > > How about adding a flag in MemoryRegionOps and detect the reentrancy
> > > > through that flag?
> > >
> > > This won't catch everything. Consider this situation:
> > >   Device A makes DMA access to device B
> > >   Device B's write-handling causes it to raise an
> > >    outbound qemu_irq signal
> > >   The qemu_irq signal is connected to device A
> >
> > Here mean device A is an interrupt controller?
>
> No. Any device can have an inbound or outbound qemu_irq line.
> We use them not just for actual IRQ lines but for any
> situation where we need to pass an on-or-off signal from
> one device to another.

Could you please provide some example, I haven't noticed this before. Thanks.

>
> > This is special case I think.
>
> It's an example of why looking purely at MMIO is not
> sufficient. We should prefer to see if we can come up with
> a design principle that works for all between-device
> coordination before we implement something that is specific
> to MMIO.

So first we may be define a clean boundary/interface between device
coordination?

Thanks,
Li Qiang

>
> thanks
> -- PMM

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Peter Maydell]

On Thu, 3 Sep 2020 at 12:24, Li Qiang <liq3ea@gmail.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午7:19写道：
> > No. Any device can have an inbound or outbound qemu_irq line.
> > We use them not just for actual IRQ lines but for any
> > situation where we need to pass an on-or-off signal from
> > one device to another.
>
> Could you please provide some example, I haven't noticed this before.

Look at any device that calls qdev_init_gpio_in() or
qdev_init_gpio_in_named() for an example of inbound signals.
Outbound signals might be created via qdev_init_gpio_out(),
qdev_init_gpio_out_named() or sysbus_init_irq().

thanks
-- PMM

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Philippe Mathieu-Daudé]

On 9/3/20 1:28 PM, Peter Maydell wrote:
> On Thu, 3 Sep 2020 at 12:24, Li Qiang <liq3ea@gmail.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午7:19写道：
>>> No. Any device can have an inbound or outbound qemu_irq line.
>>> We use them not just for actual IRQ lines but for any
>>> situation where we need to pass an on-or-off signal from
>>> one device to another.
>>
>> Could you please provide some example, I haven't noticed this before.
> 
> Look at any device that calls qdev_init_gpio_in() or
> qdev_init_gpio_in_named() for an example of inbound signals.
> Outbound signals might be created via qdev_init_gpio_out(),
> qdev_init_gpio_out_named() or sysbus_init_irq().

Not sure if this is a valid example, but when adding:

-- >8 --
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index bca71b5934b..b8b4ba362b1 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -96,6 +96,8 @@ static void ioapic_service(IOAPICCommonState *s)
     uint32_t mask;
     uint64_t entry;

+    assert(!resettable_is_in_reset(OBJECT(s)));
+
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         mask = 1 << i;
         if (s->irr & mask) {
---

I get a MMIO write triggered from an IRQ:

(gdb) bt
#3  0x0000555558e44a12 in memory_region_write_accessor
(mr=0x61600001ab10, addr=0, value=0x7fffffffaa10, size=4, shift=0,
mask=4294967295, attrs=...) at softmmu/memory.c:482
#4  0x0000555558e4453b in access_with_adjusted_size (addr=0,
value=0x7fffffffaa10, size=4, access_size_min=1, access_size_max=4,
access_fn=
    0x555558e44600 <memory_region_write_accessor>, mr=0x61600001ab10,
attrs=...) at softmmu/memory.c:545
#5  0x0000555558e42c56 in memory_region_dispatch_write
(mr=0x61600001ab10, addr=0, data=0, op=MO_32, attrs=...) at
softmmu/memory.c:1466
#6  0x0000555558f322b3 in address_space_stl_internal (as=0x55555c0120e0
<address_space_memory>, addr=4276092928, val=0, attrs=..., result=0x0,
endian=DEVICE_LITTLE_ENDIAN)
    at memory_ldst.c.inc:315
#7  0x0000555558f32802 in address_space_stl_le (as=0x55555c0120e0
<address_space_memory>, addr=4276092928, val=0, attrs=..., result=0x0)
at memory_ldst.c.inc:353
#8  0x0000555558be2e22 in stl_le_phys (as=0x55555c0120e0
<address_space_memory>, addr=4276092928, val=0) at
/home/phil/source/qemu/include/exec/memory_ldst_phys.h.inc:103
#9  0x0000555558be0e14 in ioapic_service (s=0x61b000002a80) at
hw/intc/ioapic.c:138
#10 0x0000555558be4901 in ioapic_set_irq (opaque=0x61b000002a80,
vector=2, level=1) at hw/intc/ioapic.c:186
#11 0x00005555598769f6 in qemu_set_irq (irq=0x606000040f40, level=1) at
hw/core/irq.c:44
#12 0x00005555585fc097 in gsi_handler (opaque=0x61200000b8c0, n=0,
level=1) at hw/i386/x86.c:336
#13 0x00005555598769f6 in qemu_set_irq (irq=0x60600003db80, level=1) at
hw/core/irq.c:44
#14 0x0000555557653047 in hpet_handle_legacy_irq (opaque=0x61f000000080,
n=0, level=1) at hw/timer/hpet.c:707
#15 0x00005555598769f6 in qemu_set_irq (irq=0x606000042500, level=1) at
hw/core/irq.c:44
#16 0x00005555571c0686 in pit_irq_timer_update (s=0x616000032018,
current_time=0) at hw/timer/i8254.c:262
#17 0x00005555571c01c9 in pit_irq_control (opaque=0x616000031e80, n=0,
enable=1) at hw/timer/i8254.c:304
#18 0x00005555598769f6 in qemu_set_irq (irq=0x6060000435e0, level=1) at
hw/core/irq.c:44
#19 0x00005555576518cb in hpet_reset (d=0x61f000000080) at
hw/timer/hpet.c:690
#20 0x000055555986dfbe in device_transitional_reset (obj=0x61f000000080)
at hw/core/qdev.c:1114
#21 0x0000555559870e8e in resettable_phase_hold (obj=0x61f000000080,
opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:182
#22 0x0000555559846add in bus_reset_child_foreach (obj=0x60c00002e000,
cb=0x5555598707e0 <resettable_phase_hold>, opaque=0x0,
type=RESET_TYPE_COLD) at hw/core/bus.c:94
#23 0x0000555559873c29 in resettable_child_foreach (rc=0x60e00003e160,
obj=0x60c00002e000, cb=0x5555598707e0 <resettable_phase_hold>,
opaque=0x0, type=RESET_TYPE_COLD)
    at hw/core/resettable.c:96
#24 0x0000555559870b01 in resettable_phase_hold (obj=0x60c00002e000,
opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:173
#25 0x000055555986fbc3 in resettable_assert_reset (obj=0x60c00002e000,
type=RESET_TYPE_COLD) at hw/core/resettable.c:60
#26 0x000055555986fa6a in resettable_reset (obj=0x60c00002e000,
type=RESET_TYPE_COLD) at hw/core/resettable.c:45
#27 0x00005555598725ba in resettable_cold_reset_fn
(opaque=0x60c00002e000) at hw/core/resettable.c:269
#28 0x000055555986f9e9 in qemu_devices_reset () at hw/core/reset.c:69
#29 0x000055555865d711 in pc_machine_reset (machine=0x615000020100) at
hw/i386/pc.c:1901
#30 0x00005555589ea197 in qemu_system_reset (reason=SHUTDOWN_CAUSE_NONE)
at softmmu/vl.c:1403
#31 0x00005555589f7738 in qemu_init (argc=16, argv=0x7fffffffd278,
envp=0x7fffffffd300) at softmmu/vl.c:4458
#32 0x00005555571615fa in main (argc=16, argv=0x7fffffffd278,
envp=0x7fffffffd300) at softmmu/main.c:49

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Peter Maydell]

On Thu, 3 Sep 2020 at 14:36, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Not sure if this is a valid example, but when adding:
>
> -- >8 --
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index bca71b5934b..b8b4ba362b1 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -96,6 +96,8 @@ static void ioapic_service(IOAPICCommonState *s)
>      uint32_t mask;
>      uint64_t entry;
>
> +    assert(!resettable_is_in_reset(OBJECT(s)));
> +
>      for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>          mask = 1 << i;
>          if (s->irr & mask) {
> ---
>
> I get a MMIO write triggered from an IRQ:

Yeah, IRQs can trigger MMIO writes. In this case one underlying
problem is that the hpet_reset() code is asserting a qemu_irq
in a reset phase that it should not, because it's an old-style
reset function and not a new-style 3-phase one (which would
do the assertion of the IRQ only in the 3rd phase). I don't
think this is a case of ending up with a recursive re-entry
into the code of the original device, though.

thanks
-- PMM

Re: [RFC 0/3] try to solve the DMA to MMIO issue

[Jason Wang]

On 2020/9/3 下午7:19, Peter Maydell wrote:
> On Thu, 3 Sep 2020 at 12:11, Li Qiang <liq3ea@gmail.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> 于2020年9月3日周四 下午6:53写道：
>>> On Thu, 3 Sep 2020 at 04:55, Jason Wang <jasowang@redhat.com> wrote:
>>>> I think we still need to seek a way to address this issue completely.
>>>>
>>>> How about adding a flag in MemoryRegionOps and detect the reentrancy
>>>> through that flag?
>>> This won't catch everything. Consider this situation:
>>>    Device A makes DMA access to device B
>>>    Device B's write-handling causes it to raise an
>>>     outbound qemu_irq signal
>>>    The qemu_irq signal is connected to device A
>> Here mean device A is an interrupt controller?
> No. Any device can have an inbound or outbound qemu_irq line.
> We use them not just for actual IRQ lines but for any
> situation where we need to pass an on-or-off signal from
> one device to another.
>
>> This is special case I think.
> It's an example of why looking purely at MMIO is not
> sufficient. We should prefer to see if we can come up with
> a design principle that works for all between-device
> coordination before we implement something that is specific
> to MMIO.


As discussed, maybe we can track the pending operations in device itself 
and check it in all the possible entry of device codes (irq, MMIO or 
what ever else). This may be easier for stable backport.

Thanks


>
> thanks
> -- PMM
>