Re: [Qemu-devel] [PATCH] block/vhdx: add check for truncated image files

[Kevin Wolf <kwolf@redhat.com>]

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