[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/3] block: Fix NULL deference for unaligned wri
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-block] [PATCH 2/3] block: Fix NULL deference for unaligned write if qiov is NULL |
Date: |
Fri, 24 Apr 2015 11:01:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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.
>
> 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.
>
> 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.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
> block.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> 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(BlockDriverState *bs,
> uint64_t align = bdrv_get_align(bs);
> uint8_t *head_buf = NULL;
> uint8_t *tail_buf = NULL;
> + uint8_t *qiov_buf = NULL;
> QEMUIOVector local_qiov;
> bool use_local_qiov = false;
> int ret;
> @@ -3436,9 +3437,15 @@ static int coroutine_fn
> bdrv_co_do_pwritev(BlockDriverState *bs,
> }
> BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
>
> - 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 = qemu_blockalign(bs, bytes);
> + memset(qiov_buf, 0, bytes);
> + qemu_iovec_add(&local_qiov, qiov_buf, bytes);
> + }
> use_local_qiov = true;
>
> bytes += offset & (align - 1);
> @@ -3471,8 +3478,16 @@ static int coroutine_fn
> bdrv_co_do_pwritev(BlockDriverState *bs,
> BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
>
> 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 = qemu_blockalign(bs, bytes);
> + memset(qiov_buf, 0, bytes);
> + }
> + qemu_iovec_add(&local_qiov, qiov_buf, bytes);
> + }
Same here.
> use_local_qiov = true;
> }
>
> @@ -3498,6 +3513,7 @@ fail:
> }
> qemu_vfree(head_buf);
> qemu_vfree(tail_buf);
> + qemu_vfree(qiov_buf);
>
> return ret;
> }
>