Re: [Qemu-devel] [PATCH 13/22] qcow2-bitmap: check constraints

[Max Reitz <mreitz@redhat.com>]

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

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Check bitmap header constraints as specified in docs/specs/qcow2.txt
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)

I'd pull this patch to some previous point in the series because the
previous patches would already require you to check these constraints
(which you just haven't done until now).

> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 8cf40f0..1c3abea 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -154,6 +154,34 @@ static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
>      }
>  }
>  
> +static int check_constraints(BlockDriverState *bs, Qcow2BitmapDirEntry *h)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t phys_bitmap_bytes =
> +        (uint64_t)h->bitmap_table_size * s->cluster_size;
> +    uint64_t max_virtual_bits = (phys_bitmap_bytes * 8) << h->granularity_bits;
> +    int64_t nb_sectors = bdrv_nb_sectors(bs);
> +
> +    if (nb_sectors < 0) {
> +        return nb_sectors;
> +    }
> +
> +    int fail =
> +            ((h->bitmap_table_size == 0) != (h->bitmap_table_offset == 0)) ||
> +            (h->bitmap_table_offset % s->cluster_size) ||
> +            (h->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
> +            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> +            (h->bitmap_table_offset != 0 &&
> +                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
> +            (h->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
> +            (h->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
> +            (h->flags & BME_RESERVED_FLAGS) ||
> +            (h->name_size > BME_MAX_NAME_SIZE) ||
> +            (h->type != BT_DIRTY_TRACKING_BITMAP);
> +
> +    return fail ? -EINVAL : 0;
> +}
> +
>  static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
>                                 uint32_t bitmap_table_size)
>  {
> @@ -372,6 +400,12 @@ static uint8_t *directory_read(BlockDriverState *bs,
>                         bdrv_get_device_or_node_name(bs));
>              goto fail;
>          }
> +
> +        ret = check_constraints(bs, e);
> +        if (ret < 0) {
> +            error_setg(errp, "Bitmap doesn't satisfy the constraints.");

I think I'd at least mention the name of the bitmap; also, no full stop
at the end of error messages.

> +            goto fail;
> +        }
>      }
>  
>      assert((uint8_t *)e == dir_end);
> @@ -713,6 +747,11 @@ static int store_bitmap(BlockDriverState *bs,
>      entry->extra_data_size = 0;
>      memcpy(entry + 1, bm_name, entry->name_size);
>  
> +    ret = check_constraints(bs, entry);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +

As I said in my second reply to patch 9, I think it's a bit too late if
we detect that the bitmap is actually invalid at this point. We really
should notice earlier.

Apart from what would actually better for the user, it is actually too
late to check the constraints here, as you have already written the
bitmap data to disk. You should always check the constraints before
reading and also before writing, not afterwards.

Max

>      return 0;
>  
>  fail:
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]