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

[Max Reitz <mreitz@redhat.com>]

On 20.08.2014 12:51, Kevin Wolf wrote:
> 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?

I'll do my very best.

>> 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.

Probably so, yes.

>> +        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.

I guess writing a fail path would result in more lines of code overall, 
but deduplicated longer code is probably better than shorter code with 
duplications, so why not. ;-)


>>           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.

We already do it in qcow2_free_clusters().

I decided not to make it a corruption because we don't lose anything. 
The entry is corrupted, but we don't need it anymore anyway; it's 
overwritten with 0 and wherever the cluster might have been meant to be 
located, it doesn't matter anymore.

>>           }
>>           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

Thanks for all of your reviews!

Max