* [PATCH 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() @ 2020-08-12 10:49 Stefan Hajnoczi 2020-08-12 10:49 ` [PATCH 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2020-08-12 10:49 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Max Reitz, Alexander Bulekov, Gonglei (Arei), Stefan Hajnoczi 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 t= he 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 t= he 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 | 9 +- hw/virtio/virtio-crypto.c | 17 +++- tests/test-iov.c | 165 +++++++++++++++++++++++++++++++++ util/iov.c | 50 +++++++++- 6 files changed, 257 insertions(+), 9 deletions(-) --=20 2.26.2 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] util/iov: add iov_discard_undo() 2020-08-12 10:49 [PATCH 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi @ 2020-08-12 10:49 ` Stefan Hajnoczi 2020-08-16 8:26 ` Li Qiang 2020-08-12 10:49 ` [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi 2020-08-12 10:49 ` [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg Stefan Hajnoczi 2 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2020-08-12 10:49 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, 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. 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 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; + } + 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] 11+ messages in thread
* Re: [PATCH 1/3] util/iov: add iov_discard_undo() 2020-08-12 10:49 ` [PATCH 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi @ 2020-08-16 8:26 ` Li Qiang 2020-09-16 10:09 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Li Qiang @ 2020-08-16 8:26 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Qemu Developers, Max Reitz, Alexander Bulekov, Gonglei (Arei) [-- Attachment #1: Type: text/plain, Size: 13011 bytes --] Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道: > 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. > > Seems there are some errors. See below > 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)); > 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. Also, maybe we could abstract a function to do these discard test? > + 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 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'. Thanks, Li Qiang > 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 > > [-- Attachment #2: Type: text/html, Size: 15923 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] util/iov: add iov_discard_undo() 2020-08-16 8:26 ` Li Qiang @ 2020-09-16 10:09 ` Stefan Hajnoczi 2020-09-16 15:36 ` Li Qiang 0 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2020-09-16 10:09 UTC (permalink / raw) To: Li Qiang Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Qemu Developers, Max Reitz, Alexander Bulekov, Gonglei (Arei) [-- Attachment #1: Type: text/plain, Size: 3020 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] util/iov: add iov_discard_undo() 2020-09-16 10:09 ` Stefan Hajnoczi @ 2020-09-16 15:36 ` Li Qiang 0 siblings, 0 replies; 11+ messages in thread From: Li Qiang @ 2020-09-16 15:36 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月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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations 2020-08-12 10:49 [PATCH 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi 2020-08-12 10:49 ` [PATCH 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi @ 2020-08-12 10:49 ` Stefan Hajnoczi 2020-09-16 15:38 ` Li Qiang 2020-08-12 10:49 ` [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg Stefan Hajnoczi 2 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2020-08-12 10:49 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 | 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); -- 2.26.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations 2020-08-12 10:49 ` [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi @ 2020-09-16 15:38 ` Li Qiang 2020-09-17 9:34 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Li Qiang @ 2020-09-16 15:38 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年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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations 2020-09-16 15:38 ` Li Qiang @ 2020-09-17 9:34 ` Stefan Hajnoczi 0 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2020-09-17 9:34 UTC (permalink / raw) To: Li Qiang Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Qemu Developers, Max Reitz, Alexander Bulekov, Gonglei (Arei) [-- Attachment #1: Type: text/plain, Size: 914 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg 2020-08-12 10:49 [PATCH 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi 2020-08-12 10:49 ` [PATCH 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi 2020-08-12 10:49 ` [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi @ 2020-08-12 10:49 ` Stefan Hajnoczi 2020-08-16 8:32 ` Li Qiang 2 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2020-08-12 10:49 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, 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. 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] 11+ messages in thread
* Re: [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg 2020-08-12 10:49 ` [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg Stefan Hajnoczi @ 2020-08-16 8:32 ` Li Qiang 2020-09-16 10:12 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Li Qiang @ 2020-08-16 8:32 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Qemu Developers, Max Reitz, Alexander Bulekov, Gonglei (Arei) [-- Attachment #1: Type: text/plain, Size: 3156 bytes --] Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道: > 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. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > virtio-net also uses this method. Reviewed-by: Li Qiang <liq3ea@gmail.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 > > [-- Attachment #2: Type: text/html, Size: 4124 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg 2020-08-16 8:32 ` Li Qiang @ 2020-09-16 10:12 ` Stefan Hajnoczi 0 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2020-09-16 10:12 UTC (permalink / raw) To: Li Qiang Cc: Kevin Wolf, qemu-block, Michael S. Tsirkin, Qemu Developers, Max Reitz, Alexander Bulekov, Gonglei (Arei) [-- Attachment #1: Type: text/plain, Size: 1027 bytes --] On Sun, Aug 16, 2020 at 04:32:06PM +0800, Li Qiang wrote: > Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:51写道: > > > 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. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > virtio-net also uses this method. virtio-net operates on a copy of the iovecs (g_memdup()) so no changes are necessary. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-09-17 9:36 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-12 10:49 [PATCH 0/3] virtio: restore elem->in/out_sg after iov_discard_front/back() Stefan Hajnoczi 2020-08-12 10:49 ` [PATCH 1/3] util/iov: add iov_discard_undo() Stefan Hajnoczi 2020-08-16 8:26 ` Li Qiang 2020-09-16 10:09 ` Stefan Hajnoczi 2020-09-16 15:36 ` Li Qiang 2020-08-12 10:49 ` [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations Stefan Hajnoczi 2020-09-16 15:38 ` Li Qiang 2020-09-17 9:34 ` Stefan Hajnoczi 2020-08-12 10:49 ` [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg Stefan Hajnoczi 2020-08-16 8:32 ` Li Qiang 2020-09-16 10:12 ` 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.