[PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()

Stefan Hajnoczi posted 3 patches 3 years, 6 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200917094455.822379-1-stefanha@redhat.com
include/hw/virtio/virtio-blk.h |   2 +
include/qemu/iov.h             |  23 +++++
hw/block/virtio-blk.c          |  11 ++-
hw/virtio/virtio-crypto.c      |  17 +++-
tests/test-iov.c               | 165 +++++++++++++++++++++++++++++++++
util/iov.c                     |  50 +++++++++-
6 files changed, 259 insertions(+), 9 deletions(-)
[PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()
Posted by Stefan Hajnoczi 3 years, 6 months ago
v2:
 * Add missing undo in virtio-blk write zeroes error path [Li Qiang]

Both virtio-blk and virtio-crypto use destructive iov_discard_front/back()
operations on elem->in/out_sg. virtqueue_push() calls dma_memory_unmap() on the
modified iovec arrays. The memory addresses may not match those originally
mapped with dma_memory_map().

This raises several issues:
1. MemoryRegion references can be leaked.
2. Dirty memory may not be tracked.
3. The non-RAM bounce buffer can be leaked.

This patch series solves the issue in two ways:
1. virtio-blk uses a new iov_discard_undo() API to restore iovec arrays.
2. virtio-crypto uses g_memdup() to avoid modifying the original iovec arrays.

The g_memdup() approach is slower than iov_discard_undo() but less
complex/fragile. I am less familiar with the virtio-crypto code and it uses
more complex sequences of iov_discard_front/back() calls than virtio-blk. If
anyone feels like optimizing virtio-crypto, please go ahead.

The virtio-blk bug was found by Alexander Bulekov's fuzzing effort. I found the
virtio-crypto bug through code inspection.

Stefan Hajnoczi (3):
  util/iov: add iov_discard_undo()
  virtio-blk: undo destructive iov_discard_*() operations
  virtio-crypto: don't modify elem->in/out_sg

 include/hw/virtio/virtio-blk.h |   2 +
 include/qemu/iov.h             |  23 +++++
 hw/block/virtio-blk.c          |  11 ++-
 hw/virtio/virtio-crypto.c      |  17 +++-
 tests/test-iov.c               | 165 +++++++++++++++++++++++++++++++++
 util/iov.c                     |  50 +++++++++-
 6 files changed, 259 insertions(+), 9 deletions(-)

-- 
2.26.2

Re: [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()
Posted by Stefan Hajnoczi 3 years, 6 months ago
On Thu, Sep 17, 2020 at 10:44:52AM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Add missing undo in virtio-blk write zeroes error path [Li Qiang]
> 
> Both virtio-blk and virtio-crypto use destructive iov_discard_front/back()
> operations on elem->in/out_sg. virtqueue_push() calls dma_memory_unmap() on the
> modified iovec arrays. The memory addresses may not match those originally
> mapped with dma_memory_map().
> 
> This raises several issues:
> 1. MemoryRegion references can be leaked.
> 2. Dirty memory may not be tracked.
> 3. The non-RAM bounce buffer can be leaked.
> 
> This patch series solves the issue in two ways:
> 1. virtio-blk uses a new iov_discard_undo() API to restore iovec arrays.
> 2. virtio-crypto uses g_memdup() to avoid modifying the original iovec arrays.
> 
> The g_memdup() approach is slower than iov_discard_undo() but less
> complex/fragile. I am less familiar with the virtio-crypto code and it uses
> more complex sequences of iov_discard_front/back() calls than virtio-blk. If
> anyone feels like optimizing virtio-crypto, please go ahead.
> 
> The virtio-blk bug was found by Alexander Bulekov's fuzzing effort. I found the
> virtio-crypto bug through code inspection.
> 
> Stefan Hajnoczi (3):
>   util/iov: add iov_discard_undo()
>   virtio-blk: undo destructive iov_discard_*() operations
>   virtio-crypto: don't modify elem->in/out_sg
> 
>  include/hw/virtio/virtio-blk.h |   2 +
>  include/qemu/iov.h             |  23 +++++
>  hw/block/virtio-blk.c          |  11 ++-
>  hw/virtio/virtio-crypto.c      |  17 +++-
>  tests/test-iov.c               | 165 +++++++++++++++++++++++++++++++++
>  util/iov.c                     |  50 +++++++++-
>  6 files changed, 259 insertions(+), 9 deletions(-)
> 
> -- 
> 2.26.2
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan