VHDX bugfixes

PATCH Applied Jeff Cody 4 participants 9 messages Aug 7, 2017 Block layer

lore

Share & tools

lore

Jeff Cody

author

2017-08-07 12:38 UTC #0 lore raw

v2 cover VHDX bugfixes

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(-)

Signature · 1 line

2.9.4

Jeff Cody

author

2017-08-07 12:38 UTC #1 lore raw

PATCH v2 1/4 block/vhdx: check error return of bdrv_getlength()

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 <[email protected]>

Reviewed-by Eric Blake <[email protected]>

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 23 ++++++++++++++++++-----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 26 insertions(+), 6 deletions(-)

block/vhdx-log.c +18 -5

index 01278f3..2e26fd4 100644
@@ -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);

block/vhdx.c +8 -1

index a9cecd2..37224b8 100644
@@ -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);

Signature · 1 line

2.9.4

Jeff Cody

author

2017-08-07 12:38 UTC #2 lore raw

PATCH v2 2/4 block/vhdx: check for offset overflow to bdrv_truncate()

In reply to 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 <[email protected]>

Reviewed-by Kevin Wolf <[email protected]>

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 6 +++++-
 block/vhdx.c     | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

block/vhdx-log.c +5 -1

index 2e26fd4..9597223 100644
@@ -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);
             }
         }

block/vhdx.c +3 -0

index 37224b8..7ae4589 100644
@@ -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);

Signature · 1 line

2.9.4

Philippe Mathieu-Daudé

Reviewed-by on v2 2/4

2017-08-07 14:06 UTC #3 lore raw

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

Quoted 4 lines · 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().

Quoted 8 lines

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 <[email protected]>
Reviewed-by Kevin Wolf <[email protected]>
Signed-off-by Jeff Cody <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Quoted 40 lines

---
 block/vhdx-log.c | 6 +++++-
 block/vhdx.c     | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

block/vhdx-log.c +5 -1

index 2e26fd4..9597223 100644
@@ -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);
             }
         }

block/vhdx.c +3 -1

index 37224b8..7ae4589 100644
@@ -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

Jeff Cody

author

2017-08-07 12:38 UTC #4 lore raw

PATCH v2 3/4 block/vhdx: check error return of bdrv_flush()

In reply to Jeff Cody

Reported-by Kevin Wolf <[email protected]>

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

block/vhdx-log.c +14 -3

index 9597223..a27dc05 100644
@@ -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;

Signature · 1 line

2.9.4

Eric Blake

Reviewed-by on v2 3/4

2017-08-07 12:55 UTC #5 lore raw

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

Quoted 6 lines · Jeff Cody

Reported-by Kevin Wolf <[email protected]>
Signed-off-by Jeff Cody <[email protected]>
---
 block/vhdx-log.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Reviewed-by Eric Blake <[email protected]>

Signature · 3 lines

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

Jeff Cody

author

2017-08-07 12:38 UTC #6 lore raw

PATCH v2 4/4 block/vhdx: check error return of bdrv_truncate()

In reply to Jeff Cody

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

block/vhdx-log.c +5 -1

index a27dc05..14b724e 100644
@@ -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);

Signature · 1 line

2.9.4

Eric Blake

Reviewed-by on v2 4/4

2017-08-07 12:56 UTC #7 lore raw

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

Quoted 5 lines · Jeff Cody

Signed-off-by Jeff Cody <[email protected]>
---
 block/vhdx-log.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by Eric Blake <[email protected]>

Quoted 18 lines

block/vhdx-log.c +5 -1

index a27dc05..14b724e 100644
@@ -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);

Signature · 3 lines

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

Kevin Wolf

Applied

2017-08-07 13:59 UTC #8 lore raw

In reply to Jeff Cody

Am 07.08.2017 um 14:38 hat Jeff Cody geschrieben:

Quoted 4 lines

Some VHDX bug fixes, including:
1. Checking bdrv_getlength(), bdrv_flush(), and bdrv_truncate() return values

Thanks, applied to the block branch.

Kevin