From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34629) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlZUo-0002xh-8m for qemu-devel@nongnu.org; Fri, 24 Apr 2015 05:02:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlZUn-0006LV-5x for qemu-devel@nongnu.org; Fri, 24 Apr 2015 05:02:02 -0400 Message-ID: <553A067C.6010706@redhat.com> Date: Fri, 24 Apr 2015 11:01:48 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1429864436-17711-1-git-send-email-famz@redhat.com> <1429864436-17711-3-git-send-email-famz@redhat.com> In-Reply-To: <1429864436-17711-3-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/3] block: Fix NULL deference for unaligned write if qiov is NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, Stefan Hajnoczi , qemu-stable@nongnu.org On 24/04/2015 10:33, Fam Zheng wrote: > For zero write, qiov passed by callers (qemu-io "write -z" and > scsi-disk "write same") is NULL. >=20 > Commit fc3959e466 fixed bdrv_co_write_zeroes which is the common case > for this bug, but it still exists in bdrv_aio_write_zeroes. A simpler > fix would be in bdrv_co_do_pwritev which is the NULL dereference point > and covers both cases. >=20 > So don't access it in bdrv_co_do_pwritev, use a zeroed buffer instead. > Device model who calls into block layer should check the request size. >=20 > Signed-off-by: Fam Zheng > --- > block.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) >=20 > diff --git a/block.c b/block.c > index f2f8ae7..878a72d 100644 > --- a/block.c > +++ b/block.c > @@ -3386,6 +3386,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockD= riverState *bs, > uint64_t align =3D bdrv_get_align(bs); > uint8_t *head_buf =3D NULL; > uint8_t *tail_buf =3D NULL; > + uint8_t *qiov_buf =3D NULL; > QEMUIOVector local_qiov; > bool use_local_qiov =3D false; > int ret; > @@ -3436,9 +3437,15 @@ static int coroutine_fn bdrv_co_do_pwritev(Block= DriverState *bs, > } > BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); > =20 > - qemu_iovec_init(&local_qiov, qiov->niov + 2); > + qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 2 : 3); > qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1)); > - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + if (qiov) { > + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + } else { If we keep this approach, I would add an assertion that the flags include BDRV_REQ_ZERO_WRITE. Same below. However, the problem here is that you drop the BDRV_REQ_MAY_UNMAP flag for the central part of the write. I think if BDRV_REQ_ZERO_WRITE is set, you should do three bdrv_aligned_pwritev rather than just one. The first without BDRV_REQ_ZERO_WRITE and inside the if (offset & (align - 1)); the second with BDRV_REQ_ZERO_WRITE and with NULL qiov, right after that if; and the third is the one that exists already. After each write you modify offset and bytes. leaving > + qiov_buf =3D qemu_blockalign(bs, bytes); > + memset(qiov_buf, 0, bytes); > + qemu_iovec_add(&local_qiov, qiov_buf, bytes); > + } > use_local_qiov =3D true; > =20 > bytes +=3D offset & (align - 1); > @@ -3471,8 +3478,16 @@ static int coroutine_fn bdrv_co_do_pwritev(Block= DriverState *bs, > BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); > =20 > if (!use_local_qiov) { > - qemu_iovec_init(&local_qiov, qiov->niov + 1); > - qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 1 : 2); > + if (qiov) { > + qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size); > + } else { > + if (!qiov_buf) { > + qiov_buf =3D qemu_blockalign(bs, bytes); > + memset(qiov_buf, 0, bytes); > + } > + qemu_iovec_add(&local_qiov, qiov_buf, bytes); > + } Same here. > use_local_qiov =3D true; > } > =20 > @@ -3498,6 +3513,7 @@ fail: > } > qemu_vfree(head_buf); > qemu_vfree(tail_buf); > + qemu_vfree(qiov_buf); > =20 > return ret; > } >=20