[Qemu-devel] [PATCH 0/2] VHDX cleanup

[Qemu-devel] [PATCH 0/2] VHDX cleanup

[Jeff Cody]

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

-- 
2.9.4

[Qemu-devel] [PATCH 1/2] 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.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 01278f3..fd4e7af 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;
@@ -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);
diff --git a/block/vhdx.c b/block/vhdx.c
index a9cecd2..6a14999 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 2/2] 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().

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 <jcody@redhat.com>
---
 block/vhdx-log.c | 4 ++++
 block/vhdx.c     | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index fd4e7af..3b74e5d 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -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);
             }
         }
diff --git a/block/vhdx.c b/block/vhdx.c
index 6a14999..c45af73 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

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

[Kevin Wolf]

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:
> 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 <armbru@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vhdx-log.c | 20 ++++++++++++++++----
>  block/vhdx.c     |  9 ++++++++-
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index 01278f3..fd4e7af 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 */

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.

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

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.

>      desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);
> diff --git a/block/vhdx.c b/block/vhdx.c
> index a9cecd2..6a14999 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;
> +    }

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

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

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

[Kevin Wolf]

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:
> 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 <jcody@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] VHDX cleanup

[Kevin Wolf]

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:
> 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

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

[Eric Blake]

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

On 08/06/2017 10:08 PM, Jeff Cody 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().
> 
> 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 <jcody@redhat.com>
> ---
>  block/vhdx-log.c | 4 ++++
>  block/vhdx.c     | 3 +++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index fd4e7af..3b74e5d 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -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?

> +                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 <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 1/2] block/vhdx: check error return of bdrv_getlength()

[Eric Blake]

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

On 08/06/2017 10:08 PM, Jeff Cody wrote:
> 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 <armbru@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/vhdx-log.c | 20 ++++++++++++++++----
>  block/vhdx.c     |  9 ++++++++-
>  2 files changed, 24 insertions(+), 5 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 2/2] block/vhdx: check for offset overflow to bdrv_truncate()

[Jeff Cody]

On Mon, Aug 07, 2017 at 06:24:30AM -0500, Eric Blake wrote:
> On 08/06/2017 10:08 PM, Jeff Cody 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().
> > 
> > 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 <jcody@redhat.com>
> > ---
> >  block/vhdx-log.c | 4 ++++
> >  block/vhdx.c     | 3 +++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > index fd4e7af..3b74e5d 100644
> > --- a/block/vhdx-log.c
> > +++ b/block/vhdx-log.c
> > @@ -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.

> > +                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 <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 

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

[Jeff Cody]

On Mon, Aug 07, 2017 at 12:46:30PM +0200, Kevin Wolf wrote:
> Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:
> > 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 <armbru@redhat.com>
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/vhdx-log.c | 20 ++++++++++++++++----
> >  block/vhdx.c     |  9 ++++++++-
> >  2 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > index 01278f3..fd4e7af 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 */
> 
> 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.


> > @@ -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;
> 
> 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.
> 
> >      desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);
> > diff --git a/block/vhdx.c b/block/vhdx.c
> > index a9cecd2..6a14999 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;
> > +    }
> 
> Don't you want the empty line to be between declaration and code rather
> than assignment and check?
> 
> > +    *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.