Re: [Qemu-block] [PATCH v2 2/2] block: Fix NULL deference for unaligned write if qiov is NULL

[Paolo Bonzini]

On 24/04/2015 12: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 in this case, use three aligned
> writes.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c | 78 
> +++++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0fe97de..cbd0708 100644
> --- a/block.c
> +++ b/block.c
> @@ -3403,6 +3403,8 @@ static int coroutine_fn 
> bdrv_co_do_pwritev(BlockDriverState *bs,
>       */
>      tracked_request_begin(&req, bs, offset, bytes, true);
>  
> +    assert(qiov || flags & BDRV_REQ_ZERO_WRITE);

Perhaps as a follow-up you can add

if ((flags & (BDRV_REQ_ZERO_WRITE|BDRV_REQ_MAY_UNMAP))
    == BDRV_REQ_ZERO_WRITE|BDRV_REQ_MAY_UNMAP) {
    qiov = NULL;
}

so that the central area is always unmapped.  You can have non-NULL qiov
if the flags were added because of detect-zeroes=unmap.

But in any case that would be a separate change.

>      if (offset & (align - 1)) {
>          QEMUIOVector head_qiov;
>          struct iovec head_iov;
> @@ -3425,13 +3427,37 @@ 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_add(&local_qiov, head_buf, offset & (align - 1));
> -        qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> -        use_local_qiov = true;
> +        if (qiov) {
> +            qemu_iovec_init(&local_qiov, qiov ? qiov->niov + 2 : 1);
> +            qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
> +            qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> +            use_local_qiov = true;
> +            bytes += offset & (align - 1);
> +            offset = offset & ~(align - 1);
> +        } else {
> +            memset(head_buf + (offset & (align - 1)), 0,
> +                   align - (offset & (align - 1)));
> +            ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), 
> align,
> +                                       &head_qiov, 0);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +            bytes -= align - (offset & (align - 1));
> +            offset = ROUND_UP(offset, align);
> +        }
> +    }
>  
> -        bytes += offset & (align - 1);
> -        offset = offset & ~(align - 1);
> +    if (!qiov) {
> +        uint64_t aligned_bytes = bytes & ~(align - 1);
> +
> +        assert((offset & (align - 1)) == 0);
> +        ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes,
> +                                   NULL, flags);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        bytes -= aligned_bytes;
> +        offset += aligned_bytes;
>      }
>  
>      if ((offset + bytes) & (align - 1)) {
> @@ -3459,21 +3485,39 @@ 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);
> -            use_local_qiov = true;
> +        if (qiov) {
> +            if (!use_local_qiov) {
> +                qemu_iovec_init(&local_qiov, qiov->niov + 1);
> +                qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> +                use_local_qiov = true;
> +            }
> +
> +            tail_bytes = (offset + bytes) & (align - 1);
> +            qemu_iovec_add(&local_qiov, tail_buf + tail_bytes,
> +                           align - tail_bytes);
> +
> +            bytes = ROUND_UP(bytes, align);
> +        } else {
> +            assert((offset & (align - 1)) == 0);
> +            assert(bytes < align);
> +
> +            memset(tail_buf, 0, bytes & (align - 1));
> +            ret = bdrv_aligned_pwritev(bs, &req, offset, align,
> +                                       &tail_qiov, 0);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +            offset += align;
> +            bytes = 0;
>          }
>  
> -        tail_bytes = (offset + bytes) & (align - 1);
> -        qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - 
> tail_bytes);
> -
> -        bytes = ROUND_UP(bytes, align);
>      }
>  
> -    ret = bdrv_aligned_pwritev(bs, &req, offset, bytes,
> -                               use_local_qiov ? &local_qiov : qiov,
> -                               flags);
> +    if (bytes) {
> +        ret = bdrv_aligned_pwritev(bs, &req, offset, bytes,
> +                                   use_local_qiov ? &local_qiov : qiov,
> +                                   flags);
> +    }
>  
>  fail:
>      tracked_request_end(&req);
> 

Reviewed-by: Paolo Bonzini <address@hidden>