Re: [Qemu-devel] [PATCH 2/5] qcow2: Metadata overlap checks

[Kevin Wolf]

Am 26.08.2013 um 15:04 hat Max Reitz geschrieben:
> Two new functions are added; the first one checks a given range in the
> image file for overlaps with metadata (main header, L1 tables, L2
> tables, refcount table and blocks).
> 
> The second one should be used immediately before writing to the image
> file as it calls the first function and, upon collision, marks the
> image as corrupt and makes the BDS unusable, thereby preventing
> further access.
> 
> Both functions take a bitmask argument specifying the structures which
> should be checked for overlaps, making it possible to also check
> metadata writes against colliding with other structures.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-refcount.c | 142 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h          |  28 ++++++++++
>  2 files changed, 170 insertions(+)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 1244693..c8141c8 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -25,6 +25,7 @@
>  #include "qemu-common.h"
>  #include "block/block_int.h"
>  #include "block/qcow2.h"
> +#include "qemu/range.h"
>  
>  static int64_t alloc_clusters_noref(BlockDriverState *bs, int64_t size);
>  static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
> @@ -1372,3 +1373,144 @@ fail:
>      return ret;
>  }
>  
> +/*
> + * Checks if the given offset into the image file is actually free to use by
> + * looking for overlaps with important metadata sections (L1/L2 tables etc.),
> + * i.e. a sanity check without relying on the refcount tables.
> + *
> + * The chk parameter specifies exactly what checks to perform.
> + *
> + * Returns:
> + * - 0 if writing to this offset will not affect the mentioned metadata
> + * - a positive QCow2MetadataOverlap value indicating one overlapping section
> + * - a negative value (-errno) indicating an error while performing a check,
> + *   e.g. when bdrv_read failed on QCOW2_OL_INACTIVE_L2
> + */
> +int qcow2_check_metadata_overlap(BlockDriverState *bs, QCow2MetadataOverlap 
> chk,

chk is really just an int, because you don't pass a single enum value but
a bit mask consisting of multiple enum values ored together.

> +                                 int64_t offset, int64_t size)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int i, j;
> +
> +    if (!size) {
> +        return 0;
> +    }
> +
> +    if (chk & QCOW2_OL_MAIN_HEADER) {
> +        if (offset < s->cluster_size) {
> +            return QCOW2_OL_MAIN_HEADER;
> +        }
> +    }
> +
> +    if ((chk & QCOW2_OL_ACTIVE_L1) && s->l1_size) {
> +        if (ranges_overlap(offset, size, s->l1_table_offset,
> +            s->l1_size * sizeof(uint64_t))) {

The size could be rounded up to the next cluster boundary (same thing
for other metadata types).

> +            return QCOW2_OL_ACTIVE_L1;
> +        }
> +    }
> +
> +    if ((chk & QCOW2_OL_REFCOUNT_TABLE) && s->refcount_table_size) {
> +        if (ranges_overlap(offset, size, s->refcount_table_offset,
> +            s->refcount_table_size * sizeof(uint64_t))) {
> +            return QCOW2_OL_REFCOUNT_TABLE;
> +        }
> +    }
> +
> +    if ((chk & QCOW2_OL_SNAPSHOT_TABLE) && s->snapshots_size) {
> +        if (ranges_overlap(offset, size, s->snapshots_offset,
> +            s->snapshots_size)) {
> +            return QCOW2_OL_SNAPSHOT_TABLE;
> +        }
> +    }
> +
> +    if ((chk & QCOW2_OL_INACTIVE_L1) && s->snapshots) {
> +        for (i = 0; i < s->nb_snapshots; i++) {
> +            if (s->snapshots[i].l1_size &&
> +                ranges_overlap(offset, size, s->snapshots[i].l1_table_offset,
> +                s->snapshots[i].l1_size * sizeof(uint64_t))) {
> +                return QCOW2_OL_INACTIVE_L1;
> +            }
> +        }
> +    }
> +
> +    if ((chk & QCOW2_OL_ACTIVE_L2) && s->l1_table) {
> +        for (i = 0; i < s->l1_size; i++) {
> +            if ((s->l1_table[i] & L1E_OFFSET_MASK) &&
> +                ranges_overlap(offset, size, s->l1_table[i] & 
> L1E_OFFSET_MASK,
> +                s->cluster_size)) {
> +                return QCOW2_OL_ACTIVE_L2;
> +            }
> +        }
> +    }
> +
> +    if ((chk & QCOW2_OL_REFCOUNT_BLOCK) && s->refcount_table) {
> +        for (i = 0; i < s->refcount_table_size; i++) {
> +            if ((s->refcount_table[i] & REFT_OFFSET_MASK) &&
> +                ranges_overlap(offset, size, s->refcount_table[i] &
> +                REFT_OFFSET_MASK, s->cluster_size)) {
> +                return QCOW2_OL_REFCOUNT_BLOCK;
> +            }
> +        }
> +    }
> +
> +    if ((chk & QCOW2_OL_INACTIVE_L2) && s->snapshots) {
> +        for (i = 0; i < s->nb_snapshots; i++) {
> +            uint64_t l1_ofs = s->snapshots[i].l1_table_offset;
> +            uint32_t l1_sz  = s->snapshots[i].l1_size;
> +            uint64_t *l1 = g_malloc(l1_sz * sizeof(uint64_t));
> +            int ret;
> +
> +            ret = bdrv_read(bs->file, l1_ofs / BDRV_SECTOR_SIZE, (uint8_t 
> *)l1,
> +                            l1_sz * sizeof(uint64_t) / BDRV_SECTOR_SIZE);
> +
> +            if (ret < 0) {
> +                g_free(l1);
> +                return ret;
> +            }
> +
> +            for (j = 0; j < l1_sz; j++) {
> +                if ((l1[j] & L1E_OFFSET_MASK) &&
> +                    ranges_overlap(offset, size, l1[j] & L1E_OFFSET_MASK,
> +                    s->cluster_size)) {
> +                    g_free(l1);
> +                    return QCOW2_OL_INACTIVE_L2;
> +                }
> +            }
> +
> +            g_free(l1);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * First performs a check for metadata overlaps (through
> + * qcow2_check_metadata_overlap); if that fails with a negative value (error
> + * while performing a check), it will print a message but otherwise ignore 
> that
> + * error. If an impending overlap is detected, the BDS will be made unusable 
> and
> + * the qcow2 file marked corrupt.
> + *
> + * Returns 0 if there were no overlaps (or an error occured while checking 
> for
> + * overlaps) or a positive QCow2MetadataOverlap value on overlap (then, the 
> BDS
> + * will be unusable and the qcow2 file marked corrupt).
> + */
> +int qcow2_pre_write_overlap_check(BlockDriverState *bs, QCow2MetadataOverlap 
> chk,
> +                                  int64_t offset, int64_t size)
> +{
> +    int ret = qcow2_check_metadata_overlap(bs, chk, offset, size);
> +
> +    if (ret < 0) {
> +        fprintf(stderr, "qcow2: Error while checking for metadata overlaps: "
> +                "%s\n", strerror(-ret));

Leftover debug code?

> +        return ret;
> +    } else if (ret > 0) {
> +        fprintf(stderr, "qcow2: Preventing invalid write on metadata; "
> +                "image marked as corrupt.\n");

This one makes actually sense to keep even for production as it is a
condition that we want to make sure to appear in log files.

Another thing to consider would be to send out a QMP event when this
happens.

> +        qcow2_mark_corrupt(bs);
> +        bs->drv = NULL; /* make BDS unusable */
> +        return ret;
> +    }
> +
> +    return 0;
> +}

Kevin