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

[Max Reitz]

On 08.09.2014 16:40, Benoît Canet wrote:
> The Friday 05 Sep 2014 à 16:07:18 (+0200), Max Reitz wrote :
> > 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 <address@hidden>
> > ---
> >   block/qcow2-cluster.c  | 43 ++++++++++++++++++++++++++++++++++++++++---
> >   block/qcow2-refcount.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> >   2 files changed, 82 insertions(+), 5 deletions(-)
> >
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 735f687..f7dd8c0 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -486,6 +486,13 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
> > uint64_t offset,
> >           goto out;
> >       }
> >   
> > +    if (offset_into_cluster(s, l2_offset)) {
> > +        qcow2_signal_corruption(bs, true, -1, -1, "L2 table offset %#" PRIx64
> > +                                " unaligned (L1 index: %#" PRIx64 ")",
> > +                                l2_offset, l1_index);
> > +        return -EIO;
> This function mix return ret and goto out and there is more of the second.
> Can we do ret = -EIO and goto out for consistency ?
> bs->drv == NULL after qcow2_signal_corruption so we are not afraid of out
> sides effects.

The "out" label here is for success; that's why I introduced the "fail" 
label in this series. I could make qcow2_cache_put() in the fail path 
optional and then use goto fail, though. But this would only increase 
the code size with no real benefit apparent to me (no code 
deduplication; and as far as I remember, we have many functions with 
fail labels which however use a plain "return" before cleaning up is 
needed).

(before this patch, there were two places using "goto out" in this 
function, both of which were "successes" (cluster found to be 
unallocated)); and two places using "return -errno", both of which were 
failures (the first one due to l2_load() failing and the second one due 
to a zero cluster found in a pre-v3 image))

Max

> > +    }
> > +
> >       /* load the l2 table in memory */
> >   
> >       ret = l2_load(bs, l2_offset, &l2_table);
> > @@ -508,8 +515,11 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
> > uint64_t offset,
> >           break;
> >       case QCOW2_CLUSTER_ZERO:
> >           if (s->qcow_version < 3) {
> > -            qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
> > -            return -EIO;
> > +            qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry 
> > found"
> > +                                    " in pre-v3 image (L2 offset: %#" PRIx64
> > +                                    ", L2 index: %#x)", l2_offset, l2_index);
> > +            ret = -EIO;
> > +            goto fail;
> >           }
> >           c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> >                   &l2_table[l2_index], QCOW_OFLAG_ZERO);
> > @@ -525,6 +535,14 @@ 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, true, -1, -1, "Data cluster offset %#"
> > +                                    PRIx64 " unaligned (L2 offset: %#" PRIx64
> > +                                    ", L2 index: %#x)", *cluster_offset,
> > +                                    l2_offset, l2_index);
> > +            ret = -EIO;
> > +            goto fail;
> > +        }
> >           break;
> >       default:
> >           abort();
> > @@ -541,6 +559,10 @@ out:
> >       *num = nb_available - index_in_cluster;
> >   
> >       return ret;
> > +
> > +fail:
> > +    qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
> > +    return ret;
> >   }
> >   
> >   /*
> > @@ -576,6 +598,12 @@ 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, true, -1, -1, "L2 table offset %#" PRIx64
> > +                                " unaligned (L1 index: %#" PRIx64 ")",
> > +                                l2_offset, l1_index);
> > +        return -EIO;
> > +    }
> >   
> >       /* seek the l2 table of the given l2 offset */
> >   
> > @@ -948,6 +976,15 @@ 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, true, -1, -1, "Data cluster offset "
> > +                                    "%#llx unaligned (guest offset: %#" PRIx64
> > +                                    ")", cluster_offset & L2E_OFFSET_MASK,
> > +                                    guest_offset);
> > +            ret = -EIO;
> > +            goto out;
> > +        }
> > +
> >           if (*host_offset != 0 && !offset_matches) {
> >               *bytes = 0;
> >               ret = 0;
> > @@ -979,7 +1016,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 b9d421e..2bcaaf9 100644
> > --- a/block/qcow2-refcount.c
> > +++ b/block/qcow2-refcount.c
> > @@ -108,6 +108,13 @@ 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, true, -1, -1, "Refblock offset %#" PRIx64
> > +                                " unaligned (reftable index: %#" PRIx64 ")",
> > +                                refcount_block_offset, 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 +188,14 @@ 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, true, -1, -1, "Refblock offset %#"
> > +                                        PRIx64 " unaligned (reftable index: "
> > +                                        "%#x)", refcount_block_offset,
> > +                                        refcount_table_index);
> > +                return -EIO;
> > +            }
> > +
> >                return load_refcount_block(bs, refcount_block_offset,
> >                    (void**) refcount_block);
> >           }
> > @@ -836,8 +851,14 @@ 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)) {
> > +                qcow2_signal_corruption(bs, false, -1, -1,
> > +                                        "Cannot free unaligned cluster %#llx",
> > +                                        l2_entry & L2E_OFFSET_MASK);
> > +            } else {
> > +                qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
> > +                                    nb_clusters << s->cluster_bits, type);
> > +            }
> >           }
> >           break;
> >       case QCOW2_CLUSTER_UNALLOCATED:
> > @@ -901,6 +922,14 @@ 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, true, -1, -1, "L2 table offset %#"
> > +                                        PRIx64 " unaligned (L1 index: %#x)",
> > +                                        l2_offset, i);
> > +                ret = -EIO;
> > +                goto fail;
> > +            }
> > +
> >               ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
> >                   (void**) &l2_table);
> >               if (ret < 0) {
> > @@ -933,6 +962,17 @@ 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, true, -1, -1, "Data "
> > +                                                    "cluster offset %#llx "
> > +                                                    "unaligned (L2 offset: %#"
> > +                                                    PRIx64 ", L2 index: %#x)",
> > +                                                    offset & L2E_OFFSET_MASK,
> > +                                                    l2_offset, j);
> > +                            ret = -EIO;
> > +                            goto fail;
> > +                        }
> > +
> >                           cluster_index = (offset & L2E_OFFSET_MASK) >> 
> > s->cluster_bits;
> >                           if (!cluster_index) {
> >                               /* unallocated */
> > --
> > 2.1.0