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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

Am 22.11.2017 um 18:06 hat Stefan Hajnoczi geschrieben:
> On Wed, Nov 22, 2017 at 07:33:28AM -0800, 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.
> 
> Thanks for the patch!
> 
> AIO APIs currently don't require the caller to match qiov's lifetime to
> the I/O request lifetime.  This patch changes that for blk_aio_ioctl()
> only.  If we want to do this consistently then all aio callers need to
> be audited and fixed.
> 
> The alternative is to make the API copy qiov when necessary.  That is
> less efficient but avoids modifying all callers.
> 
> Either way, the lifetime of qiov must be consistent across all aio APIs,
> not just blk_aio_ioctl().

Don't all blk_aio_*() APIs that take a qiov pointer require that it
remains valid until the request completes? I don't think they are copied
anywhere for blk_aio_preadv/pwritev() before being passed to the block
driver.

So this does look consistent with the existing functions to me.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]