qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH] block: Fix qemu crash when using s


From: Deepa Srinivasan
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH] block: Fix qemu crash when using scsi-block
Date: Wed, 22 Nov 2017 18:55:52 -0800

I agree that passing in QEMUIOVector to blk_aio_ioctl() as a holder of the void* buffer used in blk_aio_ioctl_entry() is unnecessary. But, as Kevin noted, read and write were using the QEMUIOVector in BlkRwCo.

To avoid changes to the callers of blk_aio_ioctl(), I’ll change blk_aio_prwv() to take a void pointer instead of QEMUIOVector* and use a union to hold the buffer in BlkRwCo.

On Nov 22, 2017, at 11:24 AM, Paolo Bonzini <address@hidden> wrote:

On 22/11/2017 19:06, Kevin Wolf wrote:
Am 22.11.2017 um 17:34 hat Paolo Bonzini geschrieben:
On 22/11/2017 16:33, Deepa Srinivasan wrote:
Starting qemu with the following arguments causes qemu to segfault:
... -device lsi,id=lsi0 -drive file=iscsi:<...>,format=raw,if=none,node-name=
iscsi1 -device scsi-block,bus=lsi0.0,id=<...>,drive=iscsi1

This patch fixes blk_aio_ioctl() so it does not pass stack addresses to
blk_aio_ioctl_entry() which may be invoked after blk_aio_ioctl() returns. More
details about the bug follow.

blk_aio_ioctl() invokes blk_aio_prwv() with blk_aio_ioctl_entry as the
coroutine parameter. blk_aio_prwv() ultimately calls aio_co_enter().

When blk_aio_ioctl() is executed from within a coroutine context (e.g.
iscsi_bh_cb()), aio_co_enter() adds the coroutine (blk_aio_ioctl_entry) to
the current coroutine's wakeup queue. blk_aio_ioctl() then returns.

When blk_aio_ioctl_entry() executes later, it accesses an invalid pointer:
....
   BlkRwCo *rwco = &acb->rwco;

   rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset,
                            rwco->qiov->iov[0].iov_base);  <--- qiov is
                                                                invalid here
...

In the case when blk_aio_ioctl() is called from a non-coroutine context,
blk_aio_ioctl_entry() executes immediately. But if bdrv_co_ioctl() calls
qemu_coroutine_yield(), blk_aio_ioctl() will return. When the coroutine
execution is complete, control returns to blk_aio_ioctl_entry() after the call
to blk_co_ioctl(). There is no invalid reference after this point, but the
function is still holding on to invalid pointers.

The fix is to allocate memory for the QEMUIOVector and struct iovec as part of
the request struct which the IO buffer is part of. The memory for this struct is
guaranteed to be valid till the AIO is completed.

Signed-off-by: Deepa Srinivasan <address@hidden>
Signed-off-by: Konrad Rzeszutek Wilk <address@hidden>
Reviewed-by: Mark Kanda <address@hidden>
---
block/block-backend.c          | 13 ++-----------
hw/block/virtio-blk.c          |  9 ++++++++-
hw/scsi/scsi-disk.c            | 10 +++++++++-
hw/scsi/scsi-generic.c         |  9 ++++++++-
include/sysemu/block-backend.h |  2 +-
5 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index baef8e7..c275827 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1472,19 +1472,10 @@ static void blk_aio_ioctl_entry(void *opaque)
    blk_aio_complete(acb);
}

-BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
+BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, QEMUIOVector *qiov,
                          BlockCompletionFunc *cb, void *opaque)

I think this is not the best way to fix the bug, because it adds extra
unnecessary code in the callers.

Perhaps you can change BlkRwCo's "qiov" field to "void *buf" and the
same for blk_aio_prwv's "qiov" argument?

Then the QEMUIOVector is not needed at all, and blk_co_ioctl can just
use rwco->buf.

But the same struct is used for read and write requests that do use an
actual QEMUIOVector and not just a linear buffer.

Then let's call it "void *opaque", or make it a union (but I think
that's overkill).

The QEMUIOVector pointer is opaque as far as blk_aio_prwv is concerned,
and it is only created by blk_aio_ioctl for blk_aio_ioctl_entry to
extract buf:

   rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset,
                            rwco->qiov->iov[0].iov_base);

Exposing the fake QEMUIOVector to the callers of blk_aio_ioctl is much
uglier than using a void* for what is effectively a multi-type pointer.

Paolo


reply via email to

[Prev in Thread] Current Thread [Next in Thread]