Re: [PATCH 1/2] hw/ide/core.c (cmd_read_native_max): Avoid limited device parameters
On 230112 0412, Lev Kujawski wrote: > > John Snow writes: > > > On Mon, Oct 10, 2022 at 4:52 AM Lev Kujawski wrote: > >> > >> Always use the native CHS device parameters for the ATA commands READ > >> NATIVE MAX ADDRESS and READ NATIVE MAX ADDRESS EXT, not those limited > >> by the ATA command INITIALIZE_DEVICE_PARAMETERS (introduced in patch > >> 176e4961, hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS > >> command, 2022-07-07.) > >> > >> As stated by the ATA/ATAPI specification, "[t]he native maximum is the > >> highest address accepted by the device in the factory default > >> condition." Therefore this patch substitutes the native values in > >> drive_heads and drive_sectors before calling ide_set_sector(). > >> > >> One consequence of the prior behavior was that setting zero sectors > >> per track could lead to an FPE within ide_set_sector(). Thanks to > >> Alexander Bulekov for reporting this issue. > >> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1243 > >> Signed-off-by: Lev Kujawski > > > > Does this need attention? > > > > --js > > > > Hi John, > > This patch needs to be merged to mitigate issue 1243, which is still > present within QEMU master as of aa96ab7c9d. > > Thanks, Lev > Ping. oss-fuzz re-discovered this bug.
[PATCH] async: avoid use-after-free on re-entrancy guard
A BH callback can free the BH, causing a use-after-free in aio_bh_call.
Fix that by keeping a local copy of the re-entrancy guard pointer.
Buglink: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58513
Fixes: 9c86c97f12 ("async: Add an optional reentrancy guard to the BH API")
Signed-off-by: Alexander Bulekov
---
util/async.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/util/async.c b/util/async.c
index 9df7674b4e..055070ffbd 100644
--- a/util/async.c
+++ b/util/async.c
@@ -156,18 +156,20 @@ void aio_bh_call(QEMUBH *bh)
{
bool last_engaged_in_io = false;
-if (bh->reentrancy_guard) {
-last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
-if (bh->reentrancy_guard->engaged_in_io) {
+/* Make a copy of the guard-pointer as cb may free the bh */
+MemReentrancyGuard *reentrancy_guard = bh->reentrancy_guard;
+if (reentrancy_guard) {
+last_engaged_in_io = reentrancy_guard->engaged_in_io;
+if (reentrancy_guard->engaged_in_io) {
trace_reentrant_aio(bh->ctx, bh->name);
}
-bh->reentrancy_guard->engaged_in_io = true;
+reentrancy_guard->engaged_in_io = true;
}
bh->cb(bh->opaque);
-if (bh->reentrancy_guard) {
-bh->reentrancy_guard->engaged_in_io = last_engaged_in_io;
+if (reentrancy_guard) {
+reentrancy_guard->engaged_in_io = last_engaged_in_io;
}
}
--
2.39.0
[PATCH v10 2/8] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index e267d918fd..89bbc536f9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -323,9 +325,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -334,7 +338,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, c
[PATCH v10 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 80ce1e9a93..f1c0eb7dfc 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render
Re: [PATCH v9 7/8] memory: abort on re-entrancy in debug builds
On 230426 1219, Alexander Bulekov wrote: > This is useful for using unit-tests/fuzzing to detect bugs introduced by > the re-entrancy guard mechanism into devices that are intentionally > re-entrant. > > Signed-off-by: Alexander Bulekov > Reviewed-by: Thomas Huth > --- This doesn't actually do anything right now. Doesn't look like DEBUG is defined with --enable-debug Any suggestion for how to make re-entrancy louder/fatal on debug/developer builds? Maybe we can just replace the trace event with an unconditional log-message?
[PATCH v9 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 80ce1e9a93..f1c0eb7dfc 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render
[PATCH v9 2/8] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index e267d918fd..89bbc536f9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -323,9 +325,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -334,7 +338,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, c
[PATCH v9 7/8] memory: abort on re-entrancy in debug builds
This is useful for using unit-tests/fuzzing to detect bugs introduced by
the re-entrancy guard mechanism into devices that are intentionally
re-entrant.
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
softmmu/memory.c | 3 +++
util/async.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af9365bb81..d038633a6c 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -547,6 +547,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+#ifdef DEBUG
+abort();
+#endif
return MEMTX_ACCESS_ERROR;
}
mr->dev->mem_reentrancy_guard.engaged_in_io = true;
diff --git a/util/async.c b/util/async.c
index a9b528c370..2dc9389e0d 100644
--- a/util/async.c
+++ b/util/async.c
@@ -160,6 +160,9 @@ void aio_bh_call(QEMUBH *bh)
last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
if (bh->reentrancy_guard->engaged_in_io) {
trace_reentrant_aio(bh->ctx, bh->name);
+#ifdef DEBUG
+abort();
+#endif
}
bh->reentrancy_guard->engaged_in_io = true;
}
--
2.39.0
[PATCH v8 8/8] memory: abort on re-entrancy in debug builds
This is useful for using unit-tests/fuzzing to detect bugs introduced by
the re-entrancy guard mechanism into devices that are intentionally
re-entrant.
Signed-off-by: Alexander Bulekov
---
softmmu/memory.c | 3 +++
util/async.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index a11ee3e30d..5390f91db6 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -547,6 +547,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+#ifdef DEBUG
+abort();
+#endif
return MEMTX_ACCESS_ERROR;
}
mr->dev->mem_reentrancy_guard.engaged_in_io = true;
diff --git a/util/async.c b/util/async.c
index a9b528c370..2dc9389e0d 100644
--- a/util/async.c
+++ b/util/async.c
@@ -160,6 +160,9 @@ void aio_bh_call(QEMUBH *bh)
last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
if (bh->reentrancy_guard->engaged_in_io) {
trace_reentrant_aio(bh->ctx, bh->name);
+#ifdef DEBUG
+abort();
+#endif
}
bh->reentrancy_guard->engaged_in_io = true;
}
--
2.39.0
[PATCH v8 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..c0460c4ef1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render
[PATCH v8 2/8] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 543717f294..db6f23c619 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -331,9 +333,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -342,7 +346,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, c
[PATCH v7 2/6] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 8fba6a3584..3e3bdb9352 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -331,9 +333,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -342,7 +346,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c25f390696..84d1ce57f0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
void qemu_fd_register(int fd);
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opa
[PATCH v7 4/6] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..c0460c4ef1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
[PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Alexander Bulekov
---
hw/9pfs/xen-9p-backend.c| 4 +++-
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/core.c | 3 ++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
24 files changed, 63 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 65c4979c3c..f077c1b255 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&DEVICE(xen_9pdev)->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 2785b9e849..e31806b317 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -632,8 +632,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..c0460c4ef1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
-qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, &qxl->ssd);
+qxl->update_area_bh = qemu_bh_new_guarded(qxl_render_update_area_bh, qxl,
+
&DEVICE(qxl)->mem_reentrancy_guard);
+qxl->ssd.cursor_bh = qemu_bh_new_guarded(qemu_spice_cursor_refresh_bh,
&qxl->ssd,
+
[PATCH v6 2/4] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 8fba6a3584..3e3bdb9352 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -331,9 +333,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -342,7 +346,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c25f390696..84d1ce57f0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
void qemu_fd_register(int fd);
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f5e75a96b6..24d5413f9d 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opa
[PATCH v5 2/4] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 0f65a3cc9e..94d661ff7e 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -332,9 +334,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -343,7 +347,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c25f390696..84d1ce57f0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
void qemu_fd_register(int fd);
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f5e75a96b6..24d5413f9d 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QE
[PATCH v5 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Alexander Bulekov
---
hw/9pfs/xen-9p-backend.c| 4 +++-
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/block/virtio-blk.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/core.c | 3 ++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 35 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 65c4979c3c..f077c1b255 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&DEVICE(xen_9pdev)->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 26f965cabc..191a8c90aa 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(s)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 2785b9e849..e31806b317 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -632,8 +632,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f717550fdc..e9f516e633 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -866,8 +866,9 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool
running,
* requests will be processed while starting the data plane.
*/
if (!s->bh && !virtio_bus_ioeventfd_enabled(bus)) {
-s->bh = aio_bh_new(blk_get_aio_context(s->conf.conf.blk),
- virtio_blk_dma_restart_bh, s);
+s->bh = aio_bh_new_guarded(blk_get_aio_context(s->conf.conf.blk),
+ virtio_blk_dma_restart_bh, s,
+ &DEVICE(s)->mem_reentrancy_guard);
blk_inc_in_flight(s->conf.conf.blk);
qemu_bh_schedule(s->bh);
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 6772849dec..67efa3c3ef 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2223,11 +2223,14 @@ sta
Re: [PATCH v4 2/3] async: Add an optional reentrancy guard to the BH API
On 230125 1624, Stefan Hajnoczi wrote: > On Thu, Jan 19, 2023 at 02:03:07AM -0500, Alexander Bulekov wrote: > > Devices can pass their MemoryReentrancyGuard (from their DeviceState), > > when creating new BHes. Then, the async API will toggle the guard > > before/after calling the BH call-back. This prevents bh->mmio reentrancy > > issues. > > > > Signed-off-by: Alexander Bulekov > > --- > > docs/devel/multiple-iothreads.txt | 2 ++ > > include/block/aio.h | 18 -- > > include/qemu/main-loop.h | 7 +-- > > tests/unit/ptimer-test-stubs.c| 3 ++- > > util/async.c | 12 +++- > > util/main-loop.c | 5 +++-- > > 6 files changed, 39 insertions(+), 8 deletions(-) > > > > diff --git a/docs/devel/multiple-iothreads.txt > > b/docs/devel/multiple-iothreads.txt > > index 343120f2ef..e4fafed9d9 100644 > > --- a/docs/devel/multiple-iothreads.txt > > +++ b/docs/devel/multiple-iothreads.txt > > @@ -61,6 +61,7 @@ There are several old APIs that use the main loop > > AioContext: > > * LEGACY qemu_aio_set_event_notifier() - monitor an event notifier > > * LEGACY timer_new_ms() - create a timer > > * LEGACY qemu_bh_new() - create a BH > > + * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy > > guard > > * LEGACY qemu_aio_wait() - run an event loop iteration > > > > Since they implicitly work on the main loop they cannot be used in code > > that > > @@ -72,6 +73,7 @@ Instead, use the AioContext functions directly (see > > include/block/aio.h): > > * aio_set_event_notifier() - monitor an event notifier > > * aio_timer_new() - create a timer > > * aio_bh_new() - create a BH > > + * aio_bh_new_guarded() - create a BH with a device re-entrancy guard > > * aio_poll() - run an event loop iteration > > > > The AioContext can be obtained from the IOThread using > > diff --git a/include/block/aio.h b/include/block/aio.h > > index 0f65a3cc9e..94d661ff7e 100644 > > --- a/include/block/aio.h > > +++ b/include/block/aio.h > > @@ -23,6 +23,8 @@ > > #include "qemu/thread.h" > > #include "qemu/timer.h" > > #include "block/graph-lock.h" > > +#include "hw/qdev-core.h" > > + > > > > typedef struct BlockAIOCB BlockAIOCB; > > typedef void BlockCompletionFunc(void *opaque, int ret); > > @@ -332,9 +334,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, > > QEMUBHFunc *cb, void *opaque, > > * is opaque and must be allocated prior to its use. > > * > > * @name: A human-readable identifier for debugging purposes. > > + * @reentrancy_guard: A guard set when entering a cb to prevent > > + * device-reentrancy issues > > */ > > QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque, > > -const char *name); > > +const char *name, MemReentrancyGuard > > *reentrancy_guard); > > > > /** > > * aio_bh_new: Allocate a new bottom half structure > > @@ -343,7 +347,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc > > *cb, void *opaque, > > * string. > > */ > > #define aio_bh_new(ctx, cb, opaque) \ > > -aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb))) > > +aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL) > > + > > +/** > > + * aio_bh_new_guarded: Allocate a new bottom half structure with a > > + * reentrancy_guard > > + * > > + * A convenience wrapper for aio_bh_new_full() that uses the cb as the name > > + * string. > > + */ > > +#define aio_bh_new_guarded(ctx, cb, opaque, guard) \ > > +aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard) > > > > /** > > * aio_notify: Force processing of pending events. > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > > index c25f390696..84d1ce57f0 100644 > > --- a/include/qemu/main-loop.h > > +++ b/include/qemu/main-loop.h > > @@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int > > ms); > > > > void qemu_fd_register(int fd); > > > > +#define qemu_bh_new_guarded(cb, opaque, guard) \ > > +qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard) > > #define qemu_bh_new(cb, opaque) \ > > -qemu_bh_new_full((cb), (opaque), (stringify(cb))) > > -QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name); > > +q
[PATCH v4 2/3] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 2 ++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 12 +++-
util/main-loop.c | 5 +++--
6 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..e4fafed9d9 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,6 +73,7 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
The AioContext can be obtained from the IOThread using
diff --git a/include/block/aio.h b/include/block/aio.h
index 0f65a3cc9e..94d661ff7e 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -332,9 +334,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -343,7 +347,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c25f390696..84d1ce57f0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
void qemu_fd_register(int fd);
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f5e75a96b6..24d5413f9d 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard)
{
QEMUBH *bh = g_new(QEMUBH, 1);
diff --git a/util/async.c b/util/async.c
index 14d63b3091..08924c3212 100644
--- a/util/async.c
+++ b/util/async.c
@@ -65,6 +65,7 @@ struct QEMUBH {
void *opaque;
QSLIST_ENTRY(QEMUBH) next;
unsigned flags;
+MemReentrancyGuard *reentrancy_guard;
};
/* Called concurrently from any thread */
@@ -133,7 +134,7 @@ void a
[PATCH v4 3/3] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Signed-off-by: Alexander Bulekov
---
hw/9pfs/xen-9p-backend.c| 4 +++-
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/block/virtio-blk.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/core.c | 3 ++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 35 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 65c4979c3c..f077c1b255 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -441,7 +441,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&DEVICE(xen_9pdev)->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 26f965cabc..191a8c90aa 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(s)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 2785b9e849..e31806b317 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -632,8 +632,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f717550fdc..e9f516e633 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -866,8 +866,9 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool
running,
* requests will be processed while starting the data plane.
*/
if (!s->bh && !virtio_bus_ioeventfd_enabled(bus)) {
-s->bh = aio_bh_new(blk_get_aio_context(s->conf.conf.blk),
- virtio_blk_dma_restart_bh, s);
+s->bh = aio_bh_new_guarded(blk_get_aio_context(s->conf.conf.blk),
+ virtio_blk_dma_restart_bh, s,
+ &DEVICE(s)->mem_reentrancy_guard);
blk_inc_in_flight(s->conf.conf.blk);
qemu_bh_schedule(s->bh);
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 6772849dec..67efa3c3ef 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2223,11 +2223,14 @@ static void qxl_realize_commo
Re: [PATCH-for-7.2 1/2] hw/sd/sdhci: Do not set Buf Wr Ena before writing block (CVE-2022-3872)
On 221108 1225, Alexander Bulekov wrote:
> On 221107 2312, Philippe Mathieu-Daudé wrote:
> > When sdhci_write_block_to_card() is called to transfer data from
> > the FIFO to the SD bus, the data is already present in the buffer
> > and we have to consume it directly.
> >
> > See the description of the 'Buffer Write Enable' bit from the
> > 'Present State' register (prnsts::SDHC_SPACE_AVAILABLE) in Table
> > 2.14 from the SDHCI spec v2:
> >
> > Buffer Write Enable
> >
> > This status is used for non-DMA write transfers.
> >
> > The Host Controller can implement multiple buffers to transfer
> > data efficiently. This read only flag indicates if space is
> > available for write data. If this bit is 1, data can be written
> > to the buffer. A change of this bit from 1 to 0 occurs when all
> > the block data is written to the buffer. A change of this bit
> > from 0 to 1 occurs when top of block data can be written to the
> > buffer and generates the Buffer Write Ready interrupt.
> >
> > In our case, we do not want to overwrite the buffer, so we want
> > this bit to be 0, then set it to 1 once the data is written onto
> > the bus.
> >
> > This is probably a copy/paste error from commit d7dfca0807
> > ("hw/sdhci: introduce standard SD host controller").
> >
> > Reproducer:
> > https://lore.kernel.org/qemu-devel/caa8xkjxrms0fkr28akvnnpyatm0y0b+5fichpsrhd+mugnu...@mail.gmail.com/
> >
> > Fixes: CVE-2022-3872
> > Reported-by: RivenDell
> > Reported-by: Siqi Chen
> > Reported-by: ningqiang
> > Signed-off-by: Philippe Mathieu-Daudé
>
> Seems like OSS-Fuzz also found this, not sure why it never made it into
> a gitlab issue:
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=45986#c4
>
> Slightly shorter reproducer:
>
> cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
> 512M -nodefaults -device sdhci-pci -device sd-card,drive=mydrive -drive \
> if=none,index=0,file=null-co://,format=raw,id=mydrive -nographic -qtest \
> stdio
> outl 0xcf8 0x80001010
> outl 0xcfc 0xe000
> outl 0xcf8 0x80001001
> outl 0xcfc 0x0600
> write 0xe058 0x1 0x6e
> write 0xe059 0x1 0x5a
> write 0xe028 0x1 0x10
> write 0xe02c 0x1 0x05
> write 0x5a6e 0x1 0x21
> write 0x5a75 0x1 0x20
> write 0xe005 0x1 0x02
> write 0xe00c 0x1 0x01
> write 0xe00e 0x1 0x20
> write 0xe00f 0x1 0x00
> write 0xe00c 0x1 0x00
> write 0xe020 0x1 0x00
> EOF
Hi Philippe,
I ran the fuzzer with this series applied and it found:
cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
512M -nodefaults -device sdhci-pci -device sd-card,drive=mydrive -drive \
if=none,index=0,file=null-co://,format=raw,id=mydrive -nographic -qtest \
stdio
outl 0xcf8 0x80001010
outl 0xcfc 0xe000
outl 0xcf8 0x80001004
outw 0xcfc 0x06
write 0xe028 0x1 0x08
write 0xe02c 0x1 0x05
write 0xe005 0x1 0x02
write 0x0 0x1 0x21
write 0x3 0x1 0x20
write 0xe00c 0x1 0x01
write 0xe00e 0x1 0x20
write 0xe00f 0x1 0x00
write 0xe00c 0x1 0x20
write 0xe020 0x1 0x00
EOF
The crash seems very similar, but it looks like instead of
SDHC_TRNS_READ this reproducer sets SDHC_TRNS_MULTI
-Alex
Re: [PATCH-for-7.2 1/2] hw/sd/sdhci: Do not set Buf Wr Ena before writing block (CVE-2022-3872)
On 221107 2312, Philippe Mathieu-Daudé wrote:
> When sdhci_write_block_to_card() is called to transfer data from
> the FIFO to the SD bus, the data is already present in the buffer
> and we have to consume it directly.
>
> See the description of the 'Buffer Write Enable' bit from the
> 'Present State' register (prnsts::SDHC_SPACE_AVAILABLE) in Table
> 2.14 from the SDHCI spec v2:
>
> Buffer Write Enable
>
> This status is used for non-DMA write transfers.
>
> The Host Controller can implement multiple buffers to transfer
> data efficiently. This read only flag indicates if space is
> available for write data. If this bit is 1, data can be written
> to the buffer. A change of this bit from 1 to 0 occurs when all
> the block data is written to the buffer. A change of this bit
> from 0 to 1 occurs when top of block data can be written to the
> buffer and generates the Buffer Write Ready interrupt.
>
> In our case, we do not want to overwrite the buffer, so we want
> this bit to be 0, then set it to 1 once the data is written onto
> the bus.
>
> This is probably a copy/paste error from commit d7dfca0807
> ("hw/sdhci: introduce standard SD host controller").
>
> Reproducer:
> https://lore.kernel.org/qemu-devel/caa8xkjxrms0fkr28akvnnpyatm0y0b+5fichpsrhd+mugnu...@mail.gmail.com/
>
> Fixes: CVE-2022-3872
> Reported-by: RivenDell
> Reported-by: Siqi Chen
> Reported-by: ningqiang
> Signed-off-by: Philippe Mathieu-Daudé
Seems like OSS-Fuzz also found this, not sure why it never made it into
a gitlab issue:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=45986#c4
Slightly shorter reproducer:
cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
512M -nodefaults -device sdhci-pci -device sd-card,drive=mydrive -drive \
if=none,index=0,file=null-co://,format=raw,id=mydrive -nographic -qtest \
stdio
outl 0xcf8 0x80001010
outl 0xcfc 0xe000
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0xe058 0x1 0x6e
write 0xe059 0x1 0x5a
write 0xe028 0x1 0x10
write 0xe02c 0x1 0x05
write 0x5a6e 0x1 0x21
write 0x5a75 0x1 0x20
write 0xe005 0x1 0x02
write 0xe00c 0x1 0x01
write 0xe00e 0x1 0x20
write 0xe00f 0x1 0x00
write 0xe00c 0x1 0x00
write 0xe020 0x1 0x00
EOF
Re: [PATCH 0/8] tests: Make expliction defaults for tests
On 220902 1851, Juan Quintela wrote: > Hi > > For a long, long time I have had local hacks on my tree to be able to > run "make tests" when I have a minimal configure guest. This is a > first try to upstream some of it. > > - by default we always setup -display none (it already was the > default, but some places added it anyways) > > - by default we always setup -net none. Not clear what was the > default, but no tests use the default net, so it is safe change and > now it is explicit. > > - by default we always setup -vga none. This is a complete difference > can of worms. Every tests that use vga already set vga correctly, > so this is quite obvious, right? Now they are acpi tables. They > are a mess. And basically this means remove a device for each one > of them. Why going through all the trouble? Because while I am > develping, I normall compile out vga. > > - Fix several error strings that were set with copy paste. > > - replication test requires CONFIG_REPLICATION. > - test-crypto-secret requires CONFIG_SECRET_KEYRING. > > Please review. Except for the acpi changes (that I hope I have done > right following the instructions) the rest is quite obvious. I think this might break some of the fuzz regression tests, because they have "baked-in" PCI configuration commands with hard-coded PCI addresses, which will shift around if some device is removed (e.g. with -net none). Probably the fix is to add addr=... to the -device parameter in the fuzz tests to keep the PCI address stable. -Alex > > Later, Juan. > > Juan Quintela (8): > qtest: "-display none" is set in qtest_init() > qtest: Set "-net none" in qtest_init() > tests/acpi: The new default is -vga none > tests/qtest: Add -vga none by default > tests/acpi: Regenerate all needed tables > tests: Fix error strings > meson-build: Enable CONFIG_REPLICATION only when replication is set > meson-build: test-crypto-secret depends on CONFIG_SECRET_KEYRING > > meson.build | 2 +- > tests/qtest/bios-tables-test.c| 2 +- > tests/qtest/boot-serial-test.c| 4 ++-- > tests/qtest/dbus-display-test.c | 2 +- > tests/qtest/display-vga-test.c| 12 ++-- > tests/qtest/e1000-test.c | 2 +- > tests/qtest/es1370-test.c | 2 +- > tests/qtest/fuzz-lsi53c895a-test.c| 2 +- > tests/qtest/fuzz-megasas-test.c | 2 +- > tests/qtest/fuzz-sb16-test.c | 6 +++--- > tests/qtest/fuzz-sdcard-test.c| 6 +++--- > tests/qtest/fuzz-virtio-scsi-test.c | 2 +- > tests/qtest/fuzz-xlnx-dp-test.c | 2 +- > tests/qtest/fuzz/generic_fuzz.c | 3 +-- > tests/qtest/fuzz/i440fx_fuzz.c| 2 +- > tests/qtest/fuzz/qos_fuzz.c | 2 +- > tests/qtest/libqtest.c| 2 ++ > tests/data/acpi/pc/DSDT | Bin 5987 -> 5992 bytes > tests/data/acpi/pc/DSDT.acpierst | Bin 5954 -> 5959 bytes > tests/data/acpi/pc/DSDT.acpihmat | Bin 7312 -> 7317 bytes > tests/data/acpi/pc/DSDT.bridge| Bin 8653 -> 8658 bytes > tests/data/acpi/pc/DSDT.cphp | Bin 6451 -> 6456 bytes > tests/data/acpi/pc/DSDT.dimmpxm | Bin 7641 -> 7646 bytes > tests/data/acpi/pc/DSDT.hpbridge | Bin 5954 -> 5959 bytes > tests/data/acpi/pc/DSDT.hpbrroot | Bin 3069 -> 3023 bytes > tests/data/acpi/pc/DSDT.ipmikcs | Bin 6059 -> 6064 bytes > tests/data/acpi/pc/DSDT.memhp | Bin 7346 -> 7351 bytes > tests/data/acpi/pc/DSDT.nohpet| Bin 5845 -> 5850 bytes > tests/data/acpi/pc/DSDT.numamem | Bin 5993 -> 5998 bytes > tests/data/acpi/pc/DSDT.roothp| Bin 6195 -> 6151 bytes > tests/data/acpi/pc/ERST.acpierst | Bin 912 -> 912 bytes > tests/data/acpi/q35/DMAR.dmar | Bin 120 -> 112 bytes > tests/data/acpi/q35/DSDT | Bin 8274 -> 8228 bytes > tests/data/acpi/q35/DSDT.acpierst | Bin 8291 -> 8245 bytes > tests/data/acpi/q35/DSDT.acpihmat | Bin 9599 -> 9553 bytes > tests/data/acpi/q35/DSDT.applesmc | Bin 8320 -> 8274 bytes > tests/data/acpi/q35/DSDT.bridge | Bin 10988 -> 10944 bytes > tests/data/acpi/q35/DSDT.cphp | Bin 8738 -> 8692 bytes > tests/data/acpi/q35/DSDT.cxl | Bin 9600 -> 9502 bytes > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9928 -> 9882 bytes > tests/data/acpi/q35/DSDT.ipmibt | Bin 8349 -> 8303 bytes > tests/data/acpi/q35/DSDT.ipmismbus| Bin 8363 -> 8317 bytes > tests/data/acpi/q35/DSDT.ivrs | Bin 8291 -> 8245 bytes > tests/data/acpi/q35/DSDT.memhp| Bin 9633 -> 9587 bytes > tests/data/acpi/q35/DSDT.mmio64 | Bin 9404 -> 9358 bytes > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8568 -> 8524 bytes > tests/data/acpi/q35/DSDT.nohpet | Bin 8132 -> 8086 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 8280 -> 8234 bytes > tests/data/acpi/q35/DSDT.pvpanic-isa | Bin 8375 -> 8329 bytes > tests/data/acpi/q35/DSDT.t
Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
On 211123 1449, Philippe Mathieu-Daudé wrote: > On 11/23/21 14:42, Hanna Reitz wrote: > > On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: > >> From: Alexander Bulekov > >> > >> Without the previous commit, when running 'make check-qtest-i386' > >> with QEMU configured with '--enable-sanitizers' we get: > >> > >> AddressSanitizer:DEADLYSIGNAL > >> = > >> ==287878==ERROR: AddressSanitizer: SEGV on unknown address > >> 0x0344 > >> ==287878==The signal is caused by a WRITE memory access. > >> ==287878==Hint: address points to the zero page. > >> #0 0x564b2e5bac27 in blk_inc_in_flight > >> block/block-backend.c:1346:5 > >> #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 > >> #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 > >> #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 > >> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 > >> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 > >> > >> Add the reproducer for CVE-2021-20196. > >> > >> Signed-off-by: Alexander Bulekov > >> Message-Id: <20210319050906.14875-2-alx...@bu.edu> > >> [PMD: Rebased, use global test_image] > >> Reviewed-by: Darren Kenny > >> Signed-off-by: Philippe Mathieu-Daudé > >> --- > >> tests/qtest/fdc-test.c | 21 + > >> 1 file changed, 21 insertions(+) > > > > Not sure if I’m doing something wrong, but: > > > > Using the global test_image brings a problem, namely that this test > > fails unconditionally (for me at least...?), with the reason being that > > the global QEMU instance (launched by qtest_start(), quit by > > qtest_end()) still has that image open, so by launching a second > > instance concurrently, I get this: > > > > qemu-system-x86_64: Failed to get "write" lock > > Is another process using the image [/tmp/qtest.xV4IxX]? > > Hmm I had too many odd problems running qtests in parallel so I > switched to 'make check-qtest -j1' more than 1 year ago, which > is probably why I haven't noticed that issue. > > Using another 'test_image' seems against code reuse principle, > but at this point in release, duplicating it is simpler. Someone > will clean that later =) Maybe it makes sense to run this with -snasphot ? > > > So either we need to use a different image file, or we need to quit the > > global instance before using it (e.g. putting a qtest_end() at the > > beginning of test_cve_*()), although the latter just seems wrong. > > > > Second, I can’t make this test fail. When I apply this patch first (to > > master) and run it, I don’t get a SIGSEGV. > > Is your QEMU built with --enable-sanitizers ? >
Re: [PATCH-for-6.2 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
On 28 1257, Philippe Mathieu-Daudé wrote:
> Add the reproducer from https://gitlab.com/qemu-project/qemu/-/issues/339
>
> Without the previous commit, when running 'make check-qtest-i386'
> with QEMU configured with '--enable-sanitizers' we get:
>
> ==4028352==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x61962a00 at pc 0x5626d03c491a bp 0x7ffdb4199410 sp 0x7ffdb4198bc0
> READ of size 786432 at 0x61962a00 thread T0
> #0 0x5626d03c4919 in __asan_memcpy (qemu-system-i386+0x1e65919)
> #1 0x5626d1c023cc in flatview_write_continue softmmu/physmem.c:2787:13
> #2 0x5626d1bf0c0f in flatview_write softmmu/physmem.c:2822:14
> #3 0x5626d1bf0798 in address_space_write softmmu/physmem.c:2914:18
> #4 0x5626d1bf0f37 in address_space_rw softmmu/physmem.c:2924:16
> #5 0x5626d1bf14c8 in cpu_physical_memory_rw softmmu/physmem.c:2933:5
> #6 0x5626d0bd5649 in cpu_physical_memory_write
> include/exec/cpu-common.h:82:5
> #7 0x5626d0bd0a07 in i8257_dma_write_memory hw/dma/i8257.c:452:9
> #8 0x5626d09f825d in fdctrl_transfer_handler hw/block/fdc.c:1616:13
> #9 0x5626d0a048b4 in fdctrl_start_transfer hw/block/fdc.c:1539:13
> #10 0x5626d09f4c3e in fdctrl_write_data hw/block/fdc.c:2266:13
> #11 0x5626d09f22f7 in fdctrl_write hw/block/fdc.c:829:9
> #12 0x5626d1c20bc5 in portio_write softmmu/ioport.c:207:17
>
> 0x61962a00 is located 0 bytes to the right of 512-byte region
> [0x61962800,0x61962a00)
> allocated by thread T0 here:
> #0 0x5626d03c66ec in posix_memalign (qemu-system-i386+0x1e676ec)
> #1 0x5626d2b988d4 in qemu_try_memalign util/oslib-posix.c:210:11
> #2 0x5626d2b98b0c in qemu_memalign util/oslib-posix.c:226:27
> #3 0x5626d09fbaf0 in fdctrl_realize_common hw/block/fdc.c:2341:20
> #4 0x5626d0a150ed in isabus_fdc_realize hw/block/fdc-isa.c:113:5
> #5 0x5626d2367935 in device_set_realized hw/core/qdev.c:531:13
>
> SUMMARY: AddressSanitizer: heap-buffer-overflow
> (qemu-system-i386+0x1e65919) in __asan_memcpy
> Shadow bytes around the buggy address:
> 0x0c32800044f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c3280004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c3280004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c3280004520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c3280004530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x0c3280004540:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c3280004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c3280004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c3280004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c3280004580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c3280004590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Heap left redzone: fa
> Freed heap region: fd
> ==4028352==ABORTING
>
> Reported-by: Alexander Bulekov
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/qtest/fdc-test.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> index 26b69f7c5cd..f164d972d10 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -546,6 +546,25 @@ static void fuzz_registers(void)
> }
> }
>
> +static void test_cve_2021_3507(void)
> +{
> + QTestState *s;
> +
> +s = qtest_initf("-nographic -m 32M -nodefaults "
> +"-drive file=%s,format=raw,if=floppy", test_image);
Does it make sense to run this with -snapshot ?
Reviewed-by: Alexander Bulekov
> +qtest_outl(s, 0x9, 0x0a0206);
> +qtest_outw(s, 0x3f4, 0x1600);
> +qtest_outw(s, 0x3f4, 0x);
> +qtest_outw(s, 0x3f4, 0x);
> +qtest_outw(s, 0x3f4, 0x);
> +qtest_outw(s, 0x3f4, 0x0200);
> +qtest_outw(s, 0x3f4, 0x0200);
> +qtest_outw(s, 0x3f4, 0x);
> +qtest_outw(s, 0x3f4, 0x);
> +qtest_outw(s, 0x3f4, 0x);
> +qtest_quit(s);
> +}
> +
> int main(int argc, char **argv)
> {
> int fd;
> @@ -576,6 +595,7 @@ int main(int argc, char **argv)
> qtest_add_func("/fdc/read_no_dma_18", test_read_no_dma_18);
> qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19);
> qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
> +qtest_add_func("/fdc/fuzz/cve_2021_3507", test_cve_2021_3507);
>
> ret = g_test_run();
>
> --
> 2.31.1
>
Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API
On 210909 0120, Philippe Mathieu-Daudé wrote: > Hi, > > This series is experimental! The goal is to better limit the > boundary of what code is considerated security critical, and > what is less critical (but still important!). > > This approach was quickly discussed few months ago with Markus > then Daniel. Instead of classifying the code on a file path > basis (see [1]), we insert (runtime) hints into the code > (which survive code movement). Offending unsafe code can > taint the global security policy. By default this policy > is 'none': the current behavior. It can be changed on the > command line to 'warn' to display warnings, and to 'strict' > to prohibit QEMU running with a tainted policy. > > As examples I started implementing unsafe code taint from > 3 different pieces of code: > - accelerators (KVM and Xen in allow-list) > - block drivers (vvfat and parcial null-co in deny-list) > - qdev (hobbyist devices regularly hit by fuzzer) Just looking through the list of hci, storage, network and graphics devices available on i386 to see which others are potential good candidates for this tag. Obviously a lot of guesswork here: USB devices: name "ich9-usb-ehci1", bus PCI name "ich9-usb-ehci2", bus PCI name "ich9-usb-uhci1", bus PCI name "ich9-usb-uhci2", bus PCI name "ich9-usb-uhci3", bus PCI name "ich9-usb-uhci4", bus PCI name "ich9-usb-uhci5", bus PCI name "ich9-usb-uhci6", bus PCI name "nec-usb-xhci", bus PCI name "pci-ohci", bus PCI, desc "Apple USB Controller" name "piix3-usb-uhci", bus PCI name "piix4-usb-uhci", bus PCI name "qemu-xhci", bus PCI name "usb-ehci", bus PCI Not sure about these. Maybe ohci isn't sensitive? Storage devices: === Sensitive === name "floppy", bus floppy-bus, desc "virtual floppy drive" name "ide-cd", bus IDE, desc "virtual IDE CD-ROM" name "ide-hd", bus IDE, desc "virtual IDE disk" name "isa-fdc", bus ISA, desc "virtual floppy controller" name "isa-ide", bus ISA name "piix3-ide", bus PCI name "piix3-ide-xen", bus PCI name "piix4-ide", bus PCI name "scsi-block", bus SCSI, desc "SCSI block device passthrough" name "scsi-cd", bus SCSI, desc "virtual SCSI CD-ROM" name "scsi-generic", bus SCSI, desc "pass through generic scsi device (/dev/sg*)" name "scsi-hd", bus SCSI, desc "virtual SCSI disk" name "vhost-scsi", bus virtio-bus name "vhost-scsi-pci", bus PCI name "vhost-scsi-pci-non-transitional", bus PCI name "vhost-scsi-pci-transitional", bus PCI name "vhost-user-blk", bus virtio-bus name "vhost-user-blk-pci", bus PCI name "vhost-user-blk-pci-non-transitional", bus PCI name "vhost-user-blk-pci-transitional", bus PCI name "vhost-user-fs-device", bus virtio-bus name "vhost-user-fs-pci", bus PCI name "vhost-user-scsi", bus virtio-bus name "vhost-user-scsi-pci", bus PCI name "vhost-user-scsi-pci-non-transitional", bus PCI name "vhost-user-scsi-pci-transitional", bus PCI name "virtio-9p-device", bus virtio-bus name "virtio-9p-pci", bus PCI, alias "virtio-9p" name "virtio-9p-pci-non-transitional", bus PCI name "virtio-9p-pci-transitional", bus PCI name "virtio-blk-device", bus virtio-bus name "virtio-blk-pci", bus PCI, alias "virtio-blk" name "virtio-blk-pci-non-transitional", bus PCI name "virtio-blk-pci-transitional", bus PCI name "virtio-pmem", bus virtio-bus name "virtio-scsi-device", bus virtio-bus name "virtio-scsi-pci", bus PCI, alias "virtio-scsi" name "virtio-scsi-pci-non-transitional", bus PCI name "virtio-scsi-pci-transitional", bus PCI === Tainting/Not Sensitive === name "am53c974", bus PCI, desc "AMD Am53c974 PCscsi-PCI SCSI adapter" name "dc390", bus PCI, desc "Tekram DC-390 SCSI adapter" name "ich9-ahci", bus PCI, alias "ahci" name "lsi53c810", bus PCI name "lsi53c895a", bus PCI, alias "lsi" name "megasas", bus PCI, desc "LSI MegaRAID SAS 1078" name "megasas-gen2", bus PCI, desc "LSI MegaRAID SAS 2108" name "mptsas1068", bus PCI, desc "LSI SAS 1068" name "nvdimm", desc "DIMM memory module" name "nvme", bus PCI, desc "Non-Volatile Memory Express" name "nvme-ns", bus nvme-bus, desc "Virtual NVMe namespace" name "nvme-subsys", desc "Virtual NVMe subsystem" name "pvscsi", bus PCI name "sd-card", bus sd-bus name "sdhci-pci", bus PCI name "usb-bot", bus usb-bus name "usb-mtp", bus usb-bus, desc "USB Media Transfer Protocol device" name "usb-storage", bus usb-bus name "usb-uas", bus usb-bus Network devices: === Sensitive === name "e1000", bus PCI, alias "e1000-82540em", desc "Intel Gigabit Ethernet" name "e1000e", bus PCI, desc "Intel 82574L GbE Controller" name "virtio-net-device", bus virtio-bus name "virtio-net-pci", bus PCI, alias "virtio-net" name "virtio-net-pci-non-transitional", bus PCI name "virtio-net-pci-transitional", bus PCI === Tainting/Not Sensitive === name "e1000-82544gc", bus PCI, desc "Intel Gigabit Ethernet" name "e1000-82545em", bus PCI, desc "Intel Gigabit Ethernet" name "i82550", bus PCI, desc "Intel i82550 Ethernet" name "i82551", bus PCI, desc "Intel i82551 Ethernet" name "i82557a", bus PCI, desc "Intel i82557A Ethernet" name "i82557b", bu
Re: [PATCH] tests/qtest/vhost-user-blk-test: Check whether qemu-storage-daemon is available
On 210811 1159, Thomas Huth wrote: > The vhost-user-blk-test currently hangs if QTEST_QEMU_STORAGE_DAEMON_BINARY > points to a non-existing binary. Let's improve this situation by checking > for the availability of the binary first, so we can fail gracefully if > it is not accessible. > > Signed-off-by: Thomas Huth I manually removed ./storage-daemon/qemu-storage-daemon and re-ran qos-test. The test errored-out without hanging. Reviewed-by: Alexander Bulekov Tested-by: Alexander Bulekov
Re: [PATCH] storage-daemon: Add missing build dependency to the vhost-user-blk-test
On 210811 1147, Thomas Huth wrote: > vhost-user-blk-test needs the qemu-storage-deamon, otherwise it > currently hangs. So make sure that we build the daemon before running > the tests. > > Signed-off-by: Thomas Huth > --- Tested-by: Alexander Bulekov
Re: [PATCH-for-6.1 v2 0/2] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30
On 210803 0155, Philippe Mathieu-Daudé wrote: > Fix an assertion reported by OSS-Fuzz, add corresponding qtest. > > The change is (now) simple enough for the next rc. > > Since v1: > - Simplified/corrected following Peter's suggestion > > Philippe Mathieu-Daudé (2): > hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT > hw/sd/sdcard: Fix assertion accessing out-of-range addresses with > CMD30 > Fuzzed this for 20 mins, based on the OSS-Fuzz corpus, without finding anything. ./qemu-fuzz-i386 --fuzz-target=generic-fuzz-sdhci-v3 -jobs=4 -workers=4 \ -focus_function=sd_wpbits \ ~/oss-fuzz/qemu_qemu-fuzz-i386-target-generic-fuzz-sdhci-v3/ Tested-by: Alexander Bulekov Thanks! > hw/sd/sd.c | 9 - > tests/qtest/fuzz-sdcard-test.c | 36 ++ > 2 files changed, 44 insertions(+), 1 deletion(-) > > -- > 2.31.1 >
Re: [PATCH-for-6.2 3/3] hw/sd/sdcard: Rename Write Protect Group variables
On 210728 2017, Philippe Mathieu-Daudé wrote:
> 'wp_groups' holds a bitmap, rename it as 'wp_group_bmap'.
> 'wpgrps_size' is the bitmap size (in bits), rename it as
> 'wp_group_bits'.
>
> Patch created mechanically using:
>
> $ sed -i -e s/wp_groups/wp_group_bmap/ \
>-e s/wpgrps_size/wp_group_bits/ hw/sd/sd.c
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alexander Bulekov
-Alex
> ---
> hw/sd/sd.c | 28 ++--
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 273af75c1be..75dcd3f7f65 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -116,8 +116,8 @@ struct SDState {
> int32_t state;/* current card state, one of SDCardStates */
> uint32_t vhs;
> bool wp_switch;
> -unsigned long *wp_groups;
> -int32_t wpgrps_size;
> +unsigned long *wp_group_bmap;
> +int32_t wp_group_bits;
> uint64_t size;
> uint32_t blk_len;
> uint32_t multi_blk_cnt;
> @@ -567,10 +567,10 @@ static void sd_reset(DeviceState *dev)
> sd_set_cardstatus(sd);
> sd_set_sdstatus(sd);
>
> -g_free(sd->wp_groups);
> +g_free(sd->wp_group_bmap);
> sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
> -sd->wpgrps_size = sect;
> -sd->wp_groups = bitmap_new(sd->wpgrps_size);
> +sd->wp_group_bits = sect;
> +sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
> memset(sd->function_group, 0, sizeof(sd->function_group));
> sd->erase_start = INVALID_ADDRESS;
> sd->erase_end = INVALID_ADDRESS;
> @@ -673,7 +673,7 @@ static const VMStateDescription sd_vmstate = {
> VMSTATE_UINT32(card_status, SDState),
> VMSTATE_PARTIAL_BUFFER(sd_status, SDState, 1),
> VMSTATE_UINT32(vhs, SDState),
> -VMSTATE_BITMAP(wp_groups, SDState, 0, wpgrps_size),
> +VMSTATE_BITMAP(wp_group_bmap, SDState, 0, wp_group_bits),
> VMSTATE_UINT32(blk_len, SDState),
> VMSTATE_UINT32(multi_blk_cnt, SDState),
> VMSTATE_UINT32(erase_start, SDState),
> @@ -803,8 +803,8 @@ static void sd_erase(SDState *sd)
> if (sdsc) {
> /* Only SDSC cards support write protect groups */
> wpnum = sd_addr_to_wpnum(erase_addr);
> -assert(wpnum < sd->wpgrps_size);
> -if (test_bit(wpnum, sd->wp_groups)) {
> +assert(wpnum < sd->wp_group_bits);
> +if (test_bit(wpnum, sd->wp_group_bmap)) {
> sd->card_status |= WP_ERASE_SKIP;
> continue;
> }
> @@ -820,7 +820,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>
> wpnum = sd_addr_to_wpnum(addr);
>
> -for (i = 0; i < 32 && wpnum < sd->wpgrps_size - 1;
> +for (i = 0; i < 32 && wpnum < sd->wp_group_bits - 1;
> i++, wpnum++, addr += WPGROUP_SIZE) {
> if (addr >= sd->size) {
> /*
> @@ -829,7 +829,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
> */
> continue;
> }
> -if (test_bit(wpnum, sd->wp_groups)) {
> +if (test_bit(wpnum, sd->wp_group_bmap)) {
> ret |= (1 << i);
> }
> }
> @@ -869,7 +869,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
>
> static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
> {
> -return test_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
> +return test_bit(sd_addr_to_wpnum(addr), sd->wp_group_bmap);
> }
>
> static void sd_lock_command(SDState *sd)
> @@ -897,7 +897,7 @@ static void sd_lock_command(SDState *sd)
> sd->card_status |= LOCK_UNLOCK_FAILED;
> return;
> }
> -bitmap_zero(sd->wp_groups, sd->wpgrps_size);
> +bitmap_zero(sd->wp_group_bmap, sd->wp_group_bits);
> sd->csd[14] &= ~0x10;
> sd->card_status &= ~CARD_IS_LOCKED;
> sd->pwd_len = 0;
> @@ -1348,7 +1348,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> }
>
> sd->state = sd_programming_state;
> -set_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
> +set_bit(sd_addr_to_wpnum(addr), sd->wp_group_bmap);
> /* Bzzztt Operation complete. */
> sd->state = sd_transfer_state;
> return sd_r1b;
> @@ -1370,7 +1370,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> }
>
> sd->state = sd_programming_state;
> -clear_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
> +clear_bit(sd_addr_to_wpnum(addr), sd->wp_group_bmap);
> /* Bzzztt Operation complete. */
> sd->state = sd_transfer_state;
> return sd_r1b;
> --
> 2.31.1
>
Re: [PATCH-for-6.1 1/3] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
On 210728 2017, Philippe Mathieu-Daudé wrote:
> Per the 'Physical Layer Simplified Specification Version 3.01',
> Table 4-22: 'Block Oriented Write Protection Commands'
>
> SEND_WRITE_PROT (CMD30)
>
> If the card provides write protection features, this command asks
> the card to send the status of the write protection bits [1].
>
> [1] 32 write protection bits (representing 32 write protect groups
> starting at the specified address) [...]
> The last (least significant) bit of the protection bits corresponds
> to the first addressed group. If the addresses of the last groups
> are outside the valid range, then the corresponding write protection
> bits shall be set to 0.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alexander Bulekov
-Alex
> ---
> hw/sd/sd.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1f964e022b1..707dcc12a14 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>
> for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
> assert(wpnum < sd->wpgrps_size);
> -if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
> +if (addr >= sd->size) {
> +/*
> + * If the addresses of the last groups are outside the valid
> range,
> + * then the corresponding write protection bits shall be set to
> 0.
> + */
> +continue;
> +}
> +if (test_bit(wpnum, sd->wp_groups)) {
> ret |= (1 << i);
> }
> }
> --
> 2.31.1
>
Re: [PATCH 3/3] hw/sd: Check for valid address range in SEND_WRITE_PROT (CMD30)
On 210702 1759, Philippe Mathieu-Daudé wrote:
> OSS-Fuzz found sending illegal addresses when querying the write
> protection bits triggers an assertion:
>
> qemu-fuzz-i386: hw/sd/sd.c:824: uint32_t sd_wpbits(SDState *, uint64_t):
> Assertion `wpnum < sd->wpgrps_size' failed.
> ==11578== ERROR: libFuzzer: deadly signal
> #8 0x7628e091 in __assert_fail
> #9 0x588f1a3c in sd_wpbits hw/sd/sd.c:824:9
> #10 0x588dd271 in sd_normal_command hw/sd/sd.c:1383:38
> #11 0x588d777c in sd_do_command hw/sd/sd.c
> #12 0x58cb25a0 in sdbus_do_command hw/sd/core.c:100:16
> #13 0x58e02a9a in sdhci_send_command hw/sd/sdhci.c:337:12
> #14 0x58dffa46 in sdhci_write hw/sd/sdhci.c:1187:9
> #15 0x598b9d76 in memory_region_write_accessor softmmu/memory.c:489:5
>
> Similarly to commit 8573378e62d ("hw/sd: fix out-of-bounds check
> for multi block reads"), check the address range before sending
> the status of the write protection bits.
>
> Include the qtest reproducer provided by Alexander Bulekov:
>
> $ make check-qtest-i386
> ...
> Running test qtest-i386/fuzz-sdcard-test
> qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum <
> sd->wpgrps_size' failed.
>
> Reported-by: OSS-Fuzz (Issue 29225)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/450
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alexander Bulekov
Thanks
> ---
> hw/sd/sd.c | 5 +++
> tests/qtest/fuzz-sdcard-test.c | 66 ++
> MAINTAINERS| 3 +-
> tests/qtest/meson.build| 1 +
> 4 files changed, 74 insertions(+), 1 deletion(-)
> create mode 100644 tests/qtest/fuzz-sdcard-test.c
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 9c8dd11bad1..c753ae24ba9 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1379,6 +1379,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
>
> switch (sd->state) {
> case sd_transfer_state:
> +if (!address_in_range(sd, "SEND_WRITE_PROT",
> + req.arg, sd->blk_len)) {
> +return sd_r1;
> +}
> +
> sd->state = sd_sendingdata_state;
> *(uint32_t *) sd->data = sd_wpbits(sd, req.arg);
> sd->data_start = addr;
> diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
> new file mode 100644
> index 000..96602eac7e5
> --- /dev/null
> +++ b/tests/qtest/fuzz-sdcard-test.c
> @@ -0,0 +1,66 @@
> +/*
> + * QTest fuzzer-generated testcase for sdcard device
> + *
> + * Copyright (c) 2021 Philippe Mathieu-Daudé
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqos/libqtest.h"
> +
> +/*
> + * https://gitlab.com/qemu-project/qemu/-/issues/450
> + * Used to trigger:
> + * Assertion `wpnum < sd->wpgrps_size' failed.
> + */
> +static void oss_fuzz_29225(void)
> +{
> +QTestState *s;
> +
> +s = qtest_init(" -display none -m 512m -nodefaults -nographic"
> + " -device sdhci-pci,sd-spec-version=3"
> + " -device sd-card,drive=d0"
> + " -drive
> if=none,index=0,file=null-co://,format=raw,id=d0");
> +
> +qtest_outl(s, 0xcf8, 0x80001010);
> +qtest_outl(s, 0xcfc, 0xd0690);
> +qtest_outl(s, 0xcf8, 0x80001003);
> +qtest_outl(s, 0xcf8, 0x80001013);
> +qtest_outl(s, 0xcfc, 0x);
> +qtest_outl(s, 0xcf8, 0x80001003);
> +qtest_outl(s, 0xcfc, 0x3effe00);
> +
> +qtest_bufwrite(s, 0xff0d062c, "\xff", 0x1);
> +qtest_bufwrite(s, 0xff0d060f, "\xb7", 0x1);
> +qtest_bufwrite(s, 0xff0d060a, "\xc9", 0x1);
> +qtest_bufwrite(s, 0xff0d060f, "\x29", 0x1);
> +qtest_bufwrite(s, 0xff0d060f, "\xc2", 0x1);
> +qtest_bufwrite(s, 0xff0d0628, "\xf7", 0x1);
> +qtest_bufwrite(s, 0x0, "\xe3", 0x1);
> +qtest_bufwrite(s, 0x7, "\x13", 0x1);
> +qtest_bufwrite(s, 0x8, "\xe3", 0x1);
> +qtest_bufwrite(s, 0xf, "\xe3", 0x1);
> +qtest_bufwrite(s, 0xff0d060f, "\x03", 0x1);
> +qtest_bufwrite(s, 0xff0d0605, "\x01", 0x1);
> +qtest_bufwrite(s, 0xff0d060b, "\xff", 0x1);
> +qtest_bufwrite(s, 0xff0d060c, "\xff", 0x1);
> +qtest_bufwrite(s, 0xff0d060e, "\xff", 0x1);
> +qtest_bufwrite(s, 0xff0d060f, "\x06", 0x1);
> +qtest_bufwrite(s
Re: [PATCH 1/3] hw/sd: When card is in wrong state, log which state it is
On 210702 1758, Philippe Mathieu-Daudé wrote: > We report the card is in an inconsistent state, but don't precise > in which state it is. Add this information, as it is useful when > debugging problems. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Bin Meng > Message-Id: <20210624142209.1193073-2-f4...@amsat.org> Reviewed-by: Alexander Bulekov > --- > hw/sd/sd.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 282d39a7042..d8fdf84f4db 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -1504,7 +1504,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, > SDRequest req) > return sd_illegal; > } > > -qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd); > +qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n", > + req.cmd, sd_state_name(sd->state)); > return sd_illegal; > } > > -- > 2.31.1 >
Re: [RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again
On 210624 1012, Philippe Mathieu-Daudé wrote: > On 6/24/21 4:50 AM, Alexander Bulekov wrote: > > On 210623 2000, Philippe Mathieu-Daudé wrote: > >> Hi Ubi-Wan Kenubi and Tom, > >> > >> In commit a9bcedd (SD card size has to be power of 2) we decided > >> to restrict SD card size to avoid security problems (CVE-2020-13253) > >> but this became not practical to some users. > >> > >> This RFC series tries to remove the limitation, keeping our > >> functional tests working. It is unfinished work because I had to > >> attend other topics, but sending it early as RFC to get feedback. > >> I'll keep working when I get more time, except if one if you can > >> help me. > >> > >> Alexander, could you generate a qtest reproducer with the fuzzer > >> corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054 > > > > I think that bug was already fixed - the reproducer no logner causes a > > timeout on 6.0. Did I misunderstand something? > > That bug was fixed but now I'm changing the code and would like to feel > sure I'm not re-introducing the problem, so having the reproducer in the > tree would help. Ok - I'll try to come up with one. Minimizing reproducers for timeouts is trickier than crashes :-) > > > I applied this series and ran the OSS-Fuzz corpus for the sdhci-v3 > > config. The only problem it found is this assert() (that exists without the > > patch anyways): > > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29225 > > Sigh. > > > Let me know if this is something you think I should report on gitlab.. > > Yes please :( Ok here it is: https://gitlab.com/qemu-project/qemu/-/issues/450 The fuzzer I left running for 24h also found another bug (looks like DMA reentrancy), which I thought was introduced by this patchset, since OSS-Fuzz had not reported it in months of fuzzing. However, as a sanity-check I tried it against qemu.git and it crashed - strange... Anyways here it is: https://gitlab.com/qemu-project/qemu/-/issues/451 -Alex > > > I'll leave the fuzzer running for another 24h or so, but otherwise I'm > > happy to leave a Tested-by, once there is a V1 series > > -Alex > > Thanks! > > Phil.
Re: [RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again
On 210623 2000, Philippe Mathieu-Daudé wrote: > Hi Ubi-Wan Kenubi and Tom, > > In commit a9bcedd (SD card size has to be power of 2) we decided > to restrict SD card size to avoid security problems (CVE-2020-13253) > but this became not practical to some users. > > This RFC series tries to remove the limitation, keeping our > functional tests working. It is unfinished work because I had to > attend other topics, but sending it early as RFC to get feedback. > I'll keep working when I get more time, except if one if you can > help me. > > Alexander, could you generate a qtest reproducer with the fuzzer > corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054 I think that bug was already fixed - the reproducer no logner causes a timeout on 6.0. Did I misunderstand something? I applied this series and ran the OSS-Fuzz corpus for the sdhci-v3 config. The only problem it found is this assert() (that exists without the patch anyways): https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29225 Let me know if this is something you think I should report on gitlab.. I'll leave the fuzzer running for another 24h or so, but otherwise I'm happy to leave a Tested-by, once there is a V1 series -Alex > Thanks, > > Phil. > > Philippe Mathieu-Daudé (9): > hw/sd: When card is in wrong state, log which state it is > hw/sd: Extract address_in_range() helper, log invalid accesses > tests/acceptance: Tag NetBSD tests as 'os:netbsd' > tests/acceptance: Extract image_expand() helper > tests/acceptance: Use image_expand() in > test_arm_orangepi_uboot_netbsd9 > tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08 > tests/acceptance: Do not expand SD card image in test_arm_orangepi_sd > tests/acceptance: Remove now unused pow2ceil() > hw/sd: Allow card size not power of 2 again > > hw/sd/sd.c | 60 +- > tests/acceptance/boot_linux_console.py | 39 - > tests/acceptance/ppc_prep_40p.py | 2 + > 3 files changed, 52 insertions(+), 49 deletions(-) > > -- > 2.31.1 >
[PATCH v2] floppy: remove dead code related to formatting
fdctrl_format_sector was added in
baca51faff ("updated floppy driver: formatting code, disk geometry auto detect
(Jocelyn Mayer)")
The single callsite is guarded by a check:
fdctrl->data_state & FD_STATE_FORMAT
However, the only place where the FD_STATE_FORMAT flag is set (in
fdctrl_handle_format_track) is closely followed by the same flag being
unset, with no possibility to call fdctrl_format_sector in between.
This removes fdctrl_format_sector, the unncessary setting/unsetting
of the FD_STATE_FORMAT flag, and the fdctrl_handle_format_track function
(which is just a stub).
Suggested-by: Hervé Poussineau
Signed-off-by: Alexander Bulekov
---
I ran through tests/qtest/fdc-test, and ran fdformat on a dummy disk -
nothing exploded, but since I don't use floppies very often, more eyes
definitely won't hurt. In particular, I'm not sure about the
fdctrl_handle_format_track delete - that function has side-effects on
both FDrive and FDCtrl, and it is certainly reachable. If deleting the
whole thing seems wrong, I'll roll-back that change, and we can just
remove the unreachable code..
hw/block/fdc.c | 97 --
1 file changed, 97 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a825c2acba..d851d23cc0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -657,7 +657,6 @@ enum {
enum {
FD_STATE_MULTI = 0x01,/* multi track flag */
-FD_STATE_FORMAT = 0x02,/* format flag */
};
enum {
@@ -826,7 +825,6 @@ enum {
};
#define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
-#define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
struct FDCtrl {
MemoryRegion iomem;
@@ -1942,67 +1940,6 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
return retval;
}
-static void fdctrl_format_sector(FDCtrl *fdctrl)
-{
-FDrive *cur_drv;
-uint8_t kh, kt, ks;
-
-SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
-cur_drv = get_cur_drv(fdctrl);
-kt = fdctrl->fifo[6];
-kh = fdctrl->fifo[7];
-ks = fdctrl->fifo[8];
-FLOPPY_DPRINTF("format sector at %d %d %02x %02x (%d)\n",
- GET_CUR_DRV(fdctrl), kh, kt, ks,
- fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
- NUM_SIDES(cur_drv)));
-switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
-case 2:
-/* sect too big */
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
-fdctrl->fifo[3] = kt;
-fdctrl->fifo[4] = kh;
-fdctrl->fifo[5] = ks;
-return;
-case 3:
-/* track too big */
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_EC, 0x00);
-fdctrl->fifo[3] = kt;
-fdctrl->fifo[4] = kh;
-fdctrl->fifo[5] = ks;
-return;
-case 4:
-/* No seek enabled */
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
-fdctrl->fifo[3] = kt;
-fdctrl->fifo[4] = kh;
-fdctrl->fifo[5] = ks;
-return;
-case 1:
-fdctrl->status0 |= FD_SR0_SEEK;
-break;
-default:
-break;
-}
-memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
-if (cur_drv->blk == NULL ||
-blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
- BDRV_SECTOR_SIZE, 0) < 0) {
-FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
-} else {
-if (cur_drv->sect == cur_drv->last_sect) {
-fdctrl->data_state &= ~FD_STATE_FORMAT;
-/* Last sector done */
-fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
-} else {
-/* More to do */
-fdctrl->data_pos = 0;
-fdctrl->data_len = 4;
-}
-}
-}
-
static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
{
fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
@@ -2110,34 +2047,6 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl, int
direction)
(NANOSECONDS_PER_SECOND / 50));
}
-static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction)
-{
-FDrive *cur_drv;
-
-SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
-cur_drv = get_cur_drv(fdctrl);
-fdctrl->data_state |= FD_STATE_FORMAT;
-if (fdctrl->fifo[0] & 0x80)
-fdctrl->data_state |= FD_STATE_MULTI;
-else
-fdctrl->data_state &= ~FD_STATE_MULTI;
-cur_drv->bps =
-fdctrl->fifo[2] > 7 ? 16384 : 128 << fdctrl->fifo[2];
-#if 0
-cur_drv->last_sect =
-cur_drv->flags & FDISK_DBL_SIDES ? fdctrl->fifo[3] :
-fdctrl->fifo[3] / 2;
-#else
-cur_drv->last_sect = fdctrl->fifo[3];
-#endif
-
Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
On 210319 1026, Paolo Bonzini wrote: > On 19/03/21 06:53, Markus Armbruster wrote: > > I guess this is a reproducer. Please also describe actual and expected > > result. Same for PATCH 2. > > Isn't it in the patch itself? > > Alexander, I think these reproducers are self-contained enough (no writes to > PCI configuration space to set up the device) that we can place them in > fdc-test.c. > Will do -Alex > Paolo >
Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
On 210319 1054, Markus Armbruster wrote: > Paolo Bonzini writes: > > > On 19/03/21 06:53, Markus Armbruster wrote: > >> I guess this is a reproducer. Please also describe actual and expected > >> result. Same for PATCH 2. > > > > Isn't it in the patch itself? > > A commit message should tell me what the patch is trying to accomplish. > > This commit message's title tells me it's a test for a CVE. Okay. The > body additionally gives me the reproducer. To be useful, a reproducer > needs to come with actual and expected result. Yes, I can find those in > the patch. But I could find the reproducer there, too. If you're nice > enough to save me the trouble of digging through the patch for the > reproducer (thanks), please consider saving me the trouble digging for > the information I need to make use of it (thanks again). That's all :) > > [...] > Ok sounds good. I posted this in-reply-to patch [1] from August 2020, which had a stacktrace, and I hoped that would provide enough context. However, that depends on the email-viewer and I see how that context would be lost if/once these reproducer patches are applied. [1] https://lore.kernel.org/qemu-devel/20200827113806.1850687-1-ppan...@redhat.com/ (https://lists.nongnu.org/archive doesn't display this discussion as a child of that message)
[PATCH 2/2] floppy: add a regression test for CVE-2021-20196
dd if=/dev/zero of=/tmp/fda.img bs=1024 count=1440
cat << EOF | ./qemu-system-i386 -nographic -m 512M -nodefaults \
-accel qtest -fda /tmp/fda.img -qtest stdio
outw 0x3f4 0x0500
outb 0x3f5 0x00
outb 0x3f5 0x00
outw 0x3f4 0x00
outb 0x3f5 0x00
outw 0x3f1 0x0400
outw 0x3f4 0x0
outw 0x3f4 0x00
outb 0x3f5 0x0
outb 0x3f5 0x01
outw 0x3f1 0x0500
outb 0x3f5 0x00
EOF
Signed-off-by: Alexander Bulekov
---
Since this looks very similar to CVE-2021-20196 (I believe Li pointed
out that issue in this thread), I'm also posting the reproducer for that
here.
tests/qtest/fuzz-test.c | 57 +
1 file changed, 57 insertions(+)
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 62ececc66f..8e4ccdaca8 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -76,6 +76,61 @@ static void test_fdc_cve_2020_25741(void)
qtest_quit(s);
}
+
+/*
+ * ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x0344
+ * The signal is caused by a WRITE memory access.
+ * #0 0x56494543 in blk_inc_in_flight /block/block-backend.c:1356:5
+ * #1 0x56494543 in blk_prw /block/block-backend.c:1328:5
+ * #2 0x56494d03 in blk_pread /block/block-backend.c:1491:15
+ * #3 0x55ec8986 in fdctrl_read_data /hw/block/fdc.c:1910:17
+ * #4 0x55ec8986 in fdctrl_read /hw/block/fdc.c:936:18
+ * #5 0x563f26b7 in portio_read /softmmu/ioport.c:185:25
+ * #6 0x5636908a in memory_region_read_accessor /softmmu/memory.c:442:11
+ * #7 0x5635ec25 in access_with_adjusted_size /softmmu/memory.c:552:18
+ * #8 0x5635ec25 in memory_region_dispatch_read1 /softmmu/memory.c:1420:16
+ * #9 0x5635ec25 in memory_region_dispatch_read /softmmu/memory.c:1448:9
+ * #10 0x56248aa7 in flatview_read_continue /softmmu/physmem.c:2810:23
+ * #11 0x563f18f0 in address_space_read /include/exec/memory.h:2494:26
+ * #12 0x563f18f0 in cpu_inw /softmmu/ioport.c:99:5
+ * #13 0x5637619c in qtest_process_command /softmmu/qtest.c:502:21
+ * #14 0x5637535d in qtest_process_inbuf /softmmu/qtest.c:797:9
+ * #15 0x56405f9c in tcp_chr_read /chardev/char-socket.c:557:13
+ * #16 0x76f8ac3e in g_main_context_dispatch
+ * #17 0x567479f1 in glib_pollfds_poll /util/main-loop.c:231:9
+ * #18 0x567479f1 in os_host_main_loop_wait /util/main-loop.c:254:5
+ * #19 0x567479f1 in main_loop_wait /util/main-loop.c:530:11
+ * #20 0x562d9ee4 in qemu_main_loop /softmmu/runstate.c:725:9
+ * #21 0x55d5b615 in main /softmmu/main.c:50:5
+*/
+static void test_fdc_cve_2021_20196(void)
+{
+QTestState *s;
+int fd;
+char tmpdisk[] = "/tmp/fda.XX";
+fd = mkstemp(tmpdisk);
+assert(fd >= 0);
+ftruncate(fd, 1440 * 1024);
+close(fd);
+
+s = qtest_initf("-nographic -m 512M -nodefaults "
+"-drive file=%s,format=raw,if=floppy", tmpdisk);
+qtest_outw(s, 0x3f2, 0x04);
+qtest_outw(s, 0x3f4, 0x0200);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outw(s, 0x3f2, 0x01);
+qtest_inw(s, 0x3f4);
+qtest_quit(s);
+unlink(tmpdisk);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -87,6 +142,8 @@ int main(int argc, char **argv)
test_lp1878642_pci_bus_get_irq_level_assert);
qtest_add_func("fuzz/test_fdc_cve_2020_25741",
test_fdc_cve_2020_25741);
+qtest_add_func("fuzz/test_fdc_cve_2021_20196",
+ test_fdc_cve_2021_20196);
}
return g_test_run();
--
2.27.0
[PATCH 1/2] floppy: add a regression test for CVE-2020-25741
dd if=/dev/zero of=/tmp/fda.img bs=1024 count=1440
cat << EOF | ./qemu-system-i386 -nographic -m 512M -nodefaults \
-accel qtest -fda /tmp/fda.img -qtest stdio
outw 0x3f4 0x0500
outb 0x3f5 0x00
outb 0x3f5 0x00
outw 0x3f4 0x00
outb 0x3f5 0x00
outw 0x3f1 0x0400
outw 0x3f4 0x0
outw 0x3f4 0x00
outb 0x3f5 0x0
outb 0x3f5 0x01
outw 0x3f1 0x0500
outb 0x3f5 0x00
EOF
Signed-off-by: Alexander Bulekov
---
Might be useful for reproducing/regression testing
tests/qtest/fuzz-test.c | 54 +
1 file changed, 54 insertions(+)
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 00149abec7..62ececc66f 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -24,6 +24,58 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_quit(s);
}
+/*
+ * ERROR: AddressSanitizer: SEGV on unknown address 0x0344
+ * The signal is caused by a WRITE memory access.
+ * #0 0x59d7f519 in blk_inc_in_flight /block/block-backend.c:1356:5
+ * #1 0x59d7f519 in blk_prw /block/block-backend.c:1328:5
+ * #2 0x59d81673 in blk_pwrite /block/block-backend.c:1501:15
+ * #3 0x58fc763f in fdctrl_write_data /hw/block/fdc.c:2414:17
+ * #4 0x58fc763f in fdctrl_write /hw/block/fdc.c:967:9
+ * #5 0x594a622c in memory_region_write_accessor /softmmu/memory.c:491:5
+ * #6 0x594a5c93 in access_with_adjusted_size /softmmu/memory.c:552:18
+ * #7 0x594a54f0 in memory_region_dispatch_write /softmmu/memory.c
+ * #8 0x5964a686 in flatview_write_continue /softmmu/physmem.c:2776:23
+ * #9 0x5963fde8 in flatview_write /softmmu/physmem.c:2816:14
+ * #10 0x5963fde8 in address_space_write /softmmu/physmem.c:2908:18
+ * #11 0x5968f8a1 in cpu_outb /softmmu/ioport.c:60:5
+ * #12 0x597d5619 in qtest_process_command /softmmu/qtest.c:479:13
+ * #13 0x597d34bf in qtest_process_inbuf /softmmu/qtest.c:797:9
+ * #14 0x59b4539d in fd_chr_read /chardev/char-fd.c:68:9
+ * #15 0x77b7dc3e in g_main_context_dispatch
+ * #16 0x5a4facdc in glib_pollfds_poll /util/main-loop.c:231:9
+ * #17 0x5a4facdc in os_host_main_loop_wait /util/main-loop.c:254:5
+ * #18 0x5a4facdc in main_loop_wait /util/main-loop.c:530:11
+ * #19 0x59a6dec9 in qemu_main_loop /softmmu/runstate.c:725:9
+ * #20 0x581a3085 in main /softmmu/main.c:50:5
+ */
+static void test_fdc_cve_2020_25741(void)
+{
+QTestState *s;
+int fd;
+char tmpdisk[] = "/tmp/fda.XX";
+fd = mkstemp(tmpdisk);
+assert(fd >= 0);
+ftruncate(fd, 1440 * 1024);
+close(fd);
+
+s = qtest_initf("-nographic -m 512M -nodefaults "
+"-drive file=%s,format=raw,if=floppy", tmpdisk);
+qtest_outw(s, 0x3f4, 0x0500);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_outw(s, 0x3f1, 0x0400);
+qtest_outw(s, 0x3f4, 0x0);
+qtest_outw(s, 0x3f4, 0x00);
+qtest_outb(s, 0x3f5, 0x0);
+qtest_outb(s, 0x3f5, 0x01);
+qtest_outw(s, 0x3f1, 0x0500);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -33,6 +85,8 @@ int main(int argc, char **argv)
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
test_lp1878642_pci_bus_get_irq_level_assert);
+qtest_add_func("fuzz/test_fdc_cve_2020_25741",
+ test_fdc_cve_2020_25741);
}
return g_test_run();
--
2.27.0
Re: [PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()
On 210216 1146, Bin Meng wrote:
> s->prnsts is updated in both branches of the if () else () statement.
> Move the common bits outside so that it is cleaner.
>
> Signed-off-by: Bin Meng
Reviewed-by: Alexander Bulekov
> ---
>
> (no changes since v1)
>
> hw/sd/sdhci.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 0b0ca6f..7a2003b 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -598,9 +598,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState
> *s)
> page_aligned = true;
> }
>
> +s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
> if (s->trnmod & SDHC_TRNS_READ) {
> -s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
> -SDHC_DAT_LINE_ACTIVE;
> +s->prnsts |= SDHC_DOING_READ;
> while (s->blkcnt) {
> if (s->data_count == 0) {
> sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
> @@ -627,8 +627,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState
> *s)
> }
> }
> } else {
> -s->prnsts |= SDHC_DOING_WRITE | SDHC_DATA_INHIBIT |
> -SDHC_DAT_LINE_ACTIVE;
> +s->prnsts |= SDHC_DOING_WRITE;
> while (s->blkcnt) {
> begin = s->data_count;
> if (((boundary_count + begin) < block_size) && page_aligned) {
> --
> 2.7.4
>
Re: [PATCH v2 0/6] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409
Hi Bin, For this series, Tested-by: Alexander Bulekov Thank you -Alex On 210216 1146, Bin Meng wrote: > This series includes several fixes to CVE-2020-17380, CVE-2020-25085 > and CVE-2021-3409 that are heap-based buffer overflow issues existing > in the sdhci model. > > These CVEs are pretty much similar, and were filed using different > reproducers. With this series, current known reproducers I have > cannot be reproduced any more. > > The implementation of this model still has some issues, e.g.: some codes > do not seem to strictly match the spec, but since this series only aimes > to address CVEs, such issue should be fixed in a separate series in the > future, if I have time :) > > Changes in v2: > - new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is > writable > - new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a > different block size is programmed > > Bin Meng (6): > hw/sd: sdhci: Don't transfer any data when command time out > hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in > progress > hw/sd: sdhci: Correctly set the controller status for ADMA > hw/sd: sdhci: Simplify updating s->prnsts in > sdhci_sdma_transfer_multi_blocks() > hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is > writable > hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a > different block size is programmed > > hw/sd/sdhci.c | 60 > ++- > 1 file changed, 39 insertions(+), 21 deletions(-) > > -- > 2.7.4 >
Re: [PATCH 0/4] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409
On 210216 0855, Bin Meng wrote: > Hi Alex, > > On Tue, Feb 16, 2021 at 12:48 AM Alexander Bulekov wrote: > > > > Hi Bin, > > Thank you for this. I ran through the OSS-Fuzz tests again, and it found > > one thing: > > Thanks for testing. Are there instructions to run OSS-Fuzz tests myself? Yes we have some documentation in docs/devel/fuzzing.rst, but it doesn't talk about using the OSS-Fuzz corpus. The OSS-Fuzz corpus is private, by default, but I uploaded a copy of the current sdhci corpus here: https://drive.google.com/file/d/1PcwFbY9YXPdaJ3aapIV2BI-bN5mbBgif/view?usp=sharing To build the fuzzer, you need clang: build the fuzzers $ CC=clang CXX=clang++ ../configure --enable-fuzzing --enable-sanitizers \ --disable-werror $ ninja -j`nproc` qemu-fuzz-i386 untar the corpus somewhere (~300 MB uncompressed) $ tar -xvf sdhci-corpus.tar.gz run through all the inputs once $ ./qemu-fuzz-i386 --fuzz-target=generic-fuzz-sdhci-v3 \ ~/path/to/corpus/qemu_qemu-fuzz-i386-target-generic-fuzz-sdhci-v3/* &> out That will take some minutes, but you can look at the out file and search for "ERROR" to find crashing inputs. -Alex > > > Maybe this is already much better than the current state of the code, so > > this one can be fixed in a later patch? > > Depend on when Philippe can pick up this sereis, but I can also try to > have a quick look :) > > > > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest \ > > -m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \ > > -device sd-card,drive=mydrive \ > > -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \ > > -nographic -qtest stdio > > outl 0xcf8 0x80001010 > > outl 0xcfc 0xe000 > > outl 0xcf8 0x80001001 > > outl 0xcfc 0x0600 > > write 0xe02c 0x1 0x05 > > write 0xe005 0x1 0x02 > > write 0xe007 0x1 0x01 > > write 0xe028 0x1 0x10 > > write 0x0 0x1 0x23 > > write 0x2 0x1 0x08 > > write 0xe00c 0x1 0x01 > > write 0xe00e 0x1 0x20 > > write 0xe00f 0x1 0x00 > > write 0xe00c 0x1 0x32 > > write 0xe004 0x2 0x0200 > > write 0xe028 0x1 0x00 > > write 0xe003 0x1 0x40 > > EOF > > > > > > ==1730971==ERROR: AddressSanitizer: heap-buffer-overflow on address > > 0x61531880 at pc 0x55d070f2c6d9 bp 0x7ffdcb63f130 sp 0x7ffdcb63f128 > > READ of size 4 at 0x61531880 thread T0 > > #0 0x55d070f2c6d8 in ldl_he_p bswap.h:347:5 > > #1 0x55d070f2c6d8 in ldn_he_p bswap.h:546:1 > > #2 0x55d070f2c6d8 in flatview_write_continue > > build/../softmmu/physmem.c:2775:19 > > #3 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14 > > #4 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18 > > #5 0x55d07040de4a in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 > > #6 0x55d07040de4a in dma_memory_rw include/sysemu/dma.h:127:12 > > #7 0x55d07040de4a in dma_memory_write include/sysemu/dma.h:163:12 > > #8 0x55d07040de4a in sdhci_sdma_transfer_multi_blocks > > build/../hw/sd/sdhci.c:619:13 > > #9 0x55d07041d15b in sdhci_write build/../hw/sd/sdhci.c:1134:21 > > #10 0x55d07123b1ac in memory_region_write_accessor > > build/../softmmu/memory.c:491:5 > > #11 0x55d07123acab in access_with_adjusted_size > > build/../softmmu/memory.c:552:18 > > #12 0x55d07123a4b0 in memory_region_dispatch_write build/../softmmu/memory.c > > #13 0x55d070f2c29b in flatview_write_continue > > build/../softmmu/physmem.c:2776:23 > > #14 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14 > > #15 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18 > > Regards, > Bin
Re: [PATCH 0/4] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409
Hi Bin, Thank you for this. I ran through the OSS-Fuzz tests again, and it found one thing: Maybe this is already much better than the current state of the code, so this one can be fixed in a later patch? cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest \ -m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \ -device sd-card,drive=mydrive \ -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \ -nographic -qtest stdio outl 0xcf8 0x80001010 outl 0xcfc 0xe000 outl 0xcf8 0x80001001 outl 0xcfc 0x0600 write 0xe02c 0x1 0x05 write 0xe005 0x1 0x02 write 0xe007 0x1 0x01 write 0xe028 0x1 0x10 write 0x0 0x1 0x23 write 0x2 0x1 0x08 write 0xe00c 0x1 0x01 write 0xe00e 0x1 0x20 write 0xe00f 0x1 0x00 write 0xe00c 0x1 0x32 write 0xe004 0x2 0x0200 write 0xe028 0x1 0x00 write 0xe003 0x1 0x40 EOF ==1730971==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61531880 at pc 0x55d070f2c6d9 bp 0x7ffdcb63f130 sp 0x7ffdcb63f128 READ of size 4 at 0x61531880 thread T0 #0 0x55d070f2c6d8 in ldl_he_p bswap.h:347:5 #1 0x55d070f2c6d8 in ldn_he_p bswap.h:546:1 #2 0x55d070f2c6d8 in flatview_write_continue build/../softmmu/physmem.c:2775:19 #3 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14 #4 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18 #5 0x55d07040de4a in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 #6 0x55d07040de4a in dma_memory_rw include/sysemu/dma.h:127:12 #7 0x55d07040de4a in dma_memory_write include/sysemu/dma.h:163:12 #8 0x55d07040de4a in sdhci_sdma_transfer_multi_blocks build/../hw/sd/sdhci.c:619:13 #9 0x55d07041d15b in sdhci_write build/../hw/sd/sdhci.c:1134:21 #10 0x55d07123b1ac in memory_region_write_accessor build/../softmmu/memory.c:491:5 #11 0x55d07123acab in access_with_adjusted_size build/../softmmu/memory.c:552:18 #12 0x55d07123a4b0 in memory_region_dispatch_write build/../softmmu/memory.c #13 0x55d070f2c29b in flatview_write_continue build/../softmmu/physmem.c:2776:23 #14 0x55d070f219eb in flatview_write build/../softmmu/physmem.c:2816:14 #15 0x55d070f219eb in address_space_write build/../softmmu/physmem.c:2908:18 -Alex On 210215 2311, Bin Meng wrote: > From: Bin Meng > > This series includes several fixes to CVE-2020-17380, CVE-2020-25085 > and CVE-2021-3409 that are heap-based buffer overflow issues existing > in the sdhci model. > > These CVEs are pretty much similar, and were filed using different > reproducers. With this series, current known reproducers I have > cannot be reproduced any more. > > The implementation of this model may still have some issues, i.e.: > some codes do not strictly match the spec, but since this series > only aimes to address CVEs, such issue should be fixed in a separate > series in the future, if I have time :) > > > Bin Meng (4): > hw/sd: sdhci: Don't transfer any data when command time out > hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in > progress > hw/sd: sdhci: Correctly set the controller status for ADMA > hw/sd: sdhci: Simplify updating s->prnsts in > sdhci_sdma_transfer_multi_blocks() > > hw/sd/sdhci.c | 34 -- > 1 file changed, 20 insertions(+), 14 deletions(-) > > -- > 2.7.4 >
Re: [PATCH] hw/sd: sdhci: Do not transfer any data when command fails
On 210211 1154, Alexander Bulekov wrote: ... > I applied this along with <20210208193450.2689517-1-f4...@amsat.org> > "hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress" > > I ran through the entire OSS-Fuzz corpus, and could not reproduce the > crash. > > Tested-by: Alexander Bulekov > Hi Bin, Phil explained to me that this patch should fix the problem independent of "hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress" With only this patch, there are still crashes. Here are three reproducers: Some of these are quite long, so here are pastebins for convenience: Repro 1: https://paste.debian.net/plain/1185137 Repro 2: https://paste.debian.net/plain/1185141 Repro 3: https://paste.debian.net/plain/1185136 Just wget and pipe them into ./qemu-system-i386 -display none -machine accel=qtest -nographic \ -m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \ -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \ -device sd-card,drive=mydrive -qtest stdio Repro 1 cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest -nographic \ -m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \ -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \ -device sd-card,drive=mydrive -qtest stdio outl 0xcf8 0x80001010 outl 0xcfc 0xfbefff00 outl 0xcf8 0x80001001 outl 0xcfc 0x0600 write 0xfbefff2c 0x1 0x05 write 0xfbefff0f 0x1 0x37 write 0xfbefff0a 0x1 0x01 write 0xfbefff0f 0x1 0x29 write 0xfbefff0f 0x1 0x02 write 0xfbefff0f 0x1 0x03 write 0xfbefff04 0x1 0x01 write 0xfbefff05 0x1 0x01 write 0xfbefff07 0x1 0x02 write 0xfbefff0c 0x1 0x33 write 0xfbefff0e 0x1 0x20 write 0xfbefff0f 0x1 0x00 write 0xfbefff2a 0x1 0x01 write 0xfbefff0c 0x1 0x00 write 0xfbefff03 0x1 0x00 write 0xfbefff05 0x1 0x00 write 0xfbefff2a 0x1 0x02 write 0xfbefff0c 0x1 0x32 write 0xfbefff01 0x1 0x01 write 0xfbefff02 0x1 0x01 write 0xfbefff03 0x1 0x01 EOF Stack Trace 1 ==929953==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61531880 at pc 0x564cf01ceae7 bp 0x7ffe17361e10 sp 0x7ffe173615d8 READ of size 520027904 at 0x61531880 thread T0 #0 0x564cf01ceae6 in __asan_memcpy (/home/alxndr/Development/qemu/build/qemu-system-i386+0x2a8cae6) #1 0x564cf19111a5 in flatview_write_continue /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2781:13 #2 0x564cf1906beb in flatview_write /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2816:14 #3 0x564cf1906beb in address_space_write /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2908:18 #4 0x564cf096348c in dma_memory_rw_relaxed /home/alxndr/Development/qemu/include/sysemu/dma.h:88:12 #5 0x564cf096348c in dma_memory_rw /home/alxndr/Development/qemu/include/sysemu/dma.h:127:12 #6 0x564cf096348c in dma_memory_write /home/alxndr/Development/qemu/include/sysemu/dma.h:163:12 #7 0x564cf096348c in sdhci_sdma_transfer_multi_blocks /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:619:13 #8 0x564cf097237d in sdhci_write /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:1131:17 #9 0x564cf154333c in memory_region_write_accessor /home/alxndr/Development/qemu/build/../softmmu/memory.c:491:5 Repro 2 cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest -nographic \ -m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \ -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \ -device sd-card,drive=mydrive -qtest stdio outl 0xcf8 0x80001013 outl 0xcfc 0x91 outl 0xcf8 0x80001001 outl 0xcfc 0x0600 write 0x912c 0x1 0x05 write 0x910f 0x1 0x37 write 0x910a 0x1 0x01 write 0x910f 0x1 0x29 write 0x910f 0x1 0x02 write 0x910f 0x1 0x03 write 0x0 0x1 0x01 write 0x8 0x1 0x01 write 0x10 0x1 0x01 write 0x18 0x1 0x01 write 0x20 0x1 0x01 write 0x28 0x1 0x01 write 0x30 0x1 0x01 write 0x38 0x1 0x01 write 0x40 0x1 0x01 write 0x48 0x1 0x01 write 0x50 0x1 0x01 write 0x58 0x1 0x01 write 0x60 0x1 0x01 write 0x68 0x1 0x01 write 0x70 0x1 0x01 write 0x9105 0x1 0x02 write 0x9107 0x1 0x20 write 0x78 0x1 0x01 write 0x80 0x1 0x01 write 0x88 0x1 0x01 write 0x90 0x1 0x01 write 0x98 0x1 0x01 write 0xa0 0x1 0x01 write 0xa8 0x1 0x01 write 0xb0 0x1 0x01 write 0xb8 0x1 0x01 write 0xc0 0x1 0x01 write 0x910e 0x1 0x21 write 0x9128 0x1 0x10 write 0x910c 0x1 0x01 write 0x910f 0x1 0x06 write 0xc8 0x1 0x01 write 0xd0 0x1 0x01 write 0xd8 0x1 0x01 write 0xe0 0x1 0x01 write 0xe8 0x1 0x01 write 0xf0 0x1 0x01 write 0xf8 0x1 0x01 write 0x100 0x1 0x01 write 0x108 0x1 0x01 write 0x110 0x1 0x01 write 0x118 0x1 0x01 write 0x120 0x1 0x01 write 0x128 0x1 0x01 write 0x130 0x1 0x01 write 0x138 0x1 0x01 write 0x140 0x1 0x01 write 0x148 0x1 0x01 write 0x150 0x1 0x01 write 0x158 0x1 0x01 write 0x160 0x1 0x01 write 0x168 0x1 0x01 write 0x170 0x1 0x01 write 0x178 0x1 0x01 write 0x180 0x1 0x01 write 0x188 0x1 0x01 write 0x190 0x1 0x01 write 0x198 0x1 0x01 write
Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress
On 210211 2045, Philippe Mathieu-Daudé wrote: > Hi Alexander, > > On 2/11/21 6:04 PM, Alexander Bulekov wrote: > > On 210208 2034, Philippe Mathieu-Daudé wrote: > >> Per the "SD Host Controller Simplified Specification Version 2.00" > >> spec. 'Table 2-4 : Block Size Register': > >> > >> Transfer Block Size [...] can be accessed only if no > >> transaction is executing (i.e., after a transaction has stopped). > >> Read operations during transfers may return an invalid value, > >> and write operations shall be ignored. > >> > >> Transactions will update 'data_count', so do not modify 'blksize' > >> and 'blkcnt' when 'data_count' is used. This fixes: > >> > >> $ cat << EOF | qemu-system-x86_64 -qtest stdio -monitor none \ > >>-nographic -serial none -M pc-q35-5.0 \ > >>-device sdhci-pci,sd-spec-version=3 \ > >>-device sd-card,drive=mydrive \ > >>-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive > >> outl 0xcf8 0x80001810 > >> outl 0xcfc 0xe1068000 > >> outl 0xcf8 0x80001814 > >> outl 0xcf8 0x80001804 > >> outw 0xcfc 0x7 > >> outl 0xcf8 0x8000fa20 > >> write 0xe106802c 0x1 0x0f > >> write 0xe1068004 0xc 0x2801d10101fbff28a384 > >> write 0xe106800c 0x1f > >> 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f > >> write 0xe1068003 0x28 > >> 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576 > >> write 0xe1068003 0x1 0xfe > >> EOF > >> = > >> ==2686219==ERROR: AddressSanitizer: heap-buffer-overflow on address > >> 0x6153bb00 at pc 0x55ab469f456c bp 0x7ffee71be330 sp 0x7ffee71bdae0 > >> WRITE of size 4 at 0x6153bb00 thread T0 > >> #0 0x55ab469f456b in __asan_memcpy (qemu-system-i386+0x1cea56b) > >> #1 0x55ab483dc396 in stl_he_p include/qemu/bswap.h:353:5 > >> #2 0x55ab483af5e4 in stn_he_p include/qemu/bswap.h:546:1 > >> #3 0x55ab483aeb4b in flatview_read_continue softmmu/physmem.c:2839:13 > >> #4 0x55ab483b0705 in flatview_read softmmu/physmem.c:2877:12 > >> #5 0x55ab483b028e in address_space_read_full > >> softmmu/physmem.c:2890:18 > >> #6 0x55ab483b1294 in address_space_rw softmmu/physmem.c:2918:16 > >> #7 0x55ab479374a2 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 > >> #8 0x55ab47936f50 in dma_memory_rw include/sysemu/dma.h:127:12 > >> #9 0x55ab4793665f in dma_memory_read include/sysemu/dma.h:145:12 > >> #10 0x55ab4792f176 in sdhci_sdma_transfer_multi_blocks > >> hw/sd/sdhci.c:639:13 > >> #11 0x55ab4793dc9d in sdhci_write hw/sd/sdhci.c:1129:17 > >> #12 0x55ab483f8db8 in memory_region_write_accessor > >> softmmu/memory.c:491:5 > >> #13 0x55ab483f868a in access_with_adjusted_size > >> softmmu/memory.c:552:18 > >> #14 0x55ab483f6da5 in memory_region_dispatch_write > >> softmmu/memory.c:1501:16 > >> #15 0x55ab483c3b11 in flatview_write_continue > >> softmmu/physmem.c:2774:23 > >> #16 0x55ab483b0eb6 in flatview_write softmmu/physmem.c:2814:14 > >> #17 0x55ab483b0a3e in address_space_write softmmu/physmem.c:2906:18 > >> #18 0x55ab48465c56 in qtest_process_command softmmu/qtest.c:654:9 > >> > >> 0x6153bb00 is located 0 bytes to the right of 512-byte region > >> [0x6153b900,0x6153bb00) > >> allocated by thread T0 here: > >> #0 0x55ab469f58a7 in calloc (qemu-system-i386+0x1ceb8a7) > >> #1 0x7f21d678f9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0) > >> #2 0x55ab479530ed in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5 > >> #3 0x55ab476f102a in pci_qdev_realize hw/pci/pci.c:2108:9 > >> #4 0x55ab48baaad2 in device_set_realized hw/core/qdev.c:761:13 > >> > >> SUMMARY: AddressSanitizer: heap-buffer-overflow > >> (qemu-system-i386+0x1cea56b) in __asan_memcpy > >> Shadow bytes around the buggy address: > >> 0x0c2a7710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > >> 0x0c2a7720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> 0x0c2a7730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> 0x0c2a7740: 00 00 00 00 00 00 00 00 00
Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress
On 210208 2034, Philippe Mathieu-Daudé wrote: > Per the "SD Host Controller Simplified Specification Version 2.00" > spec. 'Table 2-4 : Block Size Register': > > Transfer Block Size [...] can be accessed only if no > transaction is executing (i.e., after a transaction has stopped). > Read operations during transfers may return an invalid value, > and write operations shall be ignored. > > Transactions will update 'data_count', so do not modify 'blksize' > and 'blkcnt' when 'data_count' is used. This fixes: > > $ cat << EOF | qemu-system-x86_64 -qtest stdio -monitor none \ >-nographic -serial none -M pc-q35-5.0 \ >-device sdhci-pci,sd-spec-version=3 \ >-device sd-card,drive=mydrive \ >-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive > outl 0xcf8 0x80001810 > outl 0xcfc 0xe1068000 > outl 0xcf8 0x80001814 > outl 0xcf8 0x80001804 > outw 0xcfc 0x7 > outl 0xcf8 0x8000fa20 > write 0xe106802c 0x1 0x0f > write 0xe1068004 0xc 0x2801d10101fbff28a384 > write 0xe106800c 0x1f > 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f > write 0xe1068003 0x28 > 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576 > write 0xe1068003 0x1 0xfe > EOF > = > ==2686219==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x6153bb00 at pc 0x55ab469f456c bp 0x7ffee71be330 sp 0x7ffee71bdae0 > WRITE of size 4 at 0x6153bb00 thread T0 > #0 0x55ab469f456b in __asan_memcpy (qemu-system-i386+0x1cea56b) > #1 0x55ab483dc396 in stl_he_p include/qemu/bswap.h:353:5 > #2 0x55ab483af5e4 in stn_he_p include/qemu/bswap.h:546:1 > #3 0x55ab483aeb4b in flatview_read_continue softmmu/physmem.c:2839:13 > #4 0x55ab483b0705 in flatview_read softmmu/physmem.c:2877:12 > #5 0x55ab483b028e in address_space_read_full softmmu/physmem.c:2890:18 > #6 0x55ab483b1294 in address_space_rw softmmu/physmem.c:2918:16 > #7 0x55ab479374a2 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 > #8 0x55ab47936f50 in dma_memory_rw include/sysemu/dma.h:127:12 > #9 0x55ab4793665f in dma_memory_read include/sysemu/dma.h:145:12 > #10 0x55ab4792f176 in sdhci_sdma_transfer_multi_blocks > hw/sd/sdhci.c:639:13 > #11 0x55ab4793dc9d in sdhci_write hw/sd/sdhci.c:1129:17 > #12 0x55ab483f8db8 in memory_region_write_accessor > softmmu/memory.c:491:5 > #13 0x55ab483f868a in access_with_adjusted_size softmmu/memory.c:552:18 > #14 0x55ab483f6da5 in memory_region_dispatch_write > softmmu/memory.c:1501:16 > #15 0x55ab483c3b11 in flatview_write_continue softmmu/physmem.c:2774:23 > #16 0x55ab483b0eb6 in flatview_write softmmu/physmem.c:2814:14 > #17 0x55ab483b0a3e in address_space_write softmmu/physmem.c:2906:18 > #18 0x55ab48465c56 in qtest_process_command softmmu/qtest.c:654:9 > > 0x6153bb00 is located 0 bytes to the right of 512-byte region > [0x6153b900,0x6153bb00) > allocated by thread T0 here: > #0 0x55ab469f58a7 in calloc (qemu-system-i386+0x1ceb8a7) > #1 0x7f21d678f9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0) > #2 0x55ab479530ed in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5 > #3 0x55ab476f102a in pci_qdev_realize hw/pci/pci.c:2108:9 > #4 0x55ab48baaad2 in device_set_realized hw/core/qdev.c:761:13 > > SUMMARY: AddressSanitizer: heap-buffer-overflow > (qemu-system-i386+0x1cea56b) in __asan_memcpy > Shadow bytes around the buggy address: > 0x0c2a7710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c2a7720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c2a7730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c2a7740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c2a7750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > =>0x0c2a7760:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c2a7770: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd > 0x0c2a7780: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd > 0x0c2a7790: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd > 0x0c2a77a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd > 0x0c2a77b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > Shadow byte legend (one shadow byte represents 8 application bytes): > Addressable: 00 > Heap left redzone: fa > Freed heap region: fd > ==2686219==ABORTING > > Fixes: CVE-2020-17380 > Fixes: CVE-2020-25085 > Signed-off-b
Re: [PATCH] hw/sd: sdhci: Do not transfer any data when command fails
On 210209 1854, Bin Meng wrote:
> At the end of sdhci_send_command(), it starts a data transfer if
> the command register indicates a data is associated. However the
> data transfer should only be initiated when the command execution
> has succeeded.
>
> Cc: qemu-sta...@nongnu.org
> Fixes: CVE-2020-17380
> Fixes: CVE-2020-25085
> Reported-by: Alexander Bulekov
> Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> Reported-by: Simon Wrner (Ruhr-University Bochum)
> Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> Signed-off-by: Bin Meng
I applied this along with <20210208193450.2689517-1-f4...@amsat.org>
"hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress"
I ran through the entire OSS-Fuzz corpus, and could not reproduce the
crash.
Tested-by: Alexander Bulekov
Thanks
> ---
>
> hw/sd/sdhci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8ffa539..0450110 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
> SDRequest request;
> uint8_t response[16];
> int rlen;
> +bool cmd_failure = false;
>
> s->errintsts = 0;
> s->acmd12errsts = 0;
> @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
> trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
> s->rspreg[1], s->rspreg[0]);
> } else {
> +cmd_failure = true;
> trace_sdhci_error("timeout waiting for command response");
> if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
> s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
>
> sdhci_update_irq(s);
>
> -if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> +if (!cmd_failure && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> s->data_count = 0;
> sdhci_data_transfer(s);
> }
> --
> 2.7.4
>
Re: [PATCH] hw/sd: sdhci: Do not transfer any data when command fails
On 210209 1854, Bin Meng wrote:
> At the end of sdhci_send_command(), it starts a data transfer if
> the command register indicates a data is associated. However the
> data transfer should only be initiated when the command execution
> has succeeded.
>
> Cc: qemu-sta...@nongnu.org
> Fixes: CVE-2020-17380
> Fixes: CVE-2020-25085
> Reported-by: Alexander Bulekov
> Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> Reported-by: Simon Wrner (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
(don't know how to get the email from a launchpad report)
and probably:
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
> Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> Signed-off-by: Bin Meng
> ---
>
> hw/sd/sdhci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8ffa539..0450110 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
> SDRequest request;
> uint8_t response[16];
> int rlen;
> +bool cmd_failure = false;
>
> s->errintsts = 0;
> s->acmd12errsts = 0;
> @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
> trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
> s->rspreg[1], s->rspreg[0]);
> } else {
> +cmd_failure = true;
> trace_sdhci_error("timeout waiting for command response");
> if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
> s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
>
> sdhci_update_irq(s);
>
> -if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> +if (!cmd_failure && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> s->data_count = 0;
> sdhci_data_transfer(s);
> }
> --
> 2.7.4
>
Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver
On 210211 1526, Philippe Mathieu-Daudé wrote:
> The null-co driver doesn't zeroize buffer in its default config,
> because it is designed for testing and tests want to run fast.
> However this confuses security researchers (access to uninit
> buffers).
>
Interesting.. Is there an example bug report, where it raised alarms
because of an un-zeroed null-co:// buffer?
-Alex
> A one-line patch supposed which became a painful one, because
> there is so many different syntax to express the same usage:
>
> opt = qdict_new();
> qdict_put_str(opt, "read-zeroes", "off");
> null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> &error_abort);
>
> vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...)
>
> vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none")
>
> blk0 = { 'node-name': 'src',
> 'driver': 'null-co',
> 'read-zeroes': 'off' }
>
> 'file': {
> 'driver': 'null-co',
> 'read-zeroes': False,
> }
>
> "file": {
> "driver": "null-co",
> "read-zeroes": "off"
> }
>
> { "execute": "blockdev-add",
> "arguments": {
> "driver": "null-co",
> "read-zeroes": false,
> "node-name": "disk0"
> }
> }
>
> opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size':
> 1024}
>
> qemu -drive driver=null-co,read-zeroes=off
>
> qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}"
>
> qemu-img null-co://,read-zeroes=off
>
> qemu-img ... -o
> data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}"
>
> There are probably more.
>
> Anyhow, the iotests I am not sure and should be audited are 056, 155
> (I don't understand the syntax) and 162.
>
> Please review,
>
> Phil.
>
> Philippe Mathieu-Daud=C3=A9 (2):
> block: Explicit null-co uses 'read-zeroes=3Dfalse'
> block/null: Enable 'read-zeroes' mode by default
>
> docs/devel/testing.rst | 14 +++---
> tests/qtest/fuzz/generic_fuzz_configs.h| 11 ++-
> block/null.c | 2 +-
> tests/test-bdrv-drain.c| 10 --
> tests/acceptance/virtio_check_params.py| 2 +-
> tests/perf/block/qcow2/convert-blockstatus | 6 +++---
> tests/qemu-iotests/040 | 2 +-
> tests/qemu-iotests/041 | 12
> tests/qemu-iotests/051 | 2 +-
> tests/qemu-iotests/051.out | 2 +-
> tests/qemu-iotests/051.pc.out | 4 ++--
> tests/qemu-iotests/087 | 6 --
> tests/qemu-iotests/118 | 2 +-
> tests/qemu-iotests/133 | 2 +-
> tests/qemu-iotests/153 | 8
> tests/qemu-iotests/184 | 2 ++
> tests/qemu-iotests/184.out | 10 +-
> tests/qemu-iotests/218 | 3 +++
> tests/qemu-iotests/224 | 3 ++-
> tests/qemu-iotests/224.out | 8
> tests/qemu-iotests/225 | 2 +-
> tests/qemu-iotests/227 | 4 ++--
> tests/qemu-iotests/227.out | 4 ++--
> tests/qemu-iotests/228 | 2 +-
> tests/qemu-iotests/235 | 1 +
> tests/qemu-iotests/245 | 2 +-
> tests/qemu-iotests/270 | 2 +-
> tests/qemu-iotests/283 | 3 ++-
> tests/qemu-iotests/283.out | 4 ++--
> tests/qemu-iotests/299 | 1 +
> tests/qemu-iotests/299.out | 2 +-
> tests/qemu-iotests/300 | 4 ++--
> 32 files changed, 82 insertions(+), 60 deletions(-)
>
> --=20
> 2.26.2
>
>
Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress
gt;> softmmu/physmem.c:2839:13
> > > >> #4 0x55ab483b0705 in flatview_read softmmu/physmem.c:2877:12
> > > >> #5 0x55ab483b028e in address_space_read_full
> > > >> softmmu/physmem.c:2890:18
> > > >> #6 0x55ab483b1294 in address_space_rw softmmu/physmem.c:2918:16
> > > >> #7 0x55ab479374a2 in dma_memory_rw_relaxed
> > > >> include/sysemu/dma.h:88:12
> > > >> #8 0x55ab47936f50 in dma_memory_rw include/sysemu/dma.h:127:12
> > > >> #9 0x55ab4793665f in dma_memory_read include/sysemu/dma.h:145:12
> > > >> #10 0x55ab4792f176 in sdhci_sdma_transfer_multi_blocks
> > > >> hw/sd/sdhci.c:639:13
> > > >> #11 0x55ab4793dc9d in sdhci_write hw/sd/sdhci.c:1129:17
> > > >> #12 0x55ab483f8db8 in memory_region_write_accessor
> > > >> softmmu/memory.c:491:5
> > > >> #13 0x55ab483f868a in access_with_adjusted_size
> > > >> softmmu/memory.c:552:18
> > > >> #14 0x55ab483f6da5 in memory_region_dispatch_write
> > > >> softmmu/memory.c:1501:16
> > > >> #15 0x55ab483c3b11 in flatview_write_continue
> > > >> softmmu/physmem.c:2774:23
> > > >> #16 0x55ab483b0eb6 in flatview_write softmmu/physmem.c:2814:14
> > > >> #17 0x55ab483b0a3e in address_space_write
> > > >> softmmu/physmem.c:2906:18
> > > >> #18 0x55ab48465c56 in qtest_process_command softmmu/qtest.c:654:9
> > > >>
> > > >> 0x6153bb00 is located 0 bytes to the right of 512-byte region
> > > >> [0x6153b900,0x6153bb00)
> > > >> allocated by thread T0 here:
> > > >> #0 0x55ab469f58a7 in calloc (qemu-system-i386+0x1ceb8a7)
> > > >> #1 0x7f21d678f9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
> > > >> #2 0x55ab479530ed in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5
> > > >> #3 0x55ab476f102a in pci_qdev_realize hw/pci/pci.c:2108:9
> > > >> #4 0x55ab48baaad2 in device_set_realized hw/core/qdev.c:761:13
> > > >>
> > > >> SUMMARY: AddressSanitizer: heap-buffer-overflow
> > > >> (qemu-system-i386+0x1cea56b) in __asan_memcpy
> > > >> Shadow bytes around the buggy address:
> > > >> 0x0c2a7710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > >> 0x0c2a7720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > >> 0x0c2a7730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > >> 0x0c2a7740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > >> 0x0c2a7750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > >> =>0x0c2a7760:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > >> 0x0c2a7770: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> > > >> 0x0c2a7780: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> > > >> 0x0c2a7790: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> > > >> 0x0c2a77a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> > > >> 0x0c2a77b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > > >> Shadow byte legend (one shadow byte represents 8 application bytes):
> > > >> Addressable: 00
> > > >> Heap left redzone: fa
> > > >> Freed heap region: fd
> > > >> ==2686219==ABORTING
> > > >>
> > > >> Fixes: CVE-2020-17380
> > > >> Fixes: CVE-2020-25085
> > > >> Signed-off-by: Philippe Mathieu-Daudé
> > > >> ---
> > > >> Cc: Mauro Matteo Cascella
> > > >> Cc: Alexander Bulekov
> > > >> Cc: Alistair Francis
> > > >> Cc: Prasad J Pandit
> > > >> Cc: Bandan Das
> > > >>
> > > >> RFC because missing Reported-by tags, launchpad/bugzilla links and
> > > >> qtest reproducer. Sending for review meanwhile.
> > > >> ---
> > > >> hw/sd/sdhci.c | 6 ++
> > > >> 1 file changed, 6 insertions(+)
> > > >>
> > > >> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > > >> index 8ffa53999d8..7ac7d9af9e4 100644
> > > >> --- a/hw/sd/sdhci.c
> > > >> +++ b/hw/sd/sdhci.c
> > > >> @@ -1133,6 +1133,12 @@ sdhci_write(void *opaque, hwaddr offset,
> > > >> uint64_t val, unsigned size)
> > > >> }
> > > >> break;
> > > >> case SDHC_BLKSIZE:
> > > >> +if (s->data_count) {
> > > >> +qemu_log_mask(LOG_GUEST_ERROR,
> > > >> + "%s: Can not update blksize when"
> > > >> + " transaction is executing\n", __func__);
> > > >> +break;
> > > >> +}
> > > >> if (!TRANSFERRING_DATA(s->prnsts)) {
> > > >
> > > > I am not sure I get the whole picture here.
> > >
> > > The problem is out of bound access on fifo_buffer.
> > >
> > > > Isn't write to s->blksize and s->blkcnt already protected in this if
> > > > () statement?
>
> I tried to follow the CVE link from
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-25085 which
> gave me:
> https://bugs.launchpad.net/qemu/+bug/1892960
>
> The above page says this CVE is already fixed, so I am lost.
>
> > >
> > > I tried this code but it didn't work:
> > >
> > > -- >8 --
> > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > > index 8ffa53999d8..182641ae98a 100644
> > > --- a/hw/sd/sdhci.c
> > > +++ b/hw/sd/sdhci.c
> > > @@ -584,6 +584,11 @@ static void
> > > sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
> > > uint32_t boundary_chk = 1 << (((s->blksize & ~BLOCK_SIZE_MASK) >>
> > > 12) + 12);
> > > uint32_t boundary_count = boundary_chk - (s->sdmasysad %
> > > boundary_chk);
> > >
> > > +if (TRANSFERRING_DATA(s->prnsts)) {
> > > +qemu_log_mask(LOG_GUEST_ERROR,
> > > + "%s: Transfer already in progress", __func__);
> > > +return;
> > > +}
> > > if (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || !s->blkcnt) {
> > > qemu_log_mask(LOG_UNIMP, "infinite transfer is not supported\n");
> > > return;
> > > ---
> > >
> > > Do you think we need both?
> > >
> > > Maybe we miss to set a bit in s->prnsts somewhere...
>
> Probably, but I need to take a further look.
>
> Regards,
> Bin
Re: [PATCH v2 1/2] tests/qtest: Only run fuzz-megasas-test if megasas device is available
On 210126 1851, Thomas Huth wrote:
> On 26/01/2021 12.16, Philippe Mathieu-Daudé wrote:
> > This test fails when QEMU is built without the megasas device,
> > restrict it to its availability.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > tests/qtest/fuzz-megasas-test.c | 49 +
> > tests/qtest/fuzz-test.c | 25 -
> > MAINTAINERS | 1 +
> > tests/qtest/meson.build | 4 ++-
> > 4 files changed, 53 insertions(+), 26 deletions(-)
> > create mode 100644 tests/qtest/fuzz-megasas-test.c
> >
> > diff --git a/tests/qtest/fuzz-megasas-test.c
> > b/tests/qtest/fuzz-megasas-test.c
> > new file mode 100644
> > index 000..940a76bf25a
> > --- /dev/null
> > +++ b/tests/qtest/fuzz-megasas-test.c
> > @@ -0,0 +1,49 @@
> > +/*
> > + * QTest fuzzer-generated testcase for megasas device
> > + *
> > + * Copyright (c) 2020 Li Qiang
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "libqos/libqtest.h"
> > +
> > +/*
> > + * This used to trigger the assert in scsi_dma_complete
> > + * https://bugs.launchpad.net/qemu/+bug/1878263
> > + */
> > +static void test_lp1878263_megasas_zero_iov_cnt(void)
> > +{
> > +QTestState *s;
> > +
> > +s = qtest_init("-nographic -monitor none -serial none "
> > + "-M q35 -device megasas -device scsi-cd,drive=null0 "
> > + "-blockdev
> > driver=null-co,read-zeroes=on,node-name=null0");
> > +qtest_outl(s, 0xcf8, 0x80001818);
> > +qtest_outl(s, 0xcfc, 0xc101);
> > +qtest_outl(s, 0xcf8, 0x8000181c);
> > +qtest_outl(s, 0xcf8, 0x80001804);
> > +qtest_outw(s, 0xcfc, 0x7);
> > +qtest_outl(s, 0xcf8, 0x8000186a);
> > +qtest_writeb(s, 0x14, 0xfe);
> > +qtest_writeb(s, 0x0, 0x02);
> > +qtest_outb(s, 0xc1c0, 0x17);
> > +qtest_quit(s);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +const char *arch = qtest_get_arch();
> > +
> > +g_test_init(&argc, &argv, NULL);
> > +
> > +if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> > +qtest_add_func("fuzz/test_lp1878263_megasas_zero_iov_cnt",
> > + test_lp1878263_megasas_zero_iov_cnt);
> > +}
> > +
> > +return g_test_run();
> > +}
> > diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> > index cdb1100a0b8..6188fbb8e96 100644
> > --- a/tests/qtest/fuzz-test.c
> > +++ b/tests/qtest/fuzz-test.c
> > @@ -11,29 +11,6 @@
> > #include "libqos/libqtest.h"
> > -/*
> > - * This used to trigger the assert in scsi_dma_complete
> > - * https://bugs.launchpad.net/qemu/+bug/1878263
> > - */
> > -static void test_lp1878263_megasas_zero_iov_cnt(void)
> > -{
> > -QTestState *s;
> > -
> > -s = qtest_init("-nographic -monitor none -serial none "
> > - "-M q35 -device megasas -device scsi-cd,drive=null0 "
> > - "-blockdev
> > driver=null-co,read-zeroes=on,node-name=null0");
> > -qtest_outl(s, 0xcf8, 0x80001818);
> > -qtest_outl(s, 0xcfc, 0xc101);
> > -qtest_outl(s, 0xcf8, 0x8000181c);
> > -qtest_outl(s, 0xcf8, 0x80001804);
> > -qtest_outw(s, 0xcfc, 0x7);
> > -qtest_outl(s, 0xcf8, 0x8000186a);
> > -qtest_writeb(s, 0x14, 0xfe);
> > -qtest_writeb(s, 0x0, 0x02);
> > -qtest_outb(s, 0xc1c0, 0x17);
> > -qtest_quit(s);
> > -}
> > -
> > static void test_lp1878642_pci_bus_get_irq_level_assert(void)
> > {
> > QTestState *s;
> > @@ -104,8 +81,6 @@ int main(int argc, char **argv)
> > g_test_init(&argc, &argv, NULL);
> > if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> > -qtest_add_func("fuzz/test_lp1878263_megasas_zero_iov_cnt",
> > - test_lp1878263_megasas_zero_iov_cnt);
> > qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
> > test_lp1878642_pci_bus_get_irq_level_assert);
> > qtest_add_func("fuzz/test_mmio_oob_from_memory_region_cache",
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 34359a99b8e..44cd74b03cd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1925,6 +1925,7 @@ S: Supported
> > F: hw/scsi/megasas.c
> > F: hw/scsi/mfi.h
> > F: tests/qtest/megasas-test.c
> > +F: tests/qtest/fuzz-megasas-test.c
> > Network packet abstractions
> > M: Dmitry Fleytman
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index 16d04625b8b..85682d0dfce 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -4,7 +4,9 @@
> > subdir_done()
> > endif
> > -qtests_generic = [
> > +qtests_generic = \
> > + (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ?
> > ['fuzz-megasas-test'] : []) + \
> > + [
> > 'cdrom-test',
> > 'device-introspect-test',
> > 'machine-none
[PATCH] hw/ide/ahci: map cmd_fis as DMA_DIRECTION_TO_DEVICE
cmd_fis is mapped as DMA_DIRECTION_FROM_DEVICE, however, it is read
from, and not written to anywhere. Fix the DMA_DIRECTION and mark
cmd_fis as read-only in the code.
Signed-off-by: Alexander Bulekov
---
hw/ide/ahci.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4b675b9cfd..6432d44ad8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -699,7 +699,7 @@ static void ahci_reset_port(AHCIState *s, int port)
}
/* Buffer pretty output based on a raw FIS structure. */
-static char *ahci_pretty_buffer_fis(uint8_t *fis, int cmd_len)
+static char *ahci_pretty_buffer_fis(const uint8_t *fis, int cmd_len)
{
int i;
GString *s = g_string_new("FIS:");
@@ -1099,11 +1099,11 @@ static void execute_ncq_command(NCQTransferState
*ncq_tfs)
}
-static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
+static void process_ncq_command(AHCIState *s, int port, const uint8_t *cmd_fis,
uint8_t slot)
{
AHCIDevice *ad = &s->dev[port];
-NCQFrame *ncq_fis = (NCQFrame*)cmd_fis;
+const NCQFrame *ncq_fis = (NCQFrame *)cmd_fis;
uint8_t tag = ncq_fis->tag >> 3;
NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag];
size_t size;
@@ -1183,7 +1183,7 @@ static AHCICmdHdr *get_cmd_header(AHCIState *s, uint8_t
port, uint8_t slot)
}
static void handle_reg_h2d_fis(AHCIState *s, int port,
- uint8_t slot, uint8_t *cmd_fis)
+ uint8_t slot, const uint8_t *cmd_fis)
{
IDEState *ide_state = &s->dev[port].port.ifs[0];
AHCICmdHdr *cmd = get_cmd_header(s, port, slot);
@@ -1299,7 +1299,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t
slot)
tbl_addr = le64_to_cpu(cmd->tbl_addr);
cmd_len = 0x80;
cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len,
- DMA_DIRECTION_FROM_DEVICE);
+ DMA_DIRECTION_TO_DEVICE);
if (!cmd_fis) {
trace_handle_cmd_badfis(s, port);
return -1;
@@ -1324,7 +1324,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t
slot)
}
out:
-dma_memory_unmap(s->as, cmd_fis, cmd_len, DMA_DIRECTION_FROM_DEVICE,
+dma_memory_unmap(s->as, cmd_fis, cmd_len, DMA_DIRECTION_TO_DEVICE,
cmd_len);
if (s->dev[port].port.ifs[0].status & (BUSY_STAT|DRQ_STAT)) {
--
2.28.0
Re: [PATCH 3/4] tests/qtest: Only run fuzz-megasas-test if megasas device is available
On 210115 1609, Philippe Mathieu-Daudé wrote: > This test fails when QEMU is built without the megasas device, > restrict it to its availability. Should we just make a separate directory for fuzzer tests and have a separate source file for each reproducer (or for each device)? That way, we avoid confusion about what to do with new reproducers: they always go into e.g. tests/qtest/reproducers/device_name.c
Re: [PATCH] hw/scsi/megasas: check for NULL frame in megasas_command_cancelled()
Looks like one reported by OSS-Fuzz:
Here's a reproducer
cat << EOF | ./qemu-system-i386 -qtest stdio -display none \
-machine q35,accel=qtest -m 512M -nodefaults \
-device megasas -device scsi-cd,drive=null0 \
-blockdev driver=null-co,read-zeroes=on,node-name=null0
outl 0xcf8 0x8801
outl 0xcfc 0x1500
outl 0xcf8 0x8817
outl 0xcfc 0x1e
write 0x40 0x1 0x01
write 0x47 0x1 0x03
write 0x50 0x1 0x12
write 0x55 0x1 0x10
write 0x6a 0x1 0x20
write 0x70 0x1 0x10
write 0x7b 0x1 0x10
write 0x7f 0x1 0x10
write 0x86 0x1 0x10
write 0x8b 0x1 0x10
outb 0x1e40 0x40
write 0x1a 0x1 0x0
write 0x6a000f 0x1 0x0
outb 0x1e40 0x0
outl 0x1e40 0x0
write 0x6f1 0x1 0x00
write 0x6f9 0x1 0x00
write 0x6fd 0x1 0x01
write 0x701 0x1 0x00
write 0x705 0x1 0x06
write 0x730 0x1 0x00
write 0x738 0x1 0x00
write 0x73c 0x1 0x01
write 0x740 0x1 0x00
write 0x744 0x1 0x06
write 0x75c 0x1 0x00
write 0x760 0x1 0x01
write 0x76f 0x1 0x00
write 0x770 0x1 0x20
write 0x77c 0x1 0x20
write 0x780 0x1 0x00
write 0x79b 0x1 0x00
write 0x79f 0x1 0x01
write 0x7ae 0x1 0x00
write 0x7af 0x1 0x20
write 0x7bb 0x1 0x20
write 0x7bf 0x1 0x00
write 0x7cf 0x1 0x10
write 0x7db 0x1 0x00
write 0x7df 0x1 0x20
write 0x7ee 0x1 0x20
write 0x7ef 0x1 0x06
write 0x7fb 0x1 0x10
write 0x7ff 0x1 0x00
outb 0x1e40 0x0
outl 0x1e1f 0x4200
EOF
-Alex
On 201224 1854, Mauro Matteo Cascella wrote:
> Ensure that 'cmd->frame' is not NULL before accessing the 'header' field.
> This check prevents a potential NULL pointer dereference issue.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1910346
> Signed-off-by: Mauro Matteo Cascella
> Reported-by: Cheolwoo Myung
> ---
> hw/scsi/megasas.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 1a5fc5857d..77510e120c 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1893,7 +1893,7 @@ static void megasas_command_cancelled(SCSIRequest *req)
> {
> MegasasCmd *cmd = req->hba_private;
>
> -if (!cmd) {
> +if (!cmd || !cmd->frame) {
> return;
> }
> cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED;
> --
> 2.29.2
>
>
[PATCH] floppy: remove unused function fdctrl_format_sector
fdctrl_format_sector was added in
baca51faff ("updated floppy driver: formatting code, disk geometry auto detect
(Jocelyn Mayer)")
The single callsite is guarded by a check:
fdctrl->data_state & FD_STATE_FORMAT
However, the only place where the FD_STATE_FORMAT flag is set (in
fdctrl_handle_format_track) is closely followed by the same flag being
unset, with no possibility to call fdctrl_format_sector in between.
This removes fdctrl_format_sector and the unncessary setting/unsetting
of the FD_STATE_FORMAT flag.
Signed-off-by: Alexander Bulekov
---
hw/block/fdc.c | 68 --
1 file changed, 68 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 3636874432..837dd819ea 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1952,67 +1952,6 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
return retval;
}
-static void fdctrl_format_sector(FDCtrl *fdctrl)
-{
-FDrive *cur_drv;
-uint8_t kh, kt, ks;
-
-SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
-cur_drv = get_cur_drv(fdctrl);
-kt = fdctrl->fifo[6];
-kh = fdctrl->fifo[7];
-ks = fdctrl->fifo[8];
-FLOPPY_DPRINTF("format sector at %d %d %02x %02x (%d)\n",
- GET_CUR_DRV(fdctrl), kh, kt, ks,
- fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
- NUM_SIDES(cur_drv)));
-switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
-case 2:
-/* sect too big */
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
-fdctrl->fifo[3] = kt;
-fdctrl->fifo[4] = kh;
-fdctrl->fifo[5] = ks;
-return;
-case 3:
-/* track too big */
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_EC, 0x00);
-fdctrl->fifo[3] = kt;
-fdctrl->fifo[4] = kh;
-fdctrl->fifo[5] = ks;
-return;
-case 4:
-/* No seek enabled */
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
-fdctrl->fifo[3] = kt;
-fdctrl->fifo[4] = kh;
-fdctrl->fifo[5] = ks;
-return;
-case 1:
-fdctrl->status0 |= FD_SR0_SEEK;
-break;
-default:
-break;
-}
-memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
-if (cur_drv->blk == NULL ||
-blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
- BDRV_SECTOR_SIZE, 0) < 0) {
-FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
-fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
-} else {
-if (cur_drv->sect == cur_drv->last_sect) {
-fdctrl->data_state &= ~FD_STATE_FORMAT;
-/* Last sector done */
-fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
-} else {
-/* More to do */
-fdctrl->data_pos = 0;
-fdctrl->data_len = 4;
-}
-}
-}
-
static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
{
fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
@@ -2126,7 +2065,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl,
int direction)
SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
cur_drv = get_cur_drv(fdctrl);
-fdctrl->data_state |= FD_STATE_FORMAT;
if (fdctrl->fifo[0] & 0x80)
fdctrl->data_state |= FD_STATE_MULTI;
else
@@ -2144,7 +2082,6 @@ static void fdctrl_handle_format_track(FDCtrl *fdctrl,
int direction)
* and Linux fdformat (read 3 bytes per sector via DMA and fill
* the sector with the specified fill byte
*/
-fdctrl->data_state &= ~FD_STATE_FORMAT;
fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
}
@@ -2458,11 +2395,6 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t
value)
/* We have all parameters now, execute the command */
fdctrl->phase = FD_PHASE_EXECUTION;
-if (fdctrl->data_state & FD_STATE_FORMAT) {
-fdctrl_format_sector(fdctrl);
-break;
-}
-
cmd = get_command(fdctrl->fifo[0]);
FLOPPY_DPRINTF("Calling handler for '%s'\n", cmd->name);
cmd->handler(fdctrl, cmd->direction);
--
2.27.0
[PATCH] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
From: Philippe Mathieu-Daudé
cdb_len can not be zero... (or less than 6) here, else we have a
out-of-bound read first in scsi_cdb_length():
71 int scsi_cdb_length(uint8_t *buf)
72 {
73 int cdb_len;
74
75 switch (buf[0] >> 5) {
76 case 0:
77 cdb_len = 6;
78 break;
Then another out-of-bound read when the size returned by
scsi_cdb_length() is used.
Add a reproducer which triggers:
$ make check-qtest-x86_64
Running test qtest-x86_64/fuzz-test
qemu-system-x86_64: hw/scsi/megasas.c:1679: megasas_handle_scsi: Assertion
`cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len' failed.
tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6
(Aborted) (core dumped)
ERROR qtest-x86_64/fuzz-test - too few tests run (expected 1, got 0)
Signed-off-by: Alexander Bulekov
Signed-off-by: Philippe Mathieu-Daudé
---
Ran it through the minimizer - No more blobs.
hw/scsi/megasas.c | 1 +
tests/qtest/fuzz-test.c | 19 +++
2 files changed, 20 insertions(+)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 1a5fc5857d..28efd09411 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1676,6 +1676,7 @@ static int megasas_handle_scsi(MegasasState *s,
MegasasCmd *cmd,
lun_id = cmd->frame->header.lun_id;
cdb_len = cmd->frame->header.cdb_len;
+assert(cdb_len > 0 && scsi_cdb_length(cdb) >= cdb_len);
if (is_logical) {
if (target_id >= MFI_MAX_LD || lun_id != 0) {
trace_megasas_scsi_target_not_present(
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 87b72307a5..31f90cfb4f 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -48,6 +48,23 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_quit(s);
}
+static void test_megasas_cdb_len_zero(void)
+{
+QTestState *s;
+
+s = qtest_init("-M pc -nodefaults "
+ "-device megasas-gen2 -device scsi-cd,drive=null0 "
+ "-blockdev driver=null-co,read-zeroes=on,node-name=null0");
+
+qtest_outl(s, 0xcf8, 0x80001011);
+qtest_outb(s, 0xcfc, 0xbb);
+qtest_outl(s, 0xcf8, 0x80001002);
+qtest_outl(s, 0xcfc, 0xf3ff2966);
+qtest_writeb(s, 0x4600, 0x03);
+qtest_outw(s, 0xbb40, 0x460b);
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -59,6 +76,8 @@ int main(int argc, char **argv)
test_lp1878263_megasas_zero_iov_cnt);
qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
test_lp1878642_pci_bus_get_irq_level_assert);
+qtest_add_func("fuzz/test_megasas_cdb_len_zero",
+ test_megasas_cdb_len_zero);
}
return g_test_run();
--
2.28.0
Re: [PATCH v2 0/6] hw/sd/sdcard: Do not attempt to erase out of range addresses
On 201015 0838, Philippe Mathieu-Daudé wrote: > Yet another bug in the sdcard model found by libfuzzer: > https://bugs.launchpad.net/bugs/1895310 > > Since RFC: Settled migration issue > > Philippe Mathieu-Daudé (6): > hw/sd/sdcard: Add trace event for ERASE command (CMD38) > hw/sd/sdcard: Introduce the INVALID_ADDRESS definition > hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS > hw/sd/sdcard: Reset both start/end addresses on error > hw/sd/sdcard: Do not attempt to erase out of range addresses > hw/sd/sdcard: Assert if accessing an illegal group > > hw/sd/sd.c | 30 ++ > hw/sd/trace-events | 2 +- > 2 files changed, 23 insertions(+), 9 deletions(-) > > -- > 2.26.2 > Hi Phil, For this series: Tested-by: Alexander Bulekov Thanks -Alex
Re: [PATCH] hw: ide: check the pointer before do dma memory unmap
On 200815 0020, Li Qiang wrote:
> In 'map_page' we need to check the return value of
> 'dma_memory_map' to ensure the we actully maped something.
> Otherwise, we will hit an assert in 'address_space_unmap'.
> This is because we can't find the MR with the NULL buffer.
> This is the LP#1884693:
>
> -->https://bugs.launchpad.net/qemu/+bug/1884693
>
> Reported-by: Alexander Bulekov
> Signed-off-by: Li Qiang
I'm not very familiar with the IDE code, but this seems like a simple
null-ptr check, and Li has not received a response in over a month.
Reviewed-by: Alexander Bulekov
> ---
> hw/ide/ahci.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 009120f88b..63e9fccdbe 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t **ptr,
> uint64_t addr,
> }
>
> *ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
> +
> +if (!*ptr) {
> +return;
> +}
> +
> if (len < wanted) {
> dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> *ptr = NULL;
> --
> 2.17.1
>
Re: [PATCH v2 0/3] hw/sd/sdhci: Fix DMA Transfer Block Size field width
For this series: Tested-by: Alexander Bulekov On 200901 1604, Philippe Mathieu-Daudé wrote: > Fix the SDHCI issue reported last week by Alexander: > https://bugs.launchpad.net/qemu/+bug/1892960 > > The field is 12-bit (4KiB) but the guest can set > up to 16-bit (64KiB), leading to OOB access. > > since v1: > commited unstaged change in patch #3... > > Philippe Mathieu-Daudé (3): > hw/sd/sdhci: Fix qemu_log_mask() format string > hw/sd/sdhci: Document the datasheet used > hw/sd/sdhci: Fix DMA Transfer Block Size field > > hw/sd/sdhci.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > -- > 2.26.2 >
Re: [PATCH 0/4] hw/sd/sdhci: Strengthen multiple DMA transfers
I fuzzed the SDHCI with this applied. There are still bugs in SDHCI, but this fixes the ones triggered by my initial bug-reproducers, and doesn't appear to create any new bugs. In the interest of incrementally fixing the issues, for this series: Tested-by: Alexander Bulekov On 200903 1928, Philippe Mathieu-Daudé wrote: > Still trying to fix the bugs reported by Aleksander... > > - Do not send 0 block count > - Reduce DMA to MMIO re-entrancy by yielding when pending IRQ > > Based-on: <20200901140411.112150-1-f4...@amsat.org> > > Philippe Mathieu-Daudé (4): > hw/sd/sdhci: Stop multiple transfers when block count is cleared > hw/sd/sdhci: Resume pending DMA transfers on MMIO accesses > hw/sd/sdhci: Let sdhci_update_irq() return if IRQ was delivered > hw/sd/sdhci: Yield if interrupt delivered during multiple transfer > > hw/sd/sdhci.c | 35 +++ > 1 file changed, 31 insertions(+), 4 deletions(-) > > -- > 2.26.2 >
Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer
Here's a qtest reproducer for this one:
cat << EOF |./i386-softmmu/qemu-system-i386 -nodefaults \
-device sdhci-pci -device sd-card,drive=mydrive \
-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
-nographic -accel qtest -qtest stdio -nographic
outl 0xcf8 0x80001001
outl 0xcfc 0x7e6f25b7
outl 0xcf8 0x80001012
outl 0xcfc 0x842b1212
writeb 0x12120005 0xff
writeq 0x12120027 0x5e32b7120584125e
write 0x0 0x1 0x21
write 0x8 0x1 0x21
write 0x10 0x1 0x21
write 0x18 0x1 0x21
write 0x20 0x1 0x21
write 0x23 0x1 0x2b
writeq 0x1212000c 0x123a0584052da3ab
writeq 0x1212 0xcfff0002
writeq 0x12120027 0x5c04c1c9c15e
clock_step
EOF
Is it related to this https://bugs.launchpad.net/qemu/+bug/1892960 ?
-Alex
On 200827 1723, P J P wrote:
> From: Prasad J Pandit
>
> While doing multi block SDMA, transfer block size may exceed
> the 's->fifo_buffer[s->buf_maxsz]' size. It may leave the
> current element pointer 's->data_count' pointing out of bounds.
> Leading the subsequent DMA r/w operation to OOB access issue.
> Add check to avoid it.
>
> ->
> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1
> ==1459837==ERROR: AddressSanitizer: heap-buffer-overflow
> WRITE of size 54722048 at 0x6151e280 thread T3
> #0 __interceptor_memcpy (/lib64/libasan.so.6+0x3a71d)
> #1 flatview_read_continue ../exec.c:3245
> #2 flatview_read ../exec.c:3278
> #3 address_space_read_full ../exec.c:3291
> #4 address_space_rw ../exec.c:3319
> #5 dma_memory_rw_relaxed ../include/sysemu/dma.h:87
> #6 dma_memory_rw ../include/sysemu/dma.h:110
> #7 dma_memory_read ../include/sysemu/dma.h:116
> #8 sdhci_sdma_transfer_multi_blocks ../hw/sd/sdhci.c:629
> #9 sdhci_write ../hw/sd/sdhci.c:1097
> #10 memory_region_write_accessor ../softmmu/memory.c:483
> ...
>
> Reported-by: Ruhr-University
> Signed-off-by: Prasad J Pandit
> ---
> hw/sd/sdhci.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 1785d7e1f7..155e25ceee 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -604,6 +604,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState
> *s)
> s->blkcnt--;
> }
> }
> +if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
> +break;
> +}
> dma_memory_write(s->dma_as, s->sdmasysad,
> &s->fifo_buffer[begin], s->data_count - begin);
> s->sdmasysad += s->data_count - begin;
> @@ -626,6 +629,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState
> *s)
> s->data_count = block_size;
> boundary_count -= block_size - begin;
> }
> +if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
> +break;
> +}
> dma_memory_read(s->dma_as, s->sdmasysad,
> &s->fifo_buffer[begin], s->data_count - begin);
> s->sdmasysad += s->data_count - begin;
> --
> 2.26.2
>
>
Re: [PATCH v2 8/9] hw/sd/sdcard: Update coding style to make checkpatch.pl happy
On 200713 2032, Philippe Mathieu-Daudé wrote:
> To make the next commit easier to review, clean this code first.
>
> Reviewed-by: Peter Maydell
> Signed-off-by: Philippe Mathieu-Daudé
> Reviewed-by: Alistair Francis
> Message-Id: <20200630133912.9428-3-f4...@amsat.org>
> ---
Reviewed-by: Alexander Bulekov
> hw/sd/sd.c | 24
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 5ab945dade..0f048358ab 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1175,8 +1175,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> sd->data_start = addr;
> sd->data_offset = 0;
>
> -if (sd->data_start + sd->blk_len > sd->size)
> +if (sd->data_start + sd->blk_len > sd->size) {
> sd->card_status |= ADDRESS_ERROR;
> +}
> return sd_r1;
>
> default:
> @@ -1191,8 +1192,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> sd->data_start = addr;
> sd->data_offset = 0;
>
> -if (sd->data_start + sd->blk_len > sd->size)
> +if (sd->data_start + sd->blk_len > sd->size) {
> sd->card_status |= ADDRESS_ERROR;
> +}
> return sd_r1;
>
> default:
> @@ -1237,12 +1239,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> sd->data_offset = 0;
> sd->blk_written = 0;
>
> -if (sd->data_start + sd->blk_len > sd->size)
> +if (sd->data_start + sd->blk_len > sd->size) {
> sd->card_status |= ADDRESS_ERROR;
> -if (sd_wp_addr(sd, sd->data_start))
> +}
> +if (sd_wp_addr(sd, sd->data_start)) {
> sd->card_status |= WP_VIOLATION;
> -if (sd->csd[14] & 0x30)
> +}
> +if (sd->csd[14] & 0x30) {
> sd->card_status |= WP_VIOLATION;
> +}
> return sd_r1;
>
> default:
> @@ -1261,12 +1266,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> sd->data_offset = 0;
> sd->blk_written = 0;
>
> -if (sd->data_start + sd->blk_len > sd->size)
> +if (sd->data_start + sd->blk_len > sd->size) {
> sd->card_status |= ADDRESS_ERROR;
> -if (sd_wp_addr(sd, sd->data_start))
> +}
> +if (sd_wp_addr(sd, sd->data_start)) {
> sd->card_status |= WP_VIOLATION;
> -if (sd->csd[14] & 0x30)
> +}
> +if (sd->csd[14] & 0x30) {
> sd->card_status |= WP_VIOLATION;
> +}
> return sd_r1;
>
> default:
> --
> 2.21.3
>
Re: [PATCH 1/3] Use &error_abort instead of separate assert()
On 200313 1805, Markus Armbruster wrote: > Signed-off-by: Markus Armbruster > index 1a99277d60..aa9eee6ebf 100644 > --- a/tests/qtest/fuzz/qos_fuzz.c > +++ b/tests/qtest/fuzz/qos_fuzz.c > @@ -57,8 +57,7 @@ static void qos_set_machines_devices_available(void) > QList *lst; > Error *err = NULL; Can this err declaration be removed? Don't think it's used anywhere else. > > -qmp_marshal_query_machines(NULL, &response, &err); > -assert(!err); > +qmp_marshal_query_machines(NULL, &response, &error_abort); > lst = qobject_to(QList, response); > apply_to_qlist(lst, true); > > @@ -70,8 +69,7 @@ static void qos_set_machines_devices_available(void) > qdict_put_bool(args, "abstract", true); > qdict_put_obj(req, "arguments", (QObject *) args); > > -qmp_marshal_qom_list_types(args, &response, &err); > -assert(!err); > +qmp_marshal_qom_list_types(args, &response, &error_abort); > lst = qobject_to(QList, response); > apply_to_qlist(lst, false); > qobject_unref(response); > -- > 2.21.1 > Thanks! Acked-by: Alexander Bulekov
Re: [PULL 24/31] fuzz: support for fork-based fuzzing.
On 200224 1135, Stefan Hajnoczi wrote: > On Sat, Feb 22, 2020 at 05:34:29AM -0600, Eric Blake wrote: > > On 2/22/20 2:50 AM, Stefan Hajnoczi wrote: > > > From: Alexander Bulekov > > > > > > fork() is a simple way to ensure that state does not leak in between > > > fuzzing runs. Unfortunately, the fuzzer mutation engine relies on > > > bitmaps which contain coverage information for each fuzzing run, and > > > these bitmaps should be copied from the child to the parent(where the > > > mutation occurs). These bitmaps are created through compile-time > > > instrumentation and they are not shared with fork()-ed processes, by > > > default. To address this, we create a shared memory region, adjust its > > > size and map it _over_ the counter region. Furthermore, libfuzzer > > > doesn't generally expose the globals that specify the location of the > > > counters/coverage bitmap. As a workaround, we rely on a custom linker > > > script which forces all of the bitmaps we care about to be placed in a > > > contiguous region, which is easy to locate and mmap over. > > > > > > Signed-off-by: Alexander Bulekov > > > Reviewed-by: Stefan Hajnoczi > > > Reviewed-by: Darren Kenny > > > Message-id: 20200220041118.23264-16-alx...@bu.edu > > > Signed-off-by: Stefan Hajnoczi > > > --- > > > > Random drive-by observation: > > > > > +++ b/tests/qtest/fuzz/fork_fuzz.ld > > > @@ -0,0 +1,37 @@ > > > +/* We adjust linker script modification to place all of the stuff that > > > needs to > > > + * persist across fuzzing runs into a contiguous seciton of memory. > > > Then, it is > > > > section > > Thanks, Eric! > > Alex, please send follow-up patches to fix this typo and the 80 > character line limit issues identified by patchew (see patch email reply > to this email thread). Thank you Eric, Stefan! Just sent out some fixes. > Stefan
