| Message ID | 20200812104918.107116-3-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: restore elem->in/out_sg after iov_discard_front/back() | expand |
Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道： > > Fuzzing discovered that virtqueue_unmap_sg() is being called on modified > req->in/out_sg iovecs. This means dma_memory_map() and > dma_memory_unmap() calls do not have matching memory addresses. > > Fuzzing discovered that non-RAM addresses trigger a bug: > > void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len, > bool is_write, hwaddr access_len) > { > if (buffer != bounce.buffer) { > ^^^^^^^^^^^^^^^^^^^^^^^ > > A modified iov->iov_base is no longer recognized as a bounce buffer and > the wrong branch is taken. > > There are more potential bugs: dirty memory is not tracked correctly and > MemoryRegion refcounts can be leaked. > > Use the new iov_discard_undo() API to restore elem->in/out_sg before > virtqueue_push() is called. > > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Buglink: https://bugs.launchpad.net/qemu/+bug/1890360 > Fixes: 827805a2492c1bbf1c0712ed18ee069b4ebf3dd6 ("virtio-blk: Convert VirtIOBlockReq.out to structrue") > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > include/hw/virtio/virtio-blk.h | 2 ++ > hw/block/virtio-blk.c | 9 +++++++-- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index b1334c3904..0af654cace 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -68,6 +68,8 @@ typedef struct VirtIOBlockReq { > int64_t sector_num; > VirtIOBlock *dev; > VirtQueue *vq; > + IOVDiscardUndo inhdr_undo; > + IOVDiscardUndo outhdr_undo; > struct virtio_blk_inhdr *in; > struct virtio_blk_outhdr out; > QEMUIOVector qiov; > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 413783693c..2b7cc3e1c8 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -80,6 +80,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status) > trace_virtio_blk_req_complete(vdev, req, status); > > stb_p(&req->in->status, status); > + iov_discard_undo(&req->inhdr_undo); > + iov_discard_undo(&req->outhdr_undo); > virtqueue_push(req->vq, &req->elem, req->in_len); > if (s->dataplane_started && !s->dataplane_disabled) { > virtio_blk_data_plane_notify(s->dataplane, req->vq); > @@ -632,10 +634,12 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > return -1; > } > > - iov_discard_front(&out_iov, &out_num, sizeof(req->out)); > + iov_discard_front_undoable(&out_iov, &out_num, sizeof(req->out), > + &req->outhdr_undo); > > if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) { > virtio_error(vdev, "virtio-blk request inhdr too short"); > + iov_discard_undo(&req->outhdr_undo); > return -1; > } > > @@ -644,7 +648,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > req->in = (void *)in_iov[in_num - 1].iov_base > + in_iov[in_num - 1].iov_len > - sizeof(struct virtio_blk_inhdr); > - iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr)); > + iov_discard_back_undoable(in_iov, &in_num, sizeof(struct virtio_blk_inhdr), > + &req->inhdr_undo); > > type = virtio_ldl_p(vdev, &req->out.type); > It seems there is another error path need to do the undo operations. case VIRTIO_BLK_T_WRITE_ZEROS & ~VIRTIO_BLK_T_OUT handler has an error path. Thanks, Li Qiang > -- > 2.26.2 >
On Wed, Sep 16, 2020 at 11:38:36PM +0800, Li Qiang wrote: > Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道： > > @@ -644,7 +648,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > req->in = (void *)in_iov[in_num - 1].iov_base > > + in_iov[in_num - 1].iov_len > > - sizeof(struct virtio_blk_inhdr); > > - iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr)); > > + iov_discard_back_undoable(in_iov, &in_num, sizeof(struct virtio_blk_inhdr), > > + &req->inhdr_undo); > > > > type = virtio_ldl_p(vdev, &req->out.type); > > > > It seems there is another error path need to do the undo operations. > case VIRTIO_BLK_T_WRITE_ZEROS & ~VIRTIO_BLK_T_OUT > handler has an error path. Yes, thank you! I'll fix it in the next revision. Stefan
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index b1334c3904..0af654cace 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -68,6 +68,8 @@ typedef struct VirtIOBlockReq { int64_t sector_num; VirtIOBlock *dev; VirtQueue *vq; + IOVDiscardUndo inhdr_undo; + IOVDiscardUndo outhdr_undo; struct virtio_blk_inhdr *in; struct virtio_blk_outhdr out; QEMUIOVector qiov; diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 413783693c..2b7cc3e1c8 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -80,6 +80,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status) trace_virtio_blk_req_complete(vdev, req, status); stb_p(&req->in->status, status); + iov_discard_undo(&req->inhdr_undo); + iov_discard_undo(&req->outhdr_undo); virtqueue_push(req->vq, &req->elem, req->in_len); if (s->dataplane_started && !s->dataplane_disabled) { virtio_blk_data_plane_notify(s->dataplane, req->vq); @@ -632,10 +634,12 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) return -1; } - iov_discard_front(&out_iov, &out_num, sizeof(req->out)); + iov_discard_front_undoable(&out_iov, &out_num, sizeof(req->out), + &req->outhdr_undo); if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) { virtio_error(vdev, "virtio-blk request inhdr too short"); + iov_discard_undo(&req->outhdr_undo); return -1; } @@ -644,7 +648,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) req->in = (void *)in_iov[in_num - 1].iov_base + in_iov[in_num - 1].iov_len - sizeof(struct virtio_blk_inhdr); - iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr)); + iov_discard_back_undoable(in_iov, &in_num, sizeof(struct virtio_blk_inhdr), + &req->inhdr_undo); type = virtio_ldl_p(vdev, &req->out.type);
Fuzzing discovered that virtqueue_unmap_sg() is being called on modified req->in/out_sg iovecs. This means dma_memory_map() and dma_memory_unmap() calls do not have matching memory addresses. Fuzzing discovered that non-RAM addresses trigger a bug: void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len, bool is_write, hwaddr access_len) { if (buffer != bounce.buffer) { ^^^^^^^^^^^^^^^^^^^^^^^ A modified iov->iov_base is no longer recognized as a bounce buffer and the wrong branch is taken. There are more potential bugs: dirty memory is not tracked correctly and MemoryRegion refcounts can be leaked. Use the new iov_discard_undo() API to restore elem->in/out_sg before virtqueue_push() is called. Reported-by: Alexander Bulekov <alxndr@bu.edu> Buglink: https://bugs.launchpad.net/qemu/+bug/1890360 Fixes: 827805a2492c1bbf1c0712ed18ee069b4ebf3dd6 ("virtio-blk: Convert VirtIOBlockReq.out to structrue") Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/hw/virtio/virtio-blk.h | 2 ++ hw/block/virtio-blk.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)