mbox series

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

Message ID 20200917094455.822379-1-stefanha@redhat.com
Headers show
Series virtio: restore elem->in/out_sg after iov_discard_front/back() | expand

Message

Stefan Hajnoczi Sept. 17, 2020, 9:44 a.m. UTC 
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(-)

Comments

Stefan Hajnoczi Sept. 22, 2020, 7:59 a.m. UTC | #1 
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