| Message ID | 20200812104918.107116-1-stefanha@redhat.com |
|---|---|
| Headers | show |
| Series | virtio: restore elem->in/out_sg after iov_discard_front/back() | expand |
On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote: > Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道： Thanks for your review! > > + /* Discard more bytes than vector size */ > > + iov_random(&iov, &iov_cnt); > > + iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt); > > + iov_tmp = iov; > > + iov_cnt_tmp = iov_cnt; > > + size = iov_size(iov, iov_cnt); > > + iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size + 1, &undo); > > + iov_discard_undo(&undo); > > + assert(iov_equals(iov, iov_orig, iov_cnt)); > > > > The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not > touch 'iov_orig'. > So the test will be always passed. The actually function will not be tested. The test verifies that the iovec elements are restored to their previous state by iov_discard_undo(). I think you are saying you'd like iov_discard_undo() to reset the iov_tmp pointer? Currently that is not how the API works. The caller is assumed to have the original pointer (e.g. virtio-blk has req->elem.in/out_sg) and therefore it is not necessary to reset iov_tmp. > Also, maybe we could abstract a function to do these discard test? The structure of the test cases is similar but they vary in different places. I'm not sure if this can be abstracted nicely. > > diff --git a/util/iov.c b/util/iov.c > > index 45ef3043ee..efcf04b445 100644 > > --- a/util/iov.c > > +++ b/util/iov.c > > @@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const > > QEMUIOVector *src, void *buf) > > } > > } > > > > -size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt, > > - size_t bytes) > > +void iov_discard_undo(IOVDiscardUndo *undo) > > +{ > > + /* Restore original iovec if it was modified */ > > + if (undo->modified_iov) { > > + *undo->modified_iov = undo->orig; > > + } > > +} > > + > > +size_t iov_discard_front_undoable(struct iovec **iov, > > + unsigned int *iov_cnt, > > + size_t bytes, > > + IOVDiscardUndo *undo) > > { > > size_t total = 0; > > struct iovec *cur; > > > > + if (undo) { > > + undo->modified_iov = NULL; > > + } > > + > > for (cur = *iov; *iov_cnt > 0; cur++) { > > if (cur->iov_len > bytes) { > > + if (undo) { > > + undo->modified_iov = cur; > > + undo->orig = *cur; > > + } > > + > > > > Why here we remember the 'cur'? 'cur' is the some of the 'iov'. > Maybe we remember the 'iov'. I think we need the undo structure like this: > > struct IOVUndo { > iov **modified_iov; > iov *orig; > }; > > Then we can remeber the origin 'iov'. Yes, this could be done but it's not needed (yet?). VirtQueueElement has the original struct iovec pointers so adding this would be redundant.
Stefan Hajnoczi <stefanha@redhat.com> 于2020年9月16日周三 下午6:09写道： > > On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote: > > Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道： > > Thanks for your review! > > > > + /* Discard more bytes than vector size */ > > > + iov_random(&iov, &iov_cnt); > > > + iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt); > > > + iov_tmp = iov; > > > + iov_cnt_tmp = iov_cnt; > > > + size = iov_size(iov, iov_cnt); > > > + iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size + 1, &undo); > > > + iov_discard_undo(&undo); > > > + assert(iov_equals(iov, iov_orig, iov_cnt)); > > > > > > > The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not > > touch 'iov_orig'. > > So the test will be always passed. The actually function will not be tested. > > The test verifies that the iovec elements are restored to their previous > state by iov_discard_undo(). > > I think you are saying you'd like iov_discard_undo() to reset the > iov_tmp pointer? Currently that is not how the API works. The caller is > assumed to have the original pointer (e.g. virtio-blk has > req->elem.in/out_sg) and therefore it is not necessary to reset iov_tmp. > Hi Stefan, You're right, I just have the idea in my mind the the 'discard' function change the *iov, but the caller have the origin pointer. So Reviewed-by: Li Qiang <liq3ea@gmail.com> > > Also, maybe we could abstract a function to do these discard test? > > The structure of the test cases is similar but they vary in different > places. I'm not sure if this can be abstracted nicely. > > > > diff --git a/util/iov.c b/util/iov.c > > > index 45ef3043ee..efcf04b445 100644 > > > --- a/util/iov.c > > > +++ b/util/iov.c > > > @@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const > > > QEMUIOVector *src, void *buf) > > > } > > > } > > > > > > -size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt, > > > - size_t bytes) > > > +void iov_discard_undo(IOVDiscardUndo *undo) > > > +{ > > > + /* Restore original iovec if it was modified */ > > > + if (undo->modified_iov) { > > > + *undo->modified_iov = undo->orig; > > > + } > > > +} > > > + > > > +size_t iov_discard_front_undoable(struct iovec **iov, > > > + unsigned int *iov_cnt, > > > + size_t bytes, > > > + IOVDiscardUndo *undo) > > > { > > > size_t total = 0; > > > struct iovec *cur; > > > > > > + if (undo) { > > > + undo->modified_iov = NULL; > > > + } > > > + > > > for (cur = *iov; *iov_cnt > 0; cur++) { > > > if (cur->iov_len > bytes) { > > > + if (undo) { > > > + undo->modified_iov = cur; > > > + undo->orig = *cur; > > > + } > > > + > > > > > > > Why here we remember the 'cur'? 'cur' is the some of the 'iov'. > > Maybe we remember the 'iov'. I think we need the undo structure like this: > > > > struct IOVUndo { > > iov **modified_iov; > > iov *orig; > > }; > > > > Then we can remeber the origin 'iov'. > > Yes, this could be done but it's not needed (yet?). VirtQueueElement has > the original struct iovec pointers so adding this would be redundant.
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