[Qemu-devel] [PATCH v2 for-2.10 0/4] VHDX bugfixes

[Qemu-devel] [PATCH v2 for-2.10 0/4] VHDX bugfixes

[Jeff Cody]

Some VHDX bug fixes, including:

1. Checking bdrv_getlength(), bdrv_flush(), and bdrv_truncate() return values

Changes from v1->v2:

git-backport-diff -r qemu/master.. -u vhdx-fixes-v1
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[0013] [FC] 'block/vhdx: check error return of bdrv_getlength()'
       ^^^^
Some code cleanup / movement (Thanks Kevin)

002/4:[0002] [FC] 'block/vhdx: check for offset overflow to bdrv_truncate()'
       ^^^^
Use QEMU_ALIGN_UP (Thanks Eric)

003/4:[down] 'block/vhdx: check error return of bdrv_flush()'

New, suggested by Kevin.

004/4:[down] 'block/vhdx: check error return of bdrv_truncate()'

New.

1. Check for error when calling bdrv_getlength() [Markus]

2. Check for overflow in offset prior to calling bdrv_truncate().

Jeff Cody (4):
  block/vhdx: check error return of bdrv_getlength()
  block/vhdx: check for offset overflow to bdrv_truncate()
  block/vhdx: check error return of bdrv_flush()
  block/vhdx: check error return of bdrv_truncate()

 block/vhdx-log.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
 block/vhdx.c     | 12 +++++++++++-
 2 files changed, 53 insertions(+), 11 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v2 for-2.10 1/4] block/vhdx: check error return of bdrv_getlength()

[Jeff Cody]

Calls to bdrv_getlength() were not checking for error.  In vhdx.c, this
can lead to truncating an image file, so it is a definite bug.  In
vhdx-log.c, the path for improper behavior is less clear, but it is best
to check in any case.

Some minor code movement of the log_guid intialization, as well.

Reported-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx-log.c | 23 ++++++++++++++++++-----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 01278f3..2e26fd4 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -491,6 +491,7 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
     uint32_t cnt, sectors_read;
     uint64_t new_file_size;
     void *data = NULL;
+    int64_t file_length;
     VHDXLogDescEntries *desc_entries = NULL;
     VHDXLogEntryHeader hdr_tmp = { 0 };
 
@@ -510,10 +511,15 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
         if (ret < 0) {
             goto exit;
         }
+        file_length = bdrv_getlength(bs->file->bs);
+        if (file_length < 0) {
+            ret = file_length;
+            goto exit;
+        }
         /* if the log shows a FlushedFileOffset larger than our current file
          * size, then that means the file has been truncated / corrupted, and
          * we must refused to open it / use it */
-        if (hdr_tmp.flushed_file_offset > bdrv_getlength(bs->file->bs)) {
+        if (hdr_tmp.flushed_file_offset > file_length) {
             ret = -EINVAL;
             goto exit;
         }
@@ -543,7 +549,7 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
                 goto exit;
             }
         }
-        if (bdrv_getlength(bs->file->bs) < desc_entries->hdr.last_file_offset) {
+        if (file_length < desc_entries->hdr.last_file_offset) {
             new_file_size = desc_entries->hdr.last_file_offset;
             if (new_file_size % (1024*1024)) {
                 /* round up to nearest 1MB boundary */
@@ -851,6 +857,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
     uint32_t partial_sectors = 0;
     uint32_t bytes_written = 0;
     uint64_t file_offset;
+    int64_t file_length;
     VHDXHeader *header;
     VHDXLogEntryHeader new_hdr;
     VHDXLogDescriptor *new_desc = NULL;
@@ -904,6 +911,12 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
 
     sectors += partial_sectors;
 
+    file_length = bdrv_getlength(bs->file->bs);
+    if (file_length < 0) {
+        ret = file_length;
+        goto exit;
+    }
+
     /* sectors is now how many sectors the data itself takes, not
      * including the header and descriptor metadata */
 
@@ -913,11 +926,11 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
                 .sequence_number     = s->log.sequence,
                 .descriptor_count    = sectors,
                 .reserved            = 0,
-                .flushed_file_offset = bdrv_getlength(bs->file->bs),
-                .last_file_offset    = bdrv_getlength(bs->file->bs),
+                .flushed_file_offset = file_length,
+                .last_file_offset    = file_length,
+                .log_guid            = header->log_guid,
               };
 
-    new_hdr.log_guid = header->log_guid;
 
     desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);
 
diff --git a/block/vhdx.c b/block/vhdx.c
index a9cecd2..37224b8 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1166,7 +1166,14 @@ exit:
 static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
                                     uint64_t *new_offset)
 {
-    *new_offset = bdrv_getlength(bs->file->bs);
+    int64_t current_len;
+
+    current_len = bdrv_getlength(bs->file->bs);
+    if (current_len < 0) {
+        return current_len;
+    }
+
+    *new_offset = current_len;
 
     /* per the spec, the address for a block is in units of 1MB */
     *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
-- 
2.9.4

[Qemu-devel] [PATCH v2 for-2.10 2/4] block/vhdx: check for offset overflow to bdrv_truncate()

[Jeff Cody]

VHDX uses uint64_t types for most offsets, following the VHDX spec.
However, bdrv_truncate() takes an int64_t value for the truncating
offset.  Check for overflow before calling bdrv_truncate().

While we are here, replace the bit shifting with QEMU_ALIGN_UP as well.

N.B.: For a compliant image this is not an issue, as the maximum VHDX
image size is defined per the spec to be 64TB.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx-log.c | 6 +++++-
 block/vhdx.c     | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 2e26fd4..9597223 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -553,7 +553,11 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
             new_file_size = desc_entries->hdr.last_file_offset;
             if (new_file_size % (1024*1024)) {
                 /* round up to nearest 1MB boundary */
-                new_file_size = ((new_file_size >> 20) + 1) << 20;
+                new_file_size = QEMU_ALIGN_UP(new_file_size, MiB);
+                if (new_file_size > INT64_MAX) {
+                    ret = -EINVAL;
+                    goto exit;
+                }
                 bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL);
             }
         }
diff --git a/block/vhdx.c b/block/vhdx.c
index 37224b8..7ae4589 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1177,6 +1177,9 @@ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
 
     /* per the spec, the address for a block is in units of 1MB */
     *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
+    if (*new_offset > INT64_MAX) {
+        return -EINVAL;
+    }
 
     return bdrv_truncate(bs->file, *new_offset + s->block_size,
                          PREALLOC_MODE_OFF, NULL);
-- 
2.9.4

[Qemu-devel] [PATCH v2 for-2.10 3/4] block/vhdx: check error return of bdrv_flush()

[Jeff Cody]

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx-log.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 9597223..a27dc05 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -565,7 +565,10 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
         desc_entries = NULL;
     }
 
-    bdrv_flush(bs);
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        goto exit;
+    }
     /* once the log is fully flushed, indicate that we have an empty log
      * now.  This also sets the log guid to 0, to indicate an empty log */
     vhdx_log_reset(bs, s);
@@ -1039,7 +1042,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s,
 
     /* Make sure data written (new and/or changed blocks) is stable
      * on disk, before creating log entry */
-    bdrv_flush(bs);
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        goto exit;
+    }
+
     ret = vhdx_log_write(bs, s, data, length, offset);
     if (ret < 0) {
         goto exit;
@@ -1047,7 +1054,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s,
     logs.log = s->log;
 
     /* Make sure log is stable on disk */
-    bdrv_flush(bs);
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        goto exit;
+    }
+
     ret = vhdx_log_flush(bs, s, &logs);
     if (ret < 0) {
         goto exit;
-- 
2.9.4

[Qemu-devel] [PATCH v2 for-2.10 4/4] block/vhdx: check error return of bdrv_truncate()

[Jeff Cody]

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx-log.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index a27dc05..14b724e 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -558,7 +558,11 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
                     ret = -EINVAL;
                     goto exit;
                 }
-                bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL);
+                ret = bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF,
+                                    NULL);
+                if (ret < 0) {
+                    goto exit;
+                }
             }
         }
         qemu_vfree(desc_entries);
-- 
2.9.4

Re: [Qemu-devel] [PATCH v2 for-2.10 3/4] block/vhdx: check error return of bdrv_flush()

[Eric Blake]

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

On 08/07/2017 07:38 AM, Jeff Cody wrote:
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vhdx-log.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 for-2.10 4/4] block/vhdx: check error return of bdrv_truncate()

[Eric Blake]

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

On 08/07/2017 07:38 AM, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vhdx-log.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

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

> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index a27dc05..14b724e 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -558,7 +558,11 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
>                      ret = -EINVAL;
>                      goto exit;
>                  }
> -                bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL);
> +                ret = bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF,
> +                                    NULL);
> +                if (ret < 0) {
> +                    goto exit;
> +                }
>              }
>          }
>          qemu_vfree(desc_entries);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 for-2.10 0/4] VHDX bugfixes

[Kevin Wolf]

Am 07.08.2017 um 14:38 hat Jeff Cody geschrieben:
> 
> Some VHDX bug fixes, including:
> 
> 1. Checking bdrv_getlength(), bdrv_flush(), and bdrv_truncate() return values

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v2 for-2.10 2/4] block/vhdx: check for offset overflow to bdrv_truncate()

[Philippe Mathieu-Daudé]

On Mon, Aug 7, 2017 at 9:38 AM, Jeff Cody <jcody@redhat.com> wrote:
> VHDX uses uint64_t types for most offsets, following the VHDX spec.
> However, bdrv_truncate() takes an int64_t value for the truncating
> offset.  Check for overflow before calling bdrv_truncate().
>
> While we are here, replace the bit shifting with QEMU_ALIGN_UP as well.
>
> N.B.: For a compliant image this is not an issue, as the maximum VHDX
> image size is defined per the spec to be 64TB.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/vhdx-log.c | 6 +++++-
>  block/vhdx.c     | 3 +++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index 2e26fd4..9597223 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -553,7 +553,11 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
>              new_file_size = desc_entries->hdr.last_file_offset;
>              if (new_file_size % (1024*1024)) {
>                  /* round up to nearest 1MB boundary */
> -                new_file_size = ((new_file_size >> 20) + 1) << 20;
> +                new_file_size = QEMU_ALIGN_UP(new_file_size, MiB);
> +                if (new_file_size > INT64_MAX) {
> +                    ret = -EINVAL;
> +                    goto exit;
> +                }
>                  bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL);
>              }
>          }
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 37224b8..7ae4589 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1177,6 +1177,9 @@ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
>
>      /* per the spec, the address for a block is in units of 1MB */
>      *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
> +    if (*new_offset > INT64_MAX) {
> +        return -EINVAL;
> +    }
>
>      return bdrv_truncate(bs->file, *new_offset + s->block_size,
>                           PREALLOC_MODE_OFF, NULL);
> --
> 2.9.4
>
>