[Qemu-devel] [PATCH 0/4] block: Fix qiov sizes

[Qemu-devel] [PATCH 0/4] block: Fix qiov sizes

[Kevin Wolf]

Some callers use larger qiovs than nb_sectors for their read requests. This is
bad because raw-posix uses nb_sectors to allocate a bounce buffer and then
copies the whole iov over.

This series fixes some more cases (mirroring was already fixed earlier this
week) and adds assertions to catch any other offenders.

Kevin Wolf (4):
  block: Make qiov match the request size until EOF
  qcow2: Make qiov match request size until backing file EOF
  qed: Make qiov match request size until backing file EOF
  block: Assert qiov length matches request length

 block.c           | 13 ++++++++++++-
 block/qcow2.c     | 11 ++++++++++-
 block/qed.c       | 26 +++++++++++++++++++++++---
 block/qed.h       |  1 +
 block/raw-posix.c | 15 +++++++++++----
 5 files changed, 57 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] block: Make qiov match the request size until EOF

[Kevin Wolf]

If a read request goes across EOF, the block driver sees a shortened
request that stops at EOF (the rest is memsetted in block.c), however
the original qiov was used for this request.

This patch makes the qiov size match the request size, avoiding a
potential buffer overflow in raw-posix.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 159a31e..ac4a037 100644
--- a/block.c
+++ b/block.c
@@ -3060,8 +3060,17 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
         max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num),
                                   align >> BDRV_SECTOR_BITS);
         if (max_nb_sectors > 0) {
+            QEMUIOVector local_qiov;
+
+            qemu_iovec_init(&local_qiov, qiov->niov);
+            qemu_iovec_concat(&local_qiov, qiov, 0,
+                              max_nb_sectors * BDRV_SECTOR_SIZE);
+
             ret = drv->bdrv_co_readv(bs, sector_num,
-                                     MIN(nb_sectors, max_nb_sectors), qiov);
+                                     MIN(nb_sectors, max_nb_sectors),
+                                     &local_qiov);
+
+            qemu_iovec_destroy(&local_qiov);
         } else {
             ret = 0;
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] qcow2: Make qiov match request size until backing file EOF

[Kevin Wolf]

If a qcow2 image has a shorter backing file and a read request to
unallocated clusters goes across EOF of the backing file, the backing
file sees a shortened request and the rest is filled with zeros.
However, the original too long qiov was used with the shortened request.

This patch makes the qiov size match the request size, avoiding a
potential buffer overflow in raw-posix.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 67e55c9..b0faa69 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1020,11 +1020,20 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num,
                 n1 = qcow2_backing_read1(bs->backing_hd, &hd_qiov,
                     sector_num, cur_nr_sectors);
                 if (n1 > 0) {
+                    QEMUIOVector local_qiov;
+
+                    qemu_iovec_init(&local_qiov, hd_qiov.niov);
+                    qemu_iovec_concat(&local_qiov, &hd_qiov, 0,
+                                      n1 * BDRV_SECTOR_SIZE);
+
                     BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
                     qemu_co_mutex_unlock(&s->lock);
                     ret = bdrv_co_readv(bs->backing_hd, sector_num,
-                                        n1, &hd_qiov);
+                                        n1, &local_qiov);
                     qemu_co_mutex_lock(&s->lock);
+
+                    qemu_iovec_destroy(&local_qiov);
+
                     if (ret < 0) {
                         goto fail;
                     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] qed: Make qiov match request size until backing file EOF

[Kevin Wolf]

If a QED image has a shorter backing file and a read request to
unallocated clusters goes across EOF of the backing file, the backing
file sees a shortened request and the rest is filled with zeros.
However, the original too long qiov was used with the shortened request.

This patch makes the qiov size match the request size, avoiding a
potential buffer overflow in raw-posix.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 26 +++++++++++++++++++++++---
 block/qed.h |  1 +
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index b69374b..1f63b8f 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -772,6 +772,7 @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
  */
 static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
                                   QEMUIOVector *qiov,
+                                  QEMUIOVector **backing_qiov,
                                   BlockDriverCompletionFunc *cb, void *opaque)
 {
     uint64_t backing_length = 0;
@@ -804,15 +805,20 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     /* If the read straddles the end of the backing file, shorten it */
     size = MIN((uint64_t)backing_length - pos, qiov->size);
 
+    *backing_qiov = g_new(QEMUIOVector, 1);
+    qemu_iovec_init(*backing_qiov, qiov->niov);
+    qemu_iovec_concat(*backing_qiov, qiov, 0, size);
+
     BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO);
     bdrv_aio_readv(s->bs->backing_hd, pos / BDRV_SECTOR_SIZE,
-                   qiov, size / BDRV_SECTOR_SIZE, cb, opaque);
+                   *backing_qiov, size / BDRV_SECTOR_SIZE, cb, opaque);
 }
 
 typedef struct {
     GenericCB gencb;
     BDRVQEDState *s;
     QEMUIOVector qiov;
+    QEMUIOVector *backing_qiov;
     struct iovec iov;
     uint64_t offset;
 } CopyFromBackingFileCB;
@@ -829,6 +835,12 @@ static void qed_copy_from_backing_file_write(void *opaque, int ret)
     CopyFromBackingFileCB *copy_cb = opaque;
     BDRVQEDState *s = copy_cb->s;
 
+    if (copy_cb->backing_qiov) {
+        qemu_iovec_destroy(copy_cb->backing_qiov);
+        g_free(copy_cb->backing_qiov);
+        copy_cb->backing_qiov = NULL;
+    }
+
     if (ret) {
         qed_copy_from_backing_file_cb(copy_cb, ret);
         return;
@@ -866,11 +878,12 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
     copy_cb = gencb_alloc(sizeof(*copy_cb), cb, opaque);
     copy_cb->s = s;
     copy_cb->offset = offset;
+    copy_cb->backing_qiov = NULL;
     copy_cb->iov.iov_base = qemu_blockalign(s->bs, len);
     copy_cb->iov.iov_len = len;
     qemu_iovec_init_external(&copy_cb->qiov, &copy_cb->iov, 1);
 
-    qed_read_backing_file(s, pos, &copy_cb->qiov,
+    qed_read_backing_file(s, pos, &copy_cb->qiov, &copy_cb->backing_qiov,
                           qed_copy_from_backing_file_write, copy_cb);
 }
 
@@ -1313,7 +1326,7 @@ static void qed_aio_read_data(void *opaque, int ret,
         return;
     } else if (ret != QED_CLUSTER_FOUND) {
         qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov,
-                              qed_aio_next_io, acb);
+                              &acb->backing_qiov, qed_aio_next_io, acb);
         return;
     }
 
@@ -1339,6 +1352,12 @@ static void qed_aio_next_io(void *opaque, int ret)
 
     trace_qed_aio_next_io(s, acb, ret, acb->cur_pos + acb->cur_qiov.size);
 
+    if (acb->backing_qiov) {
+        qemu_iovec_destroy(acb->backing_qiov);
+        g_free(acb->backing_qiov);
+        acb->backing_qiov = NULL;
+    }
+
     /* Handle I/O error */
     if (ret) {
         qed_aio_complete(acb, ret);
@@ -1378,6 +1397,7 @@ static BlockDriverAIOCB *qed_aio_setup(BlockDriverState *bs,
     acb->qiov_offset = 0;
     acb->cur_pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE;
     acb->end_pos = acb->cur_pos + nb_sectors * BDRV_SECTOR_SIZE;
+    acb->backing_qiov = NULL;
     acb->request.l2_table = NULL;
     qemu_iovec_init(&acb->cur_qiov, qiov->niov);
 
diff --git a/block/qed.h b/block/qed.h
index b024751..2b0e724 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -142,6 +142,7 @@ typedef struct QEDAIOCB {
 
     /* Current cluster scatter-gather list */
     QEMUIOVector cur_qiov;
+    QEMUIOVector *backing_qiov;
     uint64_t cur_pos;               /* position on block device, in bytes */
     uint64_t cur_cluster;           /* cluster offset in image file */
     unsigned int cur_nclusters;     /* number of clusters being accessed */
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] block: Assert qiov length matches request length

[Kevin Wolf]

At least raw-posix relies on this because it can allocate bounce buffers
based on the request length, but access it using all of the qiov entries
later.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c           |  2 ++
 block/raw-posix.c | 15 +++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index ac4a037..0d1795f 100644
--- a/block.c
+++ b/block.c
@@ -3016,6 +3016,7 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
 
     assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(!qiov || bytes == qiov->size);
 
     /* Handle Copy on Read and associated serialisation */
     if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -3282,6 +3283,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
 
     assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
     assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(!qiov || bytes == qiov->size);
 
     waited = wait_serialising_requests(req);
     assert(!waited || !req->serialising);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 825a0c8..b21ec57 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -790,6 +790,7 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
             memcpy(p, aiocb->aio_iov[i].iov_base, aiocb->aio_iov[i].iov_len);
             p += aiocb->aio_iov[i].iov_len;
         }
+        assert(p - buf == aiocb->aio_nbytes);
     }
 
     nbytes = handle_aiocb_rw_linear(aiocb, buf);
@@ -804,9 +805,11 @@ static ssize_t handle_aiocb_rw(RawPosixAIOData *aiocb)
                 copy = aiocb->aio_iov[i].iov_len;
             }
             memcpy(aiocb->aio_iov[i].iov_base, p, copy);
+            assert(count >= copy);
             p     += copy;
             count -= copy;
         }
+        assert(count == 0);
     }
     qemu_vfree(buf);
 
@@ -993,12 +996,14 @@ static int paio_submit_co(BlockDriverState *bs, int fd,
     acb->aio_type = type;
     acb->aio_fildes = fd;
 
+    acb->aio_nbytes = nb_sectors * BDRV_SECTOR_SIZE;
+    acb->aio_offset = sector_num * BDRV_SECTOR_SIZE;
+
     if (qiov) {
         acb->aio_iov = qiov->iov;
         acb->aio_niov = qiov->niov;
+        assert(qiov->size == acb->aio_nbytes);
     }
-    acb->aio_nbytes = nb_sectors * 512;
-    acb->aio_offset = sector_num * 512;
 
     trace_paio_submit_co(sector_num, nb_sectors, type);
     pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
@@ -1016,12 +1021,14 @@ static BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd,
     acb->aio_type = type;
     acb->aio_fildes = fd;
 
+    acb->aio_nbytes = nb_sectors * BDRV_SECTOR_SIZE;
+    acb->aio_offset = sector_num * BDRV_SECTOR_SIZE;
+
     if (qiov) {
         acb->aio_iov = qiov->iov;
         acb->aio_niov = qiov->niov;
+        assert(qiov->size == acb->aio_nbytes);
     }
-    acb->aio_nbytes = nb_sectors * 512;
-    acb->aio_offset = sector_num * 512;
 
     trace_paio_submit(acb, opaque, sector_num, nb_sectors, type);
     pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/4] block: Make qiov match the request size until EOF

[Max Reitz]

On 04.07.2014 17:55, Kevin Wolf wrote:
> If a read request goes across EOF, the block driver sees a shortened
> request that stops at EOF (the rest is memsetted in block.c), however
> the original qiov was used for this request.
>
> This patch makes the qiov size match the request size, avoiding a
> potential buffer overflow in raw-posix.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] qcow2: Make qiov match request size until backing file EOF

[Max Reitz]

On 04.07.2014 17:55, Kevin Wolf wrote:
> If a qcow2 image has a shorter backing file and a read request to
> unallocated clusters goes across EOF of the backing file, the backing
> file sees a shortened request and the rest is filled with zeros.
> However, the original too long qiov was used with the shortened request.
>
> This patch makes the qiov size match the request size, avoiding a
> potential buffer overflow in raw-posix.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] qed: Make qiov match request size until backing file EOF

[Max Reitz]

On 04.07.2014 17:55, Kevin Wolf wrote:
> If a QED image has a shorter backing file and a read request to
> unallocated clusters goes across EOF of the backing file, the backing
> file sees a shortened request and the rest is filled with zeros.
> However, the original too long qiov was used with the shortened request.
>
> This patch makes the qiov size match the request size, avoiding a
> potential buffer overflow in raw-posix.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qed.c | 26 +++++++++++++++++++++++---
>   block/qed.h |  1 +
>   2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index b69374b..1f63b8f 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -772,6 +772,7 @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
>    */
>   static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
>                                     QEMUIOVector *qiov,
> +                                  QEMUIOVector **backing_qiov,

This could be documented in the comment above the function header.

>                                     BlockDriverCompletionFunc *cb, void *opaque)
>   {
>       uint64_t backing_length = 0;
> @@ -804,15 +805,20 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
>       /* If the read straddles the end of the backing file, shorten it */
>       size = MIN((uint64_t)backing_length - pos, qiov->size);
>   
> +    *backing_qiov = g_new(QEMUIOVector, 1);

I guess at least because of the qemu_iovec_destroy() block in 
qed_aio_next_io() *backing_qiov always has to be NULL before this point. 
I guess I'd like an assert(!*backing_qiov) here (or 
assert(!acb->backing_qiov) before the call to qed_read_backing_file() in 
qed_aio_read_data()) anyway to express clearly that there can be no leaks.

Speaking of leaks: Shouldn't the backing_qiov be freed in 
qed_aio_complete()?

Max

Re: [Qemu-devel] [PATCH 4/4] block: Assert qiov length matches request length

[Max Reitz]

On 04.07.2014 17:55, Kevin Wolf wrote:
> At least raw-posix relies on this because it can allocate bounce buffers
> based on the request length, but access it using all of the qiov entries
> later.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c           |  2 ++
>   block/raw-posix.c | 15 +++++++++++----
>   2 files changed, 13 insertions(+), 4 deletions(-)

Somehow I'm not sure whether I want to give my R-b to this, in case one 
of these assertions should fail. ;-)

But they look correct (considering QIO vectors now have to be just as 
long as the request), so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.1 0/4] block: Fix qiov sizes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On 07/04/2014 09:55 AM, Kevin Wolf wrote:
> Some callers use larger qiovs than nb_sectors for their read requests. This is
> bad because raw-posix uses nb_sectors to allocate a bounce buffer and then
> copies the whole iov over.
> 
> This series fixes some more cases (mirroring was already fixed earlier this
> week) and adds assertions to catch any other offenders.

Is the intent to include this in 2.1?

> 
> Kevin Wolf (4):
>   block: Make qiov match the request size until EOF
>   qcow2: Make qiov match request size until backing file EOF
>   qed: Make qiov match request size until backing file EOF
>   block: Assert qiov length matches request length
> 
>  block.c           | 13 ++++++++++++-
>  block/qcow2.c     | 11 ++++++++++-
>  block/qed.c       | 26 +++++++++++++++++++++++---
>  block/qed.h       |  1 +
>  block/raw-posix.c | 15 +++++++++++----
>  5 files changed, 57 insertions(+), 9 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/4] qed: Make qiov match request size until backing file EOF

[Kevin Wolf]

Am 05.07.2014 um 22:06 hat Max Reitz geschrieben:
> On 04.07.2014 17:55, Kevin Wolf wrote:
> >If a QED image has a shorter backing file and a read request to
> >unallocated clusters goes across EOF of the backing file, the backing
> >file sees a shortened request and the rest is filled with zeros.
> >However, the original too long qiov was used with the shortened request.
> >
> >This patch makes the qiov size match the request size, avoiding a
> >potential buffer overflow in raw-posix.
> >
> >Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block/qed.c | 26 +++++++++++++++++++++++---
> >  block/qed.h |  1 +
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> >
> >diff --git a/block/qed.c b/block/qed.c
> >index b69374b..1f63b8f 100644
> >--- a/block/qed.c
> >+++ b/block/qed.c
> >@@ -772,6 +772,7 @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
> >   */
> >  static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
> >                                    QEMUIOVector *qiov,
> >+                                  QEMUIOVector **backing_qiov,
> 
> This could be documented in the comment above the function header.
> 
> >                                    BlockDriverCompletionFunc *cb, void *opaque)
> >  {
> >      uint64_t backing_length = 0;
> >@@ -804,15 +805,20 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
> >      /* If the read straddles the end of the backing file, shorten it */
> >      size = MIN((uint64_t)backing_length - pos, qiov->size);
> >+    *backing_qiov = g_new(QEMUIOVector, 1);
> 
> I guess at least because of the qemu_iovec_destroy() block in
> qed_aio_next_io() *backing_qiov always has to be NULL before this
> point. I guess I'd like an assert(!*backing_qiov) here (or
> assert(!acb->backing_qiov) before the call to
> qed_read_backing_file() in qed_aio_read_data()) anyway to express
> clearly that there can be no leaks.

Okay, I'll add the suggested comment and assertion.

> Speaking of leaks: Shouldn't the backing_qiov be freed in
> qed_aio_complete()?

I can't see a code path where cb (i.e. qed_aio_next_io or
qed_copy_from_backing_file_write) wouldn't be called before
reaching qed_aio_complete(). Both of them free backing_qiov.

Kevin

[Qemu-devel] [PATCH v2 for-2.1 3/4] qed: Make qiov match request size until backing file EOF

[Kevin Wolf]

If a QED image has a shorter backing file and a read request to
unallocated clusters goes across EOF of the backing file, the backing
file sees a shortened request and the rest is filled with zeros.
However, the original too long qiov was used with the shortened request.

This patch makes the qiov size match the request size, avoiding a
potential buffer overflow in raw-posix.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 38 ++++++++++++++++++++++++++++++--------
 block/qed.h |  1 +
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index b69374b..cd4872b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -761,17 +761,19 @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
 /**
  * Read from the backing file or zero-fill if no backing file
  *
- * @s:          QED state
- * @pos:        Byte position in device
- * @qiov:       Destination I/O vector
- * @cb:         Completion function
- * @opaque:     User data for completion function
+ * @s:              QED state
+ * @pos:            Byte position in device
+ * @qiov:           Destination I/O vector
+ * @backing_qiov:   Possibly shortened copy of qiov, to be allocated here
+ * @cb:             Completion function
+ * @opaque:         User data for completion function
  *
  * This function reads qiov->size bytes starting at pos from the backing file.
  * If there is no backing file then zeroes are read.
  */
 static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
                                   QEMUIOVector *qiov,
+                                  QEMUIOVector **backing_qiov,
                                   BlockDriverCompletionFunc *cb, void *opaque)
 {
     uint64_t backing_length = 0;
@@ -804,15 +806,21 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     /* If the read straddles the end of the backing file, shorten it */
     size = MIN((uint64_t)backing_length - pos, qiov->size);
 
+    assert(*backing_qiov == NULL);
+    *backing_qiov = g_new(QEMUIOVector, 1);
+    qemu_iovec_init(*backing_qiov, qiov->niov);
+    qemu_iovec_concat(*backing_qiov, qiov, 0, size);
+
     BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO);
     bdrv_aio_readv(s->bs->backing_hd, pos / BDRV_SECTOR_SIZE,
-                   qiov, size / BDRV_SECTOR_SIZE, cb, opaque);
+                   *backing_qiov, size / BDRV_SECTOR_SIZE, cb, opaque);
 }
 
 typedef struct {
     GenericCB gencb;
     BDRVQEDState *s;
     QEMUIOVector qiov;
+    QEMUIOVector *backing_qiov;
     struct iovec iov;
     uint64_t offset;
 } CopyFromBackingFileCB;
@@ -829,6 +837,12 @@ static void qed_copy_from_backing_file_write(void *opaque, int ret)
     CopyFromBackingFileCB *copy_cb = opaque;
     BDRVQEDState *s = copy_cb->s;
 
+    if (copy_cb->backing_qiov) {
+        qemu_iovec_destroy(copy_cb->backing_qiov);
+        g_free(copy_cb->backing_qiov);
+        copy_cb->backing_qiov = NULL;
+    }
+
     if (ret) {
         qed_copy_from_backing_file_cb(copy_cb, ret);
         return;
@@ -866,11 +880,12 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
     copy_cb = gencb_alloc(sizeof(*copy_cb), cb, opaque);
     copy_cb->s = s;
     copy_cb->offset = offset;
+    copy_cb->backing_qiov = NULL;
     copy_cb->iov.iov_base = qemu_blockalign(s->bs, len);
     copy_cb->iov.iov_len = len;
     qemu_iovec_init_external(&copy_cb->qiov, &copy_cb->iov, 1);
 
-    qed_read_backing_file(s, pos, &copy_cb->qiov,
+    qed_read_backing_file(s, pos, &copy_cb->qiov, &copy_cb->backing_qiov,
                           qed_copy_from_backing_file_write, copy_cb);
 }
 
@@ -1313,7 +1328,7 @@ static void qed_aio_read_data(void *opaque, int ret,
         return;
     } else if (ret != QED_CLUSTER_FOUND) {
         qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov,
-                              qed_aio_next_io, acb);
+                              &acb->backing_qiov, qed_aio_next_io, acb);
         return;
     }
 
@@ -1339,6 +1354,12 @@ static void qed_aio_next_io(void *opaque, int ret)
 
     trace_qed_aio_next_io(s, acb, ret, acb->cur_pos + acb->cur_qiov.size);
 
+    if (acb->backing_qiov) {
+        qemu_iovec_destroy(acb->backing_qiov);
+        g_free(acb->backing_qiov);
+        acb->backing_qiov = NULL;
+    }
+
     /* Handle I/O error */
     if (ret) {
         qed_aio_complete(acb, ret);
@@ -1378,6 +1399,7 @@ static BlockDriverAIOCB *qed_aio_setup(BlockDriverState *bs,
     acb->qiov_offset = 0;
     acb->cur_pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE;
     acb->end_pos = acb->cur_pos + nb_sectors * BDRV_SECTOR_SIZE;
+    acb->backing_qiov = NULL;
     acb->request.l2_table = NULL;
     qemu_iovec_init(&acb->cur_qiov, qiov->niov);
 
diff --git a/block/qed.h b/block/qed.h
index b024751..2b0e724 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -142,6 +142,7 @@ typedef struct QEDAIOCB {
 
     /* Current cluster scatter-gather list */
     QEMUIOVector cur_qiov;
+    QEMUIOVector *backing_qiov;
     uint64_t cur_pos;               /* position on block device, in bytes */
     uint64_t cur_cluster;           /* cluster offset in image file */
     unsigned int cur_nclusters;     /* number of clusters being accessed */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 for-2.1 3/4] qed: Make qiov match request size until backing file EOF

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On 07/08/2014 07:14 AM, Kevin Wolf wrote:
> If a QED image has a shorter backing file and a read request to
> unallocated clusters goes across EOF of the backing file, the backing
> file sees a shortened request and the rest is filled with zeros.
> However, the original too long qiov was used with the shortened request.
> 
> This patch makes the qiov size match the request size, avoiding a
> potential buffer overflow in raw-posix.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 38 ++++++++++++++++++++++++++++++--------
>  block/qed.h |  1 +
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/4] block: Fix qiov sizes

[Kevin Wolf]

Am 04.07.2014 um 17:55 hat Kevin Wolf geschrieben:
> Some callers use larger qiovs than nb_sectors for their read requests. This is
> bad because raw-posix uses nb_sectors to allocate a bounce buffer and then
> copies the whole iov over.
> 
> This series fixes some more cases (mirroring was already fixed earlier this
> week) and adds assertions to catch any other offenders.

Applied to the block branch (with v2 of patch 3).

Kevin