diff mbox series

[V2] block/vhdx: add check for truncated image files

Message ID 20190902152413.14070-1-pl@kamp.de (mailing list archive)
State New, archived
Headers show
Series [V2] block/vhdx: add check for truncated image files | expand

Commit Message

Peter Lieven Sept. 2, 2019, 3:24 p.m. UTC
qemu is currently not able to detect truncated vhdx image files.
Add a basic check if all allocated blocks are reachable at open and
report all errors during bdrv_co_check.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
V2: - add error reporting [Kevin]
    - use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
    - factor out BAT entry check and add error reporting for region
      overlaps
    - already check on vhdx_open

 block/vhdx.c | 85 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 68 insertions(+), 17 deletions(-)

Comments

Peter Lieven Sept. 3, 2019, 9:09 a.m. UTC | #1
Am 02.09.19 um 17:24 schrieb Peter Lieven:
> qemu is currently not able to detect truncated vhdx image files.
> Add a basic check if all allocated blocks are reachable at open and
> report all errors during bdrv_co_check.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> V2: - add error reporting [Kevin]
>      - use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
>      - factor out BAT entry check and add error reporting for region
>        overlaps
>      - already check on vhdx_open
>
>   block/vhdx.c | 85 +++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 68 insertions(+), 17 deletions(-)
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 6a09d0a55c..6afba5e8c2 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -24,6 +24,7 @@
>   #include "qemu/option.h"
>   #include "qemu/crc32c.h"
>   #include "qemu/bswap.h"
> +#include "qemu/error-report.h"
>   #include "vhdx.h"
>   #include "migration/blocker.h"
>   #include "qemu/uuid.h"
> @@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
>       end = start + length;
>       QLIST_FOREACH(r, &s->regions, entries) {
>           if (!((start >= r->end) || (end <= r->start))) {
> +            error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
> +                         "region %" PRIu64 "-%." PRIu64, start, end, r->start,
> +                         r->end);
>               ret = -EINVAL;
>               goto exit;
>           }
> @@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
>   
>   }
>   
> +static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
> +{
> +    BDRVVHDXState *s = bs->opaque;
> +    int64_t image_file_size = bdrv_getlength(bs->file->bs);
> +    uint64_t payblocks = s->chunk_ratio;
> +    int i, ret = 0;
> +
> +    for (i = 0; i < s->bat_entries; i++) {
> +        if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
> +            PAYLOAD_BLOCK_FULLY_PRESENT) {
> +            /*
> +             * Check if fully allocated BAT entries do not reside after
> +             * end of the image file.
> +             */
> +            if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
> +                image_file_size) {
> +                error_report("VHDX BAT entry %d offset points after end of "
> +                             "file. Image has probably been truncated.", i);
> +                ret = -EINVAL;
> +                if (!errcnt) {
> +                    break;
> +                }
> +                (*errcnt)++;
> +            }
> +
> +            /*
> +             * verify populated BAT field file offsets against
> +             * region table and log entries
> +             */
> +            if (payblocks--) {
> +                /* payload bat entries */
> +                int ret2;
> +                ret2 = vhdx_region_check(s, s->bat[i] & VHDX_BAT_FILE_OFF_MASK,
> +                                         s->block_size);
> +                if (ret2 < 0) {
> +                    ret = -EINVAL;
> +                    if (errcnt) {
> +                        break;
> +                    }


This should be if (!errcnt) ...


I will respin, but wait for feedback regarding the remainder of the patch.


Peter
Kevin Wolf Sept. 3, 2019, 1:02 p.m. UTC | #2
Am 02.09.2019 um 17:24 hat Peter Lieven geschrieben:
> qemu is currently not able to detect truncated vhdx image files.
> Add a basic check if all allocated blocks are reachable at open and
> report all errors during bdrv_co_check.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> V2: - add error reporting [Kevin]
>     - use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
>     - factor out BAT entry check and add error reporting for region
>       overlaps
>     - already check on vhdx_open
> 
>  block/vhdx.c | 85 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 6a09d0a55c..6afba5e8c2 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -24,6 +24,7 @@
>  #include "qemu/option.h"
>  #include "qemu/crc32c.h"
>  #include "qemu/bswap.h"
> +#include "qemu/error-report.h"
>  #include "vhdx.h"
>  #include "migration/blocker.h"
>  #include "qemu/uuid.h"
> @@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
>      end = start + length;
>      QLIST_FOREACH(r, &s->regions, entries) {
>          if (!((start >= r->end) || (end <= r->start))) {
> +            error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
> +                         "region %" PRIu64 "-%." PRIu64, start, end, r->start,
> +                         r->end);
>              ret = -EINVAL;
>              goto exit;
>          }
> @@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
>  
>  }
>  
> +static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
> +{
> +    BDRVVHDXState *s = bs->opaque;
> +    int64_t image_file_size = bdrv_getlength(bs->file->bs);
> +    uint64_t payblocks = s->chunk_ratio;
> +    int i, ret = 0;

bdrv_getlength() can fail. It's probably better to error out immediately
instead of reporting that every BAT entry is > -1.

> +    for (i = 0; i < s->bat_entries; i++) {

s->bat_entries is uint32_t, so i should probably be the same.

> +        if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
> +            PAYLOAD_BLOCK_FULLY_PRESENT) {
> +            /*
> +             * Check if fully allocated BAT entries do not reside after
> +             * end of the image file.
> +             */
> +            if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
> +                image_file_size) {

Didn't we want to introduce an overflow check before making this check?
Something like if (bat_offset > UINT64_MAX - s->block_size)?

> +                error_report("VHDX BAT entry %d offset points after end of "
> +                             "file. Image has probably been truncated.", i);
> +                ret = -EINVAL;
> +                if (!errcnt) {
> +                    break;
> +                }
> +                (*errcnt)++;
> +            }
> +
> +            /*
> +             * verify populated BAT field file offsets against
> +             * region table and log entries
> +             */
> +            if (payblocks--) {
> +                /* payload bat entries */
> +                int ret2;
> +                ret2 = vhdx_region_check(s, s->bat[i] & VHDX_BAT_FILE_OFF_MASK,
> +                                         s->block_size);
> +                if (ret2 < 0) {
> +                    ret = -EINVAL;
> +                    if (errcnt) {

This one you already noticed yourself.

> +                        break;
> +                    }
> +                    (*errcnt)++;
> +                }
> +            } else {
> +                payblocks = s->chunk_ratio;
> +                /*
> +                 * Once differencing files are supported, verify sector bitmap
> +                 * blocks here
> +                 */
> +            }
> +        }
> +    }
> +
> +    return ret;
> +}

The rest looks good to me.

Kevin
Peter Lieven Sept. 3, 2019, 1:10 p.m. UTC | #3
Am 03.09.19 um 15:02 schrieb Kevin Wolf:
> Am 02.09.2019 um 17:24 hat Peter Lieven geschrieben:
>> qemu is currently not able to detect truncated vhdx image files.
>> Add a basic check if all allocated blocks are reachable at open and
>> report all errors during bdrv_co_check.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> V2: - add error reporting [Kevin]
>>      - use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
>>      - factor out BAT entry check and add error reporting for region
>>        overlaps
>>      - already check on vhdx_open
>>
>>   block/vhdx.c | 85 +++++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 68 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/vhdx.c b/block/vhdx.c
>> index 6a09d0a55c..6afba5e8c2 100644
>> --- a/block/vhdx.c
>> +++ b/block/vhdx.c
>> @@ -24,6 +24,7 @@
>>   #include "qemu/option.h"
>>   #include "qemu/crc32c.h"
>>   #include "qemu/bswap.h"
>> +#include "qemu/error-report.h"
>>   #include "vhdx.h"
>>   #include "migration/blocker.h"
>>   #include "qemu/uuid.h"
>> @@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
>>       end = start + length;
>>       QLIST_FOREACH(r, &s->regions, entries) {
>>           if (!((start >= r->end) || (end <= r->start))) {
>> +            error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
>> +                         "region %" PRIu64 "-%." PRIu64, start, end, r->start,
>> +                         r->end);
>>               ret = -EINVAL;
>>               goto exit;
>>           }
>> @@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
>>   
>>   }
>>   
>> +static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
>> +{
>> +    BDRVVHDXState *s = bs->opaque;
>> +    int64_t image_file_size = bdrv_getlength(bs->file->bs);
>> +    uint64_t payblocks = s->chunk_ratio;
>> +    int i, ret = 0;
> bdrv_getlength() can fail. It's probably better to error out immediately
> instead of reporting that every BAT entry is > -1.
>
>> +    for (i = 0; i < s->bat_entries; i++) {
> s->bat_entries is uint32_t, so i should probably be the same.
>
>> +        if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
>> +            PAYLOAD_BLOCK_FULLY_PRESENT) {
>> +            /*
>> +             * Check if fully allocated BAT entries do not reside after
>> +             * end of the image file.
>> +             */
>> +            if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
>> +                image_file_size) {
> Didn't we want to introduce an overflow check before making this check?
> Something like if (bat_offset > UINT64_MAX - s->block_size)?


Sorry, i missed that.


The bat entries are UINT64_T so this check will always be false for the

default block size of 1MB. In fact we should check for


bat_offset > INT64_MAX - s->block_size


right?


Peter
Kevin Wolf Sept. 3, 2019, 1:27 p.m. UTC | #4
Am 03.09.2019 um 15:10 hat Peter Lieven geschrieben:
> Am 03.09.19 um 15:02 schrieb Kevin Wolf:
> > Am 02.09.2019 um 17:24 hat Peter Lieven geschrieben:
> > > qemu is currently not able to detect truncated vhdx image files.
> > > Add a basic check if all allocated blocks are reachable at open and
> > > report all errors during bdrv_co_check.
> > > 
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > > V2: - add error reporting [Kevin]
> > >      - use bdrv_getlength instead of bdrv_get_allocated_file_size [Kevin]
> > >      - factor out BAT entry check and add error reporting for region
> > >        overlaps
> > >      - already check on vhdx_open
> > > 
> > >   block/vhdx.c | 85 +++++++++++++++++++++++++++++++++++++++++-----------
> > >   1 file changed, 68 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > index 6a09d0a55c..6afba5e8c2 100644
> > > --- a/block/vhdx.c
> > > +++ b/block/vhdx.c
> > > @@ -24,6 +24,7 @@
> > >   #include "qemu/option.h"
> > >   #include "qemu/crc32c.h"
> > >   #include "qemu/bswap.h"
> > > +#include "qemu/error-report.h"
> > >   #include "vhdx.h"
> > >   #include "migration/blocker.h"
> > >   #include "qemu/uuid.h"
> > > @@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
> > >       end = start + length;
> > >       QLIST_FOREACH(r, &s->regions, entries) {
> > >           if (!((start >= r->end) || (end <= r->start))) {
> > > +            error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
> > > +                         "region %" PRIu64 "-%." PRIu64, start, end, r->start,
> > > +                         r->end);
> > >               ret = -EINVAL;
> > >               goto exit;
> > >           }
> > > @@ -877,6 +881,60 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
> > >   }
> > > +static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
> > > +{
> > > +    BDRVVHDXState *s = bs->opaque;
> > > +    int64_t image_file_size = bdrv_getlength(bs->file->bs);
> > > +    uint64_t payblocks = s->chunk_ratio;
> > > +    int i, ret = 0;
> > bdrv_getlength() can fail. It's probably better to error out immediately
> > instead of reporting that every BAT entry is > -1.
> > 
> > > +    for (i = 0; i < s->bat_entries; i++) {
> > s->bat_entries is uint32_t, so i should probably be the same.
> > 
> > > +        if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
> > > +            PAYLOAD_BLOCK_FULLY_PRESENT) {
> > > +            /*
> > > +             * Check if fully allocated BAT entries do not reside after
> > > +             * end of the image file.
> > > +             */
> > > +            if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
> > > +                image_file_size) {
> > Didn't we want to introduce an overflow check before making this check?
> > Something like if (bat_offset > UINT64_MAX - s->block_size)?
> 
> Sorry, i missed that.
> 
> The bat entries are UINT64_T so this check will always be false for the
> default block size of 1MB. In fact we should check for
> 
> bat_offset > INT64_MAX - s->block_size
> 
> right?

Hm, VHDX_BAT_FILE_OFF_MASK is 0xFFFFFFFFFFF00000ULL, so 2^64 - 1 MB.
With a block size of 1 MB, this check would trigger because the offset
would be one byte higher than allowed (because offset + block_size
would be 0). For larger block sizes, it's more obvious that we can run
into this case.

As for INT64_MAX, I'm not sure if it's strictly necessary because the
code seems to use unsigned variables everywhere. But it feels safer and
shouldn't make any difference in practice, so I agree with using it.

Kevin
diff mbox series

Patch

diff --git a/block/vhdx.c b/block/vhdx.c
index 6a09d0a55c..6afba5e8c2 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -24,6 +24,7 @@ 
 #include "qemu/option.h"
 #include "qemu/crc32c.h"
 #include "qemu/bswap.h"
+#include "qemu/error-report.h"
 #include "vhdx.h"
 #include "migration/blocker.h"
 #include "qemu/uuid.h"
@@ -235,6 +236,9 @@  static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
     end = start + length;
     QLIST_FOREACH(r, &s->regions, entries) {
         if (!((start >= r->end) || (end <= r->start))) {
+            error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
+                         "region %" PRIu64 "-%." PRIu64, start, end, r->start,
+                         r->end);
             ret = -EINVAL;
             goto exit;
         }
@@ -877,6 +881,60 @@  static void vhdx_calc_bat_entries(BDRVVHDXState *s)
 
 }
 
+static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
+{
+    BDRVVHDXState *s = bs->opaque;
+    int64_t image_file_size = bdrv_getlength(bs->file->bs);
+    uint64_t payblocks = s->chunk_ratio;
+    int i, ret = 0;
+
+    for (i = 0; i < s->bat_entries; i++) {
+        if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
+            PAYLOAD_BLOCK_FULLY_PRESENT) {
+            /*
+             * Check if fully allocated BAT entries do not reside after
+             * end of the image file.
+             */
+            if ((s->bat[i] & VHDX_BAT_FILE_OFF_MASK) + s->block_size >
+                image_file_size) {
+                error_report("VHDX BAT entry %d offset points after end of "
+                             "file. Image has probably been truncated.", i);
+                ret = -EINVAL;
+                if (!errcnt) {
+                    break;
+                }
+                (*errcnt)++;
+            }
+
+            /*
+             * verify populated BAT field file offsets against
+             * region table and log entries
+             */
+            if (payblocks--) {
+                /* payload bat entries */
+                int ret2;
+                ret2 = vhdx_region_check(s, s->bat[i] & VHDX_BAT_FILE_OFF_MASK,
+                                         s->block_size);
+                if (ret2 < 0) {
+                    ret = -EINVAL;
+                    if (errcnt) {
+                        break;
+                    }
+                    (*errcnt)++;
+                }
+            } else {
+                payblocks = s->chunk_ratio;
+                /*
+                 * Once differencing files are supported, verify sector bitmap
+                 * blocks here
+                 */
+            }
+        }
+    }
+
+    return ret;
+}
+
 static void vhdx_close(BlockDriverState *bs)
 {
     BDRVVHDXState *s = bs->opaque;
@@ -981,25 +1039,15 @@  static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    uint64_t payblocks = s->chunk_ratio;
-    /* endian convert, and verify populated BAT field file offsets against
-     * region table and log entries */
+    /* endian convert populated BAT field entires */
     for (i = 0; i < s->bat_entries; i++) {
         s->bat[i] = le64_to_cpu(s->bat[i]);
-        if (payblocks--) {
-            /* payload bat entries */
-            if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
-                    PAYLOAD_BLOCK_FULLY_PRESENT) {
-                ret = vhdx_region_check(s, s->bat[i] & VHDX_BAT_FILE_OFF_MASK,
-                                        s->block_size);
-                if (ret < 0) {
-                    goto fail;
-                }
-            }
-        } else {
-            payblocks = s->chunk_ratio;
-            /* Once differencing files are supported, verify sector bitmap
-             * blocks here */
+    }
+
+    if (!(flags & BDRV_O_CHECK)) {
+        ret = vhdx_check_bat_entries(bs, NULL);
+        if (ret < 0) {
+            goto fail;
         }
     }
 
@@ -2072,6 +2120,9 @@  static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
     if (s->log_replayed_on_open) {
         result->corruptions_fixed++;
     }
+
+    vhdx_check_bat_entries(bs, &result->corruptions);
+
     return 0;
 }