Re: [Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature

[Stefan Hajnoczi]

On Mon, Jun 08, 2015 at 06:21:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:

I haven't fully reviewed this patch yet but here are initial comments.

> +int qcow2_read_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    QCowDirtyBitmapHeader h;
> +    QCowDirtyBitmap *bm;
> +    int i, name_size;
> +    int64_t offset;
> +    int ret;
> +
> +    if (!s->nb_dirty_bitmaps) {
> +        s->dirty_bitmaps = NULL;
> +        s->dirty_bitmaps_size = 0;
> +        return 0;
> +    }
> +
> +    offset = s->dirty_bitmaps_offset;
> +    s->dirty_bitmaps = g_new0(QCowDirtyBitmap, s->nb_dirty_bitmaps);
> +
> +    for (i = 0; i < s->nb_dirty_bitmaps; i++) {
> +        /* Read statically sized part of the dirty_bitmap header */
> +        offset = align_offset(offset, 8);
> +        ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +
> +        offset += sizeof(h);
> +        bm = s->dirty_bitmaps + i;
> +        bm->l1_table_offset = be64_to_cpu(h.l1_table_offset);
> +        bm->l1_size = be32_to_cpu(h.l1_size);
> +        bm->bitmap_granularity = be32_to_cpu(h.bitmap_granularity);
> +        bm->bitmap_size = be64_to_cpu(h.bitmap_size);

Input validation is missing.  These could be junk values.  Min, max,
alignment, etc need to be checked.

> @@ -90,6 +91,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>      QCowExtension ext;
>      uint64_t offset;
>      int ret;
> +    Qcow2DirtyBitmapHeaderExt dirty_bitmaps_ext;
>  
>  #ifdef DEBUG_EXT
>      printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, 
> end_offset);
> @@ -160,6 +162,33 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>              }
>              break;
>  
> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
> +            ret = bdrv_pread(bs->file, offset, &dirty_bitmaps_ext, ext.len);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "ERROR: dirty_bitmaps_ext: "
> +                                 "Could not read ext header");
> +                return ret;
> +            }
> +
> +            be64_to_cpus(&dirty_bitmaps_ext.dirty_bitmaps_offset);
> +            be32_to_cpus(&dirty_bitmaps_ext.nb_dirty_bitmaps);
> +
> +            s->dirty_bitmaps_offset = dirty_bitmaps_ext.dirty_bitmaps_offset;
> +            s->nb_dirty_bitmaps = dirty_bitmaps_ext.nb_dirty_bitmaps;
> +
> +            ret = qcow2_read_dirty_bitmaps(bs);

Missing input validation.  We cannot trust dirty_bitmaps_offset or
nb_dirty_bitmaps.

pgpTpXDSKPR2A.pgp
Description: PGP signature