qemu is currently not able to detect truncated vhdx image files.
Add a basic check if all allocated blocks are reachable to vhdx_co_check.

Signed-off-by: Jan-Hendrik Frintrop <jhf@kamp.de>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/vhdx.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/block/vhdx.c b/block/vhdx.c
index 6a09d0a55c..4382b1375d 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2068,10 +2068,29 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
                                       BdrvCheckMode fix)
 {
     BDRVVHDXState *s = bs->opaque;
+    VHDXSectorInfo sinfo;
+    int64_t file_size = bdrv_get_allocated_file_size(bs);
+    int64_t sector_num;
 
     if (s->log_replayed_on_open) {
         result->corruptions_fixed++;
     }
+
+    for (sector_num = 0; sector_num < bs->total_sectors;
+         sector_num += s->block_size / BDRV_SECTOR_SIZE) {
+        int nb_sectors = MIN(bs->total_sectors - sector_num,
+                             s->block_size / BDRV_SECTOR_SIZE);
+        vhdx_block_translate(s, sector_num, nb_sectors, &sinfo);
+        if ((s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) ==
+            PAYLOAD_BLOCK_FULLY_PRESENT) {
+            if (sinfo.file_offset +
+                sinfo.sectors_avail * BDRV_SECTOR_SIZE > file_size) {
+                /* block is past the end of file, image has been truncated. */
+                result->corruptions++;
+            }
+        }
+    }
+
     return 0;
 }
 
-- 
2.17.1

Am 29.08.2019 um 15:36 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 to vhdx_co_check.
> 
> Signed-off-by: Jan-Hendrik Frintrop <jhf@kamp.de>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/vhdx.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 6a09d0a55c..4382b1375d 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -2068,10 +2068,29 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
>                                        BdrvCheckMode fix)
>  {
>      BDRVVHDXState *s = bs->opaque;
> +    VHDXSectorInfo sinfo;
> +    int64_t file_size = bdrv_get_allocated_file_size(bs);

Don't you mean bdrv_getlength()?

bdrv_get_allocated_file_size() is only the allocated size, i.e. without
holes. So a higher offset may actually be present.

> +    int64_t sector_num;
>  
>      if (s->log_replayed_on_open) {
>          result->corruptions_fixed++;
>      }
> +
> +    for (sector_num = 0; sector_num < bs->total_sectors;
> +         sector_num += s->block_size / BDRV_SECTOR_SIZE) {
> +        int nb_sectors = MIN(bs->total_sectors - sector_num,
> +                             s->block_size / BDRV_SECTOR_SIZE);
> +        vhdx_block_translate(s, sector_num, nb_sectors, &sinfo);
> +        if ((s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) ==
> +            PAYLOAD_BLOCK_FULLY_PRESENT) {
> +            if (sinfo.file_offset +
> +                sinfo.sectors_avail * BDRV_SECTOR_SIZE > file_size) {

Do we need to protect against integer overflows here? I think
sinfo.file_offset comes directly from the image file and might be
corrupted.

Or has it already been check somewhere?

> +                /* block is past the end of file, image has been truncated. */
> +                result->corruptions++;

I think we should print an error message like other formats do, so that
the user knows which kind of corruption 'qemu-img check' found (include
the guest and host offset of the invalid block).

> +            }
> +        }
> +    }
> +
>      return 0;
>  }

Kevin

Am 02.09.19 um 15:07 schrieb Kevin Wolf:
> Am 29.08.2019 um 15:36 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 to vhdx_co_check.
>>
>> Signed-off-by: Jan-Hendrik Frintrop <jhf@kamp.de>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/vhdx.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/block/vhdx.c b/block/vhdx.c
>> index 6a09d0a55c..4382b1375d 100644
>> --- a/block/vhdx.c
>> +++ b/block/vhdx.c
>> @@ -2068,10 +2068,29 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
>>                                         BdrvCheckMode fix)
>>   {
>>       BDRVVHDXState *s = bs->opaque;
>> +    VHDXSectorInfo sinfo;
>> +    int64_t file_size = bdrv_get_allocated_file_size(bs);
> Don't you mean bdrv_getlength()?
>
> bdrv_get_allocated_file_size() is only the allocated size, i.e. without
> holes. So a higher offset may actually be present.


Isn't bdrv_getlength the virtual disk size? I need to check if a block

points to a location after EOF of the underlying physical file.


>
>> +    int64_t sector_num;
>>   
>>       if (s->log_replayed_on_open) {
>>           result->corruptions_fixed++;
>>       }
>> +
>> +    for (sector_num = 0; sector_num < bs->total_sectors;
>> +         sector_num += s->block_size / BDRV_SECTOR_SIZE) {
>> +        int nb_sectors = MIN(bs->total_sectors - sector_num,
>> +                             s->block_size / BDRV_SECTOR_SIZE);
>> +        vhdx_block_translate(s, sector_num, nb_sectors, &sinfo);
>> +        if ((s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) ==
>> +            PAYLOAD_BLOCK_FULLY_PRESENT) {
>> +            if (sinfo.file_offset +
>> +                sinfo.sectors_avail * BDRV_SECTOR_SIZE > file_size) {
> Do we need to protect against integer overflows here? I think
> sinfo.file_offset comes directly from the image file and might be
> corrupted.
>
> Or has it already been check somewhere?


The headers are being checked in vhdx_open.  sinfo.file_offset + sinfo.sectors_avail * BDRV_SECTOR_SIZE

is exactly what is being passed to bdrv_pread when reading from the image file.


>
>> +                /* block is past the end of file, image has been truncated. */
>> +                result->corruptions++;
> I think we should print an error message like other formats do, so that
> the user knows which kind of corruption 'qemu-img check' found (include
> the guest and host offset of the invalid block).


What would be the appropriate way to do this? There is no errp in the

check funtion. Inclunde headers so that error_report() is available?


Thanks,

Peter

Am 02.09.2019 um 15:15 hat Peter Lieven geschrieben:
> Am 02.09.19 um 15:07 schrieb Kevin Wolf:
> > Am 29.08.2019 um 15:36 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 to vhdx_co_check.
> > > 
> > > Signed-off-by: Jan-Hendrik Frintrop <jhf@kamp.de>
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > >   block/vhdx.c | 19 +++++++++++++++++++
> > >   1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > index 6a09d0a55c..4382b1375d 100644
> > > --- a/block/vhdx.c
> > > +++ b/block/vhdx.c
> > > @@ -2068,10 +2068,29 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
> > >                                         BdrvCheckMode fix)
> > >   {
> > >       BDRVVHDXState *s = bs->opaque;
> > > +    VHDXSectorInfo sinfo;
> > > +    int64_t file_size = bdrv_get_allocated_file_size(bs);
> > Don't you mean bdrv_getlength()?
> > 
> > bdrv_get_allocated_file_size() is only the allocated size, i.e. without
> > holes. So a higher offset may actually be present.
> 
> 
> Isn't bdrv_getlength the virtual disk size? I need to check if a block
> points to a location after EOF of the underlying physical file.

Yes, it would have to be bdrv_getlength(bs->file->bs), i.e. call it on
the protocol layer, not on the format layer.

> > > +    int64_t sector_num;
> > >       if (s->log_replayed_on_open) {
> > >           result->corruptions_fixed++;
> > >       }
> > > +
> > > +    for (sector_num = 0; sector_num < bs->total_sectors;
> > > +         sector_num += s->block_size / BDRV_SECTOR_SIZE) {
> > > +        int nb_sectors = MIN(bs->total_sectors - sector_num,
> > > +                             s->block_size / BDRV_SECTOR_SIZE);
> > > +        vhdx_block_translate(s, sector_num, nb_sectors, &sinfo);
> > > +        if ((s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) ==
> > > +            PAYLOAD_BLOCK_FULLY_PRESENT) {
> > > +            if (sinfo.file_offset +
> > > +                sinfo.sectors_avail * BDRV_SECTOR_SIZE > file_size) {
> > Do we need to protect against integer overflows here? I think
> > sinfo.file_offset comes directly from the image file and might be
> > corrupted.
> > 
> > Or has it already been check somewhere?
> 
> 
> The headers are being checked in vhdx_open.  sinfo.file_offset +
> sinfo.sectors_avail * BDRV_SECTOR_SIZE is exactly what is being passed
> to bdrv_pread when reading from the image file.

Fair enough, though if I'm not missing anything, we only check that BAT
entries don't overlap with other regions, not that they aren't too high.
And vhdx_block_translate() doesn't seem to check for overflows either
before it sets sinfo->sectors_avail.

So maybe this is actually a bug that should be fixed in
vhdx_block_translate() so that normal accesses get the fix, too.

> > > +                /* block is past the end of file, image has been truncated. */
> > > +                result->corruptions++;
> > I think we should print an error message like other formats do, so that
> > the user knows which kind of corruption 'qemu-img check' found (include
> > the guest and host offset of the invalid block).
> 
> What would be the appropriate way to do this? There is no errp in the
> check funtion. Inclunde headers so that error_report() is available?

Yes, I think error_report() would be fine.

qcow2 even just uses fprintf(stderr, ...), but maybe that's something we
shouldn't imitate.

Kevin

Am 02.09.19 um 15:46 schrieb Kevin Wolf:
> Am 02.09.2019 um 15:15 hat Peter Lieven geschrieben:
>> Am 02.09.19 um 15:07 schrieb Kevin Wolf:
>>> Am 29.08.2019 um 15:36 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 to vhdx_co_check.
>>>>
>>>> Signed-off-by: Jan-Hendrik Frintrop <jhf@kamp.de>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>    block/vhdx.c | 19 +++++++++++++++++++
>>>>    1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/block/vhdx.c b/block/vhdx.c
>>>> index 6a09d0a55c..4382b1375d 100644
>>>> --- a/block/vhdx.c
>>>> +++ b/block/vhdx.c
>>>> @@ -2068,10 +2068,29 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
>>>>                                          BdrvCheckMode fix)
>>>>    {
>>>>        BDRVVHDXState *s = bs->opaque;
>>>> +    VHDXSectorInfo sinfo;
>>>> +    int64_t file_size = bdrv_get_allocated_file_size(bs);
>>> Don't you mean bdrv_getlength()?
>>>
>>> bdrv_get_allocated_file_size() is only the allocated size, i.e. without
>>> holes. So a higher offset may actually be present.
>>
>> Isn't bdrv_getlength the virtual disk size? I need to check if a block
>> points to a location after EOF of the underlying physical file.
> Yes, it would have to be bdrv_getlength(bs->file->bs), i.e. call it on
> the protocol layer, not on the format layer.
>
>>>> +    int64_t sector_num;
>>>>        if (s->log_replayed_on_open) {
>>>>            result->corruptions_fixed++;
>>>>        }
>>>> +
>>>> +    for (sector_num = 0; sector_num < bs->total_sectors;
>>>> +         sector_num += s->block_size / BDRV_SECTOR_SIZE) {
>>>> +        int nb_sectors = MIN(bs->total_sectors - sector_num,
>>>> +                             s->block_size / BDRV_SECTOR_SIZE);
>>>> +        vhdx_block_translate(s, sector_num, nb_sectors, &sinfo);
>>>> +        if ((s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) ==
>>>> +            PAYLOAD_BLOCK_FULLY_PRESENT) {
>>>> +            if (sinfo.file_offset +
>>>> +                sinfo.sectors_avail * BDRV_SECTOR_SIZE > file_size) {
>>> Do we need to protect against integer overflows here? I think
>>> sinfo.file_offset comes directly from the image file and might be
>>> corrupted.
>>>
>>> Or has it already been check somewhere?
>>
>> The headers are being checked in vhdx_open.  sinfo.file_offset +
>> sinfo.sectors_avail * BDRV_SECTOR_SIZE is exactly what is being passed
>> to bdrv_pread when reading from the image file.
> Fair enough, though if I'm not missing anything, we only check that BAT
> entries don't overlap with other regions, not that they aren't too high.
> And vhdx_block_translate() doesn't seem to check for overflows either
> before it sets sinfo->sectors_avail.
>
> So maybe this is actually a bug that should be fixed in
> vhdx_block_translate() so that normal accesses get the fix, too.


Or maybe already or also check in vhdx_open when we already iterate over all BAT entries?

vhdx_block_translate cannot return an error at the moment.


Peter

Am 02.09.2019 um 16:17 hat Peter Lieven geschrieben:
> Am 02.09.19 um 15:46 schrieb Kevin Wolf:
> > Am 02.09.2019 um 15:15 hat Peter Lieven geschrieben:
> > > Am 02.09.19 um 15:07 schrieb Kevin Wolf:
> > > > Am 29.08.2019 um 15:36 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 to vhdx_co_check.
> > > > > 
> > > > > Signed-off-by: Jan-Hendrik Frintrop <jhf@kamp.de>
> > > > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > > > ---
> > > > >    block/vhdx.c | 19 +++++++++++++++++++
> > > > >    1 file changed, 19 insertions(+)
> > > > > 
> > > > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > > > index 6a09d0a55c..4382b1375d 100644
> > > > > --- a/block/vhdx.c
> > > > > +++ b/block/vhdx.c
> > > > > @@ -2068,10 +2068,29 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
> > > > >                                          BdrvCheckMode fix)
> > > > >    {
> > > > >        BDRVVHDXState *s = bs->opaque;
> > > > > +    VHDXSectorInfo sinfo;
> > > > > +    int64_t file_size = bdrv_get_allocated_file_size(bs);
> > > > Don't you mean bdrv_getlength()?
> > > > 
> > > > bdrv_get_allocated_file_size() is only the allocated size, i.e. without
> > > > holes. So a higher offset may actually be present.
> > > 
> > > Isn't bdrv_getlength the virtual disk size? I need to check if a block
> > > points to a location after EOF of the underlying physical file.
> > Yes, it would have to be bdrv_getlength(bs->file->bs), i.e. call it on
> > the protocol layer, not on the format layer.
> > 
> > > > > +    int64_t sector_num;
> > > > >        if (s->log_replayed_on_open) {
> > > > >            result->corruptions_fixed++;
> > > > >        }
> > > > > +
> > > > > +    for (sector_num = 0; sector_num < bs->total_sectors;
> > > > > +         sector_num += s->block_size / BDRV_SECTOR_SIZE) {
> > > > > +        int nb_sectors = MIN(bs->total_sectors - sector_num,
> > > > > +                             s->block_size / BDRV_SECTOR_SIZE);
> > > > > +        vhdx_block_translate(s, sector_num, nb_sectors, &sinfo);
> > > > > +        if ((s->bat[sinfo.bat_idx] & VHDX_BAT_STATE_BIT_MASK) ==
> > > > > +            PAYLOAD_BLOCK_FULLY_PRESENT) {
> > > > > +            if (sinfo.file_offset +
> > > > > +                sinfo.sectors_avail * BDRV_SECTOR_SIZE > file_size) {
> > > > Do we need to protect against integer overflows here? I think
> > > > sinfo.file_offset comes directly from the image file and might be
> > > > corrupted.
> > > > 
> > > > Or has it already been check somewhere?
> > > 
> > > The headers are being checked in vhdx_open.  sinfo.file_offset +
> > > sinfo.sectors_avail * BDRV_SECTOR_SIZE is exactly what is being passed
> > > to bdrv_pread when reading from the image file.
> > Fair enough, though if I'm not missing anything, we only check that BAT
> > entries don't overlap with other regions, not that they aren't too high.
> > And vhdx_block_translate() doesn't seem to check for overflows either
> > before it sets sinfo->sectors_avail.
> > 
> > So maybe this is actually a bug that should be fixed in
> > vhdx_block_translate() so that normal accesses get the fix, too.
> 
> Or maybe already or also check in vhdx_open when we already iterate
> over all BAT entries?
> 
> vhdx_block_translate cannot return an error at the moment.

Hm, makes sense, yes.

However, in vhdx_open(), it means that 'qemu-img check' won't even be
able to open the image, so we don't properly report this as an error,
but just as "can't open".

Of course, this is already true for the existing checks, so I don't want
to require you to fix this now (probably by skipping the checks there
with BDRV_O_CHECK and instead calling them from .bdrv_co_check) when
you're trying to fix something mostly unrelated. But if you'd like to, I
certainly wouldn't mind.

Kevin