diff mbox

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

Message ID 1404489305-8750-4-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf July 4, 2014, 3:55 p.m. UTC 
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(-)

Comments

Max Reitz July 5, 2014, 8:06 p.m. UTC | #1 
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
Kevin Wolf July 8, 2014, 1:07 p.m. UTC | #2 
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
Eric Blake July 9, 2014, 1:52 a.m. UTC | #3 
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>
diff mbox

Patch

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 */