Re: [Qemu-devel] [PATCH 4/4] qcow2: Check L1/L2/reftable entries for alignment

[Kevin Wolf <kwolf@redhat.com>]

Am 16.08.2014 um 23:16 hat Max Reitz geschrieben:
> Offsets taken from the L1, L2 and refcount tables are generally assumed
> to be correctly aligned. However, this cannot be guaranteed if the image
> has been written to by something different than qemu, thus check all
> offsets taken from these tables for correct cluster alignment.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c  | 27 ++++++++++++++++++++++++++-
>  block/qcow2-refcount.c | 36 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 60 insertions(+), 3 deletions(-)

Can you extend qemu-iotests 060 to check each of these cases?

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 5b36018..2cc41b2 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -486,6 +486,12 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>          goto out;
>      }
>  
> +    if (offset_into_cluster(s, l2_offset)) {
> +        qcow2_signal_corruption(bs, -1, -1, "L2 table offset %#" PRIx64
> +                                " unaligned", l2_offset);

Should we include l1_index in the message? If you want to debug the
image with a hex editor or something, this is important information.

> +        return -EIO;
> +    }
> +
>      /* load the l2 table in memory */
>  
>      ret = l2_load(bs, l2_offset, &l2_table);
> @@ -525,6 +531,12 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>          c = count_contiguous_clusters(nb_clusters, s->cluster_size,
>                  &l2_table[l2_index], QCOW_OFLAG_ZERO);
>          *cluster_offset &= L2E_OFFSET_MASK;
> +        if (offset_into_cluster(s, *cluster_offset)) {
> +            qcow2_signal_corruption(bs, -1, -1, "Data cluster offset %#" PRIx64
> +                                    " unaligned", *cluster_offset);

The same thing here would be offset.

> +            qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
> +            return -EIO;
> +        }

I wonder whether a goto fail would start to make sense now, zero
clusters in v2 images have the same qcow2_cache_put/return -EIO code.

And actually, that's a corruption case as well, so we might call
qcow2_signal_corruption() there.

>          break;
>      default:
>          abort();
> @@ -576,6 +588,11 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
>  
>      assert(l1_index < s->l1_size);
>      l2_offset = s->l1_table[l1_index] & L1E_OFFSET_MASK;
> +    if (offset_into_cluster(s, l2_offset)) {
> +        qcow2_signal_corruption(bs, -1, -1, "L2 table offset %#" PRIx64
> +                                " unaligned", l2_offset);

l1_index again.

> +        return -EIO;
> +    }
>  
>      /* seek the l2 table of the given l2 offset */
>  
> @@ -948,6 +965,14 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>          bool offset_matches =
>              (cluster_offset & L2E_OFFSET_MASK) == *host_offset;
>  
> +        if (offset_into_cluster(s, cluster_offset & L2E_OFFSET_MASK)) {
> +            qcow2_signal_corruption(bs, -1, -1, "Data cluster offset %#llx "
> +                                    "unaligned",
> +                                    cluster_offset & L2E_OFFSET_MASK);

Worth adding guest_offset.

> +            ret = -EIO;
> +            goto out;
> +        }
> +
>          if (*host_offset != 0 && !offset_matches) {
>              *bytes = 0;
>              ret = 0;
> @@ -979,7 +1004,7 @@ out:
>  
>      /* Only return a host offset if we actually made progress. Otherwise we
>       * would make requirements for handle_alloc() that it can't fulfill */
> -    if (ret) {
> +    if (ret > 0) {
>          *host_offset = (cluster_offset & L2E_OFFSET_MASK)
>                       + offset_into_cluster(s, guest_offset);
>      }
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 0ac1339..fac2963 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -108,6 +108,12 @@ static int get_refcount(BlockDriverState *bs, int64_t cluster_index)
>      if (!refcount_block_offset)
>          return 0;
>  
> +    if (offset_into_cluster(s, refcount_block_offset)) {
> +        qcow2_signal_corruption(bs, -1, -1, "Refblock offset %#" PRIx64
> +                                " unaligned", refcount_block_offset);

Add refcount_table_index.

> +        return -EIO;
> +    }
> +
>      ret = qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset,
>          (void**) &refcount_block);
>      if (ret < 0) {
> @@ -181,6 +187,12 @@ static int alloc_refcount_block(BlockDriverState *bs,
>  
>          /* If it's already there, we're done */
>          if (refcount_block_offset) {
> +            if (offset_into_cluster(s, refcount_block_offset)) {
> +                qcow2_signal_corruption(bs, -1, -1, "Refblock offset %#" PRIx64
> +                                        " unaligned", refcount_block_offset);
> +                return -EIO;
> +            }

Same here.

>               return load_refcount_block(bs, refcount_block_offset,
>                   (void**) refcount_block);
>          }
> @@ -836,8 +848,13 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
>      case QCOW2_CLUSTER_NORMAL:
>      case QCOW2_CLUSTER_ZERO:
>          if (l2_entry & L2E_OFFSET_MASK) {
> -            qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
> -                                nb_clusters << s->cluster_bits, type);
> +            if (offset_into_cluster(s, l2_entry & L2E_OFFSET_MASK)) {
> +                fprintf(stderr, "qcow2: Cannot free unaligned cluster %#llx\n",
> +                        l2_entry & L2E_OFFSET_MASK);
> +            } else {
> +                qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
> +                                    nb_clusters << s->cluster_bits, type);
> +            }

Hm... Why isn't this a corruption like any other? Unconditional
fprintf() is something I don't like a lot.

>          }
>          break;
>      case QCOW2_CLUSTER_UNALLOCATED:
> @@ -901,6 +918,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>              old_l2_offset = l2_offset;
>              l2_offset &= L1E_OFFSET_MASK;
>  
> +            if (offset_into_cluster(s, l2_offset)) {
> +                qcow2_signal_corruption(bs, -1, -1, "L2 table offset %#" PRIx64
> +                                        " unaligned", l2_offset);
> +                ret = -EIO;
> +                goto fail;
> +            }

Add the L1 index (i) to the message.

>              ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
>                  (void**) &l2_table);
>              if (ret < 0) {
> @@ -933,6 +957,14 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>  
>                      case QCOW2_CLUSTER_NORMAL:
>                      case QCOW2_CLUSTER_ZERO:
> +                        if (offset_into_cluster(s, offset & L2E_OFFSET_MASK)) {
> +                            qcow2_signal_corruption(bs, -1, -1, "Data cluster "
> +                                                    "offset %#llx unaligned",
> +                                                    offset & L2E_OFFSET_MASK);

We don't have a single index describing the cluster here, so you might
either just print both L1 and L2 index or calculate a cluster index. The
former is probably easier and even more useful.

> +                            ret = -EIO;
> +                            goto fail;
> +                        }
> +
>                          cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits;
>                          if (!cluster_index) {
>                              /* unallocated */

Kevin