* [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()
@ 2020-09-17 9:44 Stefan Hajnoczi
2020-09-17 9:44 ` [PATCH v2 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-09-17 9:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Max Reitz,
Alexander Bulekov, Gonglei (Arei),
Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/3] util/iov: add iov_discard_undo()
2020-09-17 9:44 [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
@ 2020-09-17 9:44 ` Stefan Hajnoczi
2020-09-17 9:44 ` [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-09-17 9:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Li Qiang, Max Reitz,
Alexander Bulekov, Gonglei (Arei),
Stefan Hajnoczi
The iov_discard_front/back() operations are useful for parsing iovecs
but they modify the array elements. If the original array is needed
after parsing finishes there is currently no way to restore it.
Although g_memdup() can be used before performing destructive
iov_discard_front/back() operations, this is inefficient.
Introduce iov_discard_undo() to restore the array to the state prior to
an iov_discard_front/back() operation.
Reviewed-by: Li Qiang <liq3ea@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/qemu/iov.h | 23 +++++++
tests/test-iov.c | 165 +++++++++++++++++++++++++++++++++++++++++++++
util/iov.c | 50 ++++++++++++--
3 files changed, 234 insertions(+), 4 deletions(-)
diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index bffc151282..b6b283a5e5 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -130,6 +130,29 @@ size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
size_t bytes);
+/* Information needed to undo an iov_discard_*() operation */
+typedef struct {
+ struct iovec *modified_iov;
+ struct iovec orig;
+} IOVDiscardUndo;
+
+/*
+ * Undo an iov_discard_front_undoable() or iov_discard_back_undoable()
+ * operation. If multiple operations are made then each one needs a separate
+ * IOVDiscardUndo and iov_discard_undo() must be called in the reverse order
+ * that the operations were made.
+ */
+void iov_discard_undo(IOVDiscardUndo *undo);
+
+/*
+ * Undoable versions of iov_discard_front() and iov_discard_back(). Use
+ * iov_discard_undo() to reset to the state before the discard operations.
+ */
+size_t iov_discard_front_undoable(struct iovec **iov, unsigned int *iov_cnt,
+ size_t bytes, IOVDiscardUndo *undo);
+size_t iov_discard_back_undoable(struct iovec *iov, unsigned int *iov_cnt,
+ size_t bytes, IOVDiscardUndo *undo);
+
typedef struct QEMUIOVector {
struct iovec *iov;
int niov;
diff --git a/tests/test-iov.c b/tests/test-iov.c
index 458ca25099..9c415e2f1f 100644
--- a/tests/test-iov.c
+++ b/tests/test-iov.c
@@ -26,6 +26,12 @@ static void iov_free(struct iovec *iov, unsigned niov)
g_free(iov);
}
+static bool iov_equals(const struct iovec *a, const struct iovec *b,
+ unsigned niov)
+{
+ return memcmp(a, b, sizeof(a[0]) * niov) == 0;
+}
+
static void test_iov_bytes(struct iovec *iov, unsigned niov,
size_t offset, size_t bytes)
{
@@ -335,6 +341,87 @@ static void test_discard_front(void)
iov_free(iov, iov_cnt);
}
+static void test_discard_front_undo(void)
+{
+ IOVDiscardUndo undo;
+ struct iovec *iov;
+ struct iovec *iov_tmp;
+ struct iovec *iov_orig;
+ unsigned int iov_cnt;
+ unsigned int iov_cnt_tmp;
+ size_t size;
+
+ /* Discard zero bytes */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_tmp = iov;
+ iov_cnt_tmp = iov_cnt;
+ iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, 0, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* 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));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard entire vector */
+ 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, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard within first element */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_tmp = iov;
+ iov_cnt_tmp = iov_cnt;
+ size = g_test_rand_int_range(1, iov->iov_len);
+ iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard entire first element */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_tmp = iov;
+ iov_cnt_tmp = iov_cnt;
+ iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, iov->iov_len, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard within second element */
+ 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->iov_len + g_test_rand_int_range(1, iov[1].iov_len);
+ iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+}
+
static void test_discard_back(void)
{
struct iovec *iov;
@@ -404,6 +491,82 @@ static void test_discard_back(void)
iov_free(iov, iov_cnt);
}
+static void test_discard_back_undo(void)
+{
+ IOVDiscardUndo undo;
+ struct iovec *iov;
+ struct iovec *iov_orig;
+ unsigned int iov_cnt;
+ unsigned int iov_cnt_tmp;
+ size_t size;
+
+ /* Discard zero bytes */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_cnt_tmp = iov_cnt;
+ iov_discard_back_undoable(iov, &iov_cnt_tmp, 0, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard more bytes than vector size */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_cnt_tmp = iov_cnt;
+ size = iov_size(iov, iov_cnt);
+ iov_discard_back_undoable(iov, &iov_cnt_tmp, size + 1, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard entire vector */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_cnt_tmp = iov_cnt;
+ size = iov_size(iov, iov_cnt);
+ iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard within last element */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_cnt_tmp = iov_cnt;
+ size = g_test_rand_int_range(1, iov[iov_cnt - 1].iov_len);
+ iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard entire last element */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_cnt_tmp = iov_cnt;
+ size = iov[iov_cnt - 1].iov_len;
+ iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+
+ /* Discard within second-to-last element */
+ iov_random(&iov, &iov_cnt);
+ iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_cnt_tmp = iov_cnt;
+ size = iov[iov_cnt - 1].iov_len +
+ g_test_rand_int_range(1, iov[iov_cnt - 2].iov_len);
+ iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+ iov_discard_undo(&undo);
+ assert(iov_equals(iov, iov_orig, iov_cnt));
+ g_free(iov_orig);
+ iov_free(iov, iov_cnt);
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -412,5 +575,7 @@ int main(int argc, char **argv)
g_test_add_func("/basic/iov/io", test_io);
g_test_add_func("/basic/iov/discard-front", test_discard_front);
g_test_add_func("/basic/iov/discard-back", test_discard_back);
+ g_test_add_func("/basic/iov/discard-front-undo", test_discard_front_undo);
+ g_test_add_func("/basic/iov/discard-back-undo", test_discard_back_undo);
return g_test_run();
}
diff --git a/util/iov.c b/util/iov.c
index ae61d696aa..f3a9e92a37 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;
+ }
+
cur->iov_base += bytes;
cur->iov_len -= bytes;
total += bytes;
@@ -659,12 +678,24 @@ size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
return total;
}
-size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
- size_t bytes)
+size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
+ size_t bytes)
+{
+ return iov_discard_front_undoable(iov, iov_cnt, bytes, NULL);
+}
+
+size_t iov_discard_back_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;
+ }
+
if (*iov_cnt == 0) {
return 0;
}
@@ -673,6 +704,11 @@ size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
while (*iov_cnt > 0) {
if (cur->iov_len > bytes) {
+ if (undo) {
+ undo->modified_iov = cur;
+ undo->orig = *cur;
+ }
+
cur->iov_len -= bytes;
total += bytes;
break;
@@ -687,6 +723,12 @@ size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
return total;
}
+size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
+ size_t bytes)
+{
+ return iov_discard_back_undoable(iov, iov_cnt, bytes, NULL);
+}
+
void qemu_iovec_discard_back(QEMUIOVector *qiov, size_t bytes)
{
size_t total;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations
2020-09-17 9:44 [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
2020-09-17 9:44 ` [PATCH v2 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi
@ 2020-09-17 9:44 ` Stefan Hajnoczi
2020-09-17 14:54 ` Li Qiang
2020-09-17 9:44 ` [PATCH v2 3/3] virtio-crypto: don't modify elem->in/out_sg Stefan Hajnoczi
2020-09-22 7:59 ` [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
3 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-09-17 9:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Max Reitz,
Alexander Bulekov, Gonglei (Arei),
Stefan Hajnoczi
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 | 11 +++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 29c9f32353..df3876d49c 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -72,6 +72,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 2204ba149e..bac2d6fa2b 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);
@@ -739,6 +744,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
+ iov_discard_undo(&req->inhdr_undo);
+ iov_discard_undo(&req->outhdr_undo);
virtio_error(vdev, "virtio-blk discard/write_zeroes header"
" too short");
return -1;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] virtio-crypto: don't modify elem->in/out_sg
2020-09-17 9:44 [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
2020-09-17 9:44 ` [PATCH v2 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi
2020-09-17 9:44 ` [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi
@ 2020-09-17 9:44 ` Stefan Hajnoczi
2020-09-22 7:59 ` [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
3 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-09-17 9:44 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Li Qiang, Max Reitz,
Alexander Bulekov, Gonglei (Arei),
Stefan Hajnoczi
A number of iov_discard_front/back() operations are made by
virtio-crypto. The elem->in/out_sg iovec arrays are modified by these
operations, resulting virtqueue_unmap_sg() calls on different addresses
than were originally mapped.
This is problematic because dirty memory may not be logged correctly,
MemoryRegion refcounts may be leaked, and the non-RAM bounce buffer can
be leaked.
Take a copy of the elem->in/out_sg arrays so that the originals are
preserved. The iov_discard_undo() API could be used instead (with better
performance) but requires careful auditing of the code, so do the simple
thing instead.
Reviewed-by: Li Qiang <liq3ea@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/virtio/virtio-crypto.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 6da12e315f..54f9bbb789 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -228,6 +228,8 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
size_t s;
for (;;) {
+ g_autofree struct iovec *out_iov_copy = NULL;
+
elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
if (!elem) {
break;
@@ -240,9 +242,12 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
}
out_num = elem->out_num;
- out_iov = elem->out_sg;
+ out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+ out_iov = out_iov_copy;
+
in_num = elem->in_num;
in_iov = elem->in_sg;
+
if (unlikely(iov_to_buf(out_iov, out_num, 0, &ctrl, sizeof(ctrl))
!= sizeof(ctrl))) {
virtio_error(vdev, "virtio-crypto request ctrl_hdr too short");
@@ -582,6 +587,8 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
int queue_index = virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
struct virtio_crypto_op_data_req req;
int ret;
+ g_autofree struct iovec *in_iov_copy = NULL;
+ g_autofree struct iovec *out_iov_copy = NULL;
struct iovec *in_iov;
struct iovec *out_iov;
unsigned in_num;
@@ -598,9 +605,13 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
}
out_num = elem->out_num;
- out_iov = elem->out_sg;
+ out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+ out_iov = out_iov_copy;
+
in_num = elem->in_num;
- in_iov = elem->in_sg;
+ in_iov_copy = g_memdup(elem->in_sg, sizeof(in_iov[0]) * in_num);
+ in_iov = in_iov_copy;
+
if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
!= sizeof(req))) {
virtio_error(vdev, "virtio-crypto request outhdr too short");
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations
2020-09-17 9:44 ` [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi
@ 2020-09-17 14:54 ` Li Qiang
0 siblings, 0 replies; 6+ messages in thread
From: Li Qiang @ 2020-09-17 14:54 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Qemu Developers,
Max Reitz, Alexander Bulekov, Gonglei (Arei)
Stefan Hajnoczi <stefanha@redhat.com> 于2020年9月17日周四 下午5:47写道：
>
> 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>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
> ---
> include/hw/virtio/virtio-blk.h | 2 ++
> hw/block/virtio-blk.c | 11 +++++++++--
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 29c9f32353..df3876d49c 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -72,6 +72,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 2204ba149e..bac2d6fa2b 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);
>
> @@ -739,6 +744,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>
> if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
> sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
> + iov_discard_undo(&req->inhdr_undo);
> + iov_discard_undo(&req->outhdr_undo);
> virtio_error(vdev, "virtio-blk discard/write_zeroes header"
> " too short");
> return -1;
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back()
2020-09-17 9:44 [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
` (2 preceding siblings ...)
2020-09-17 9:44 ` [PATCH v2 3/3] virtio-crypto: don't modify elem->in/out_sg Stefan Hajnoczi
@ 2020-09-22 7:59 ` Stefan Hajnoczi
3 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 7:59 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Max Reitz,
Alexander Bulekov, Gonglei (Arei)
[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-22 8:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 9:44 [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
2020-09-17 9:44 ` [PATCH v2 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi
2020-09-17 9:44 ` [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi
2020-09-17 14:54 ` Li Qiang
2020-09-17 9:44 ` [PATCH v2 3/3] virtio-crypto: don't modify elem->in/out_sg Stefan Hajnoczi
2020-09-22 7:59 ` [PATCH v2 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.