VHDX cleanup

PATCH Superseded Jeff Cody 3 participants 10 messages Aug 7, 2017 Block layer

lore

Share & tools

lore

Jeff Cody

author

2017-08-07 03:08 UTC #0 lore raw

cover VHDX cleanup

Two VHDX items cleaned up:

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

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

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

 block/vhdx-log.c | 24 ++++++++++++++++++++----
 block/vhdx.c     | 12 +++++++++++-
 2 files changed, 31 insertions(+), 5 deletions(-)

Signature · 1 line

2.9.4

Jeff Cody

author

2017-08-07 03:08 UTC #1 lore raw

PATCH 1/2 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.

Reported-by Markus Armbruster <[email protected]>

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

---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

block/vhdx-log.c +16 -4

index 01278f3..fd4e7af 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;

@@ -913,10 +920,15 @@ 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),
               };
 
+    file_length = bdrv_getlength(bs->file->bs);
+    if (file_length < 0) {
+        ret = file_length;
+        goto exit;
+    }
+    new_hdr.flushed_file_offset = file_length;
+    new_hdr.last_file_offset    = file_length;
     new_hdr.log_guid = header->log_guid;
 
     desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);

block/vhdx.c +8 -1

index a9cecd2..6a14999 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

Kevin Wolf

2017-08-07 10:46 UTC #2 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block/vhdx: check error return of bdrv_getlength()

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:

Quoted 42 lines

Reported-by Markus Armbruster <[email protected]>

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

---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

block/vhdx-log.c +7 -1

index 01278f3..fd4e7af 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;
         }

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 555

Quoted 8 lines

block/vhdx-log.c +1 -1

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

The vhdx_log_flush() part looks good, but it made me notice a bdrv_flush() in the same function where the return value isn't checked. I'm almost sure that this is a bug.

Quoted 14 lines

block/vhdx-log.c +1 -2

@@ -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;

@@ -913,10 +920,15 @@ 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),

block/vhdx-log.c static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s, · line 932

Quoted 10 lines

block/vhdx-log.c +7 -0

               };
 
+    file_length = bdrv_getlength(bs->file->bs);
+    if (file_length < 0) {
+        ret = file_length;
+        goto exit;
+    }
+    new_hdr.flushed_file_offset = file_length;
+    new_hdr.last_file_offset    = file_length;
     new_hdr.log_guid = header->log_guid;

If you move the bdrv_getlength() above the initialisation of new_hdr, you could keep these fields in the designated initialiser, which should be better for readability.

I also don't know why .log_guid isn't part if it, could be moved, too.

Quoted 6 lines

block/vhdx-log.c +0 -0

     desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);

block/vhdx.c +0 -0

index a9cecd2..6a14999 100644

block/vhdx.c exit: · line 1174

Quoted 10 lines

block/vhdx.c +6 -1

@@ -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;
+    }

Don't you want the empty line to be between declaration and code rather than assignment and check?

block/vhdx.c exit: · line 1179

Quoted 4 lines

block/vhdx.c +1 -0

+    *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);

So the code looks correct, but we could make it a little nicer in a v2.

Kevin

Jeff Cody

author

2017-08-07 12:17 UTC #3 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block/vhdx: check error return of bdrv_getlength()

Quoted 47 lines · Kevin Wolf

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:

Quoted 46 lines

Reported-by Markus Armbruster <[email protected]>

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

---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

block/vhdx-log.c +7 -2

index 01278f3..fd4e7af 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) {

Quoted 9 lines

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 555
Quoted 4 lines
block/vhdx-log.c +1 -0
+        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 */
The vhdx_log_flush() part looks good, but it made me notice a bdrv_flush() in the same function where the return value isn't checked. I'm almost sure that this is a bug.

I'll add 2 more simple patches to the series: one for bdrv_flush(), and one for the unchecked bdrv_truncate() as well.

Quoted 51 lines

Quoted 14 lines

block/vhdx-log.c +1 -2

@@ -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;

@@ -913,10 +920,15 @@ 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),

block/vhdx-log.c static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s, · line 932

Quoted 10 lines

block/vhdx-log.c +7 -0

               };
 
+    file_length = bdrv_getlength(bs->file->bs);
+    if (file_length < 0) {
+        ret = file_length;
+        goto exit;
+    }
+    new_hdr.flushed_file_offset = file_length;
+    new_hdr.last_file_offset    = file_length;
     new_hdr.log_guid = header->log_guid;

If you move the bdrv_getlength() above the initialisation of new_hdr, you could keep these fields in the designated initialiser, which should be better for readability.

I also don't know why .log_guid isn't part if it, could be moved, too.

Quoted 6 lines

block/vhdx-log.c +0 -0

     desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);

block/vhdx.c +0 -0

index a9cecd2..6a14999 100644

block/vhdx.c exit: · line 1174

Quoted 10 lines

block/vhdx.c +6 -1

@@ -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;
+    }

Don't you want the empty line to be between declaration and code rather than assignment and check?

Quoted 7 lines

block/vhdx.c exit: · line 1179
Quoted 4 lines
block/vhdx.c +1 -0
+    *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);
So the code looks correct, but we could make it a little nicer in a v2.

Yep, thanks. I'll address all those cleanups you mentioned in v2.

Eric Blake

Reviewed-by on v1 1/2

2017-08-07 11:25 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 1/2] block/vhdx: check error return of bdrv_getlength()

In reply to Jeff Cody

Quoted 11 lines · 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.
Reported-by Markus Armbruster <[email protected]>
Signed-off-by Jeff Cody <[email protected]>
---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 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 03:08 UTC #5 lore raw

PATCH 2/2 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().

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.

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

---
 block/vhdx-log.c | 4 ++++
 block/vhdx.c     | 3 +++
 2 files changed, 7 insertions(+)

block/vhdx-log.c +4 -0

index fd4e7af..3b74e5d 100644
@@ -554,6 +554,10 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
             if (new_file_size % (1024*1024)) {
                 /* round up to nearest 1MB boundary */
                 new_file_size = ((new_file_size >> 20) + 1) << 20;
+                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 6a14999..c45af73 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

Kevin Wolf

Reviewed-by on v1 2/2

2017-08-07 10:48 UTC #6 lore raw

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

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:

Quoted 8 lines

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().
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.
Signed-off-by Jeff Cody <[email protected]>

Reviewed-by Kevin Wolf <[email protected]>

Eric Blake

Reviewed-by on v1 2/2

2017-08-07 11:24 UTC #7 lore raw

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

In reply to Jeff Cody

Quoted 18 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().
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.
Signed-off-by Jeff Cody <[email protected]>
---
 block/vhdx-log.c | 4 ++++
 block/vhdx.c     | 3 +++
 2 files changed, 7 insertions(+)
block/vhdx-log.c +0 -0
index fd4e7af..3b74e5d 100644

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 556

Quoted 3 lines

block/vhdx-log.c +0 -0

@@ -554,6 +554,10 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
             if (new_file_size % (1024*1024)) {
                 /* round up to nearest 1MB boundary */
                 new_file_size = ((new_file_size >> 20) + 1) << 20;

Since you're touching here, can you fix this to use QEMU_ALIGN_UP instead?

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 561

Quoted 5 lines

block/vhdx-log.c +4 -0

+                if (new_file_size > INT64_MAX) {
+                    ret = -EINVAL;
+                    goto exit;
+                }
                 bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL);

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:13 UTC #8 lore raw

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

Quoted 14 lines · Eric Blake

Quoted 13 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().
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.
Signed-off-by Jeff Cody <[email protected]>
---
 block/vhdx-log.c | 4 ++++
 block/vhdx.c     | 3 +++
 2 files changed, 7 insertions(+)

Quoted 11 lines

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 556
Quoted 8 lines
block/vhdx-log.c +0 -0
index fd4e7af..3b74e5d 100644
@@ -554,6 +554,10 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
             if (new_file_size % (1024*1024)) {
                 /* round up to nearest 1MB boundary */
                 new_file_size = ((new_file_size >> 20) + 1) << 20;
Since you're touching here, can you fix this to use QEMU_ALIGN_UP instead?

Good idea, yes.

Quoted 13 lines

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 561
Quoted 5 lines
block/vhdx-log.c +4 -0
+                if (new_file_size > INT64_MAX) {
+                    ret = -EINVAL;
+                    goto exit;
+                }
                 bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL);
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

Kevin Wolf

2017-08-07 10:49 UTC #9 lore raw

In reply to Jeff Cody

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:

Quoted 5 lines

Two VHDX items cleaned up:
1. Check for error when calling bdrv_getlength() [Markus]
2. Check for overflow in offset prior to calling bdrv_truncate().

This doesn't look like cleanups to me, but like proper fixes. I think it would still qualify for 2.10.

Kevin