[Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Max Reitz]

With the series adding unalignment checks and the series reworking the
zero cluster expansion code overlapping, the unalignment checks have not
been implemented in the latter code.

This series fixes it.

There are other places which would require unalignment checks, like the
offsets of L1 tables, especially for snapshots; but because it would be
best to add these checks in the function which reads the snapshot table,
this would make images with broken snapshots completely unusable, which
is something I opted to avoid for now.

Ideally, we need to make the qcow2 repair function repair such cases,
but until that is done there is not much we can do about them.


Max Reitz (2):
  qcow2: Add two more unalignment checks
  iotests: Add tests for more corruption cases

 block/qcow2-cluster.c      | 21 +++++++++++++++++++++
 tests/qemu-iotests/060     | 15 +++++++++++++++
 tests/qemu-iotests/060.out | 13 +++++++++++++
 3 files changed, 49 insertions(+)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] qcow2: Add two more unalignment checks

[Max Reitz]

This adds checks for unaligned L2 table offsets and unaligned data
cluster offsets (actually the preallocated offsets for zero clusters) to
the zero cluster expansion function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1fea514..183177d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1651,6 +1651,14 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
             continue;
         }
 
+        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;
+        }
+
         if (is_active_l1) {
             /* get active L2 tables from cache */
             ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
@@ -1709,6 +1717,19 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                 }
             }
 
+            if (offset_into_cluster(s, offset)) {
+                qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset "
+                                        "%#" PRIx64 " unaligned (L2 offset: %#"
+                                        PRIx64 ", L2 index: %#x)", offset,
+                                        l2_offset, j);
+                if (!preallocated) {
+                    qcow2_free_clusters(bs, offset, s->cluster_size,
+                                        QCOW2_DISCARD_ALWAYS);
+                }
+                ret = -EIO;
+                goto fail;
+            }
+
             ret = qcow2_pre_write_overlap_check(bs, 0, offset, s->cluster_size);
             if (ret < 0) {
                 if (!preallocated) {
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] iotests: Add tests for more corruption cases

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/060     | 15 +++++++++++++++
 tests/qemu-iotests/060.out | 13 +++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 73863bf..c81319c 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -186,6 +186,12 @@ $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
 poke_file "$TEST_IMG" "$l1_offset" "\x80\x00\x00\x00\x00\x04\x2a\x00"
 $QEMU_IO -c "read 0 64k" "$TEST_IMG" | _filter_qemu_io
 
+# Test how well zero cluster expansion can cope with this
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$l1_offset" "\x80\x00\x00\x00\x00\x04\x2a\x00"
+$QEMU_IMG amend -o compat=0.10 "$TEST_IMG"
+
 echo
 echo "=== Testing unaligned L2 entry ==="
 echo
@@ -195,6 +201,15 @@ poke_file "$TEST_IMG" "$l2_offset" "\x80\x00\x00\x00\x00\x05\x2a\x00"
 $QEMU_IO -c "read 0 64k" "$TEST_IMG" | _filter_qemu_io
 
 echo
+echo "=== Testing unaligned pre-allocated zero cluster ==="
+echo
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$l2_offset" "\x80\x00\x00\x00\x00\x05\x2a\x01"
+# zero cluster expansion
+$QEMU_IMG amend -o compat=0.10 "$TEST_IMG"
+
+echo
 echo "=== Testing unaligned reftable entry ==="
 echo
 _make_test_img 64M
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 7d493bb..dc9f6b7 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -123,6 +123,11 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: L2 table offset 0x42a00 unaligned (L1 index: 0); further corruption events will be suppressed
 read failed: Input/output error
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: L2 table offset 0x42a00 unaligned (L1 index: 0); further corruption events will be suppressed
+qemu-img: Error while amending options: Input/output error
 
 === Testing unaligned L2 entry ===
 
@@ -132,6 +137,14 @@ wrote 65536/65536 bytes at offset 0
 qcow2: Marking image as corrupt: Data cluster offset 0x52a00 unaligned (L2 offset: 0x40000, L2 index: 0); further corruption events will be suppressed
 read failed: Input/output error
 
+=== Testing unaligned pre-allocated zero cluster ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Data cluster offset 0x52a00 unaligned (L2 offset: 0x40000, L2 index: 0); further corruption events will be suppressed
+qemu-img: Error while amending options: Input/output error
+
 === Testing unaligned reftable entry ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

On 01/19/2015 01:49 PM, Max Reitz wrote:
> With the series adding unalignment checks and the series reworking the
> zero cluster expansion code overlapping, the unalignment checks have not
> been implemented in the latter code.
> 
> This series fixes it.
> 
> There are other places which would require unalignment checks, like the
> offsets of L1 tables, especially for snapshots; but because it would be
> best to add these checks in the function which reads the snapshot table,
> this would make images with broken snapshots completely unusable, which
> is something I opted to avoid for now.
> 
> Ideally, we need to make the qcow2 repair function repair such cases,
> but until that is done there is not much we can do about them.

What's the best repair? Read the data from the unaligned location, and
write a fresh copy into a new aligned allocation?  At any rate, I agree
that repairing unaligned locations is harder than detecting them, so
your series is fine as is.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Max Reitz]

On 2015-01-19 at 16:04, Eric Blake wrote:
> On 01/19/2015 01:49 PM, Max Reitz wrote:
>> With the series adding unalignment checks and the series reworking the
>> zero cluster expansion code overlapping, the unalignment checks have not
>> been implemented in the latter code.
>>
>> This series fixes it.
>>
>> There are other places which would require unalignment checks, like the
>> offsets of L1 tables, especially for snapshots; but because it would be
>> best to add these checks in the function which reads the snapshot table,
>> this would make images with broken snapshots completely unusable, which
>> is something I opted to avoid for now.
>>
>> Ideally, we need to make the qcow2 repair function repair such cases,
>> but until that is done there is not much we can do about them.
> What's the best repair?

That's what I was asking myself...

> Read the data from the unaligned location, and
> write a fresh copy into a new aligned allocation?

Maybe. Maybe there is no way of repairing them and the only way is 
asking the user whether it's fine to delete the snapshot (qemu-img check 
-r make_consistent_whatever_it_takes).

Also, the question remains for every unaligned data structure: L2 
tables, data clusters… The refcount structures are the only ones that 
can be easily recovered. For data clusters, reading from the unaligned 
location would probably be best; for L2 tables… maybe the same, then run 
a consistency check on the data and if it seems off™, throw the whole L2 
table away.

> At any rate, I agree
> that repairing unaligned locations is harder than detecting them, so
> your series is fine as is.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max

Re: [Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Kevin Wolf]

Am 19.01.2015 um 22:09 hat Max Reitz geschrieben:
> On 2015-01-19 at 16:04, Eric Blake wrote:
> >On 01/19/2015 01:49 PM, Max Reitz wrote:
> >>With the series adding unalignment checks and the series reworking the
> >>zero cluster expansion code overlapping, the unalignment checks have not
> >>been implemented in the latter code.
> >>
> >>This series fixes it.
> >>
> >>There are other places which would require unalignment checks, like the
> >>offsets of L1 tables, especially for snapshots; but because it would be
> >>best to add these checks in the function which reads the snapshot table,
> >>this would make images with broken snapshots completely unusable, which
> >>is something I opted to avoid for now.

That's something I noticed, too: For qemu-img check, our qcow2_open()
checks may be to strict. With a corrupted snapshot table, it won't even
run. Perhaps we should be laxer with some checks with BDRV_O_CHECK, and
for example just set s->nb_snapshots = 0 if loading the table fails.

> >>Ideally, we need to make the qcow2 repair function repair such cases,
> >>but until that is done there is not much we can do about them.
> >What's the best repair?
> 
> That's what I was asking myself...
> 
> >Read the data from the unaligned location, and
> >write a fresh copy into a new aligned allocation?
> 
> Maybe. Maybe there is no way of repairing them and the only way is
> asking the user whether it's fine to delete the snapshot (qemu-img
> check -r make_consistent_whatever_it_takes).
> 
> Also, the question remains for every unaligned data structure: L2
> tables, data clusters… The refcount structures are the only ones
> that can be easily recovered. For data clusters, reading from the
> unaligned location would probably be best; for L2 tables… maybe the
> same, then run a consistency check on the data and if it seems off™,
> throw the whole L2 table away.

Reading from the unaligned location doesn't help. I have never seen an
image whose L2 entries contained valid entries, except for the least
significant few bits. If your table is corrupted, it's usually garbage
from start to end.

I think the only sane option is to drop the entries. The big question
is what should be used to replace them.

Kevin

Re: [Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Max Reitz]

On 2015-01-20 at 05:09, Kevin Wolf wrote:
> Am 19.01.2015 um 22:09 hat Max Reitz geschrieben:
>> On 2015-01-19 at 16:04, Eric Blake wrote:
>>> On 01/19/2015 01:49 PM, Max Reitz wrote:
>>>> With the series adding unalignment checks and the series reworking the
>>>> zero cluster expansion code overlapping, the unalignment checks have not
>>>> been implemented in the latter code.
>>>>
>>>> This series fixes it.
>>>>
>>>> There are other places which would require unalignment checks, like the
>>>> offsets of L1 tables, especially for snapshots; but because it would be
>>>> best to add these checks in the function which reads the snapshot table,
>>>> this would make images with broken snapshots completely unusable, which
>>>> is something I opted to avoid for now.
> That's something I noticed, too: For qemu-img check, our qcow2_open()
> checks may be to strict. With a corrupted snapshot table, it won't even
> run. Perhaps we should be laxer with some checks with BDRV_O_CHECK, and
> for example just set s->nb_snapshots = 0 if loading the table fails.
>
>>>> Ideally, we need to make the qcow2 repair function repair such cases,
>>>> but until that is done there is not much we can do about them.
>>> What's the best repair?
>> That's what I was asking myself...
>>
>>> Read the data from the unaligned location, and
>>> write a fresh copy into a new aligned allocation?
>> Maybe. Maybe there is no way of repairing them and the only way is
>> asking the user whether it's fine to delete the snapshot (qemu-img
>> check -r make_consistent_whatever_it_takes).
>>
>> Also, the question remains for every unaligned data structure: L2
>> tables, data clusters… The refcount structures are the only ones
>> that can be easily recovered. For data clusters, reading from the
>> unaligned location would probably be best; for L2 tables… maybe the
>> same, then run a consistency check on the data and if it seems off™,
>> throw the whole L2 table away.
> Reading from the unaligned location doesn't help. I have never seen an
> image whose L2 entries contained valid entries, except for the least
> significant few bits. If your table is corrupted, it's usually garbage
> from start to end.

Probably so.

> I think the only sane option is to drop the entries. The big question
> is what should be used to replace them.

/dev/random of course. There is a chance we are correct…

Seriously speaking, well, unmap them? Zero clusters don't feel right to me.

Max

Re: [Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Kevin Wolf]

Am 20.01.2015 um 14:49 hat Max Reitz geschrieben:
> On 2015-01-20 at 05:09, Kevin Wolf wrote:
> >Am 19.01.2015 um 22:09 hat Max Reitz geschrieben:
> >>On 2015-01-19 at 16:04, Eric Blake wrote:
> >>>On 01/19/2015 01:49 PM, Max Reitz wrote:
> >>>>With the series adding unalignment checks and the series reworking the
> >>>>zero cluster expansion code overlapping, the unalignment checks have not
> >>>>been implemented in the latter code.
> >>>>
> >>>>This series fixes it.
> >>>>
> >>>>There are other places which would require unalignment checks, like the
> >>>>offsets of L1 tables, especially for snapshots; but because it would be
> >>>>best to add these checks in the function which reads the snapshot table,
> >>>>this would make images with broken snapshots completely unusable, which
> >>>>is something I opted to avoid for now.
> >That's something I noticed, too: For qemu-img check, our qcow2_open()
> >checks may be to strict. With a corrupted snapshot table, it won't even
> >run. Perhaps we should be laxer with some checks with BDRV_O_CHECK, and
> >for example just set s->nb_snapshots = 0 if loading the table fails.
> >
> >>>>Ideally, we need to make the qcow2 repair function repair such cases,
> >>>>but until that is done there is not much we can do about them.
> >>>What's the best repair?
> >>That's what I was asking myself...
> >>
> >>>Read the data from the unaligned location, and
> >>>write a fresh copy into a new aligned allocation?
> >>Maybe. Maybe there is no way of repairing them and the only way is
> >>asking the user whether it's fine to delete the snapshot (qemu-img
> >>check -r make_consistent_whatever_it_takes).
> >>
> >>Also, the question remains for every unaligned data structure: L2
> >>tables, data clusters… The refcount structures are the only ones
> >>that can be easily recovered. For data clusters, reading from the
> >>unaligned location would probably be best; for L2 tables… maybe the
> >>same, then run a consistency check on the data and if it seems off™,
> >>throw the whole L2 table away.
> >Reading from the unaligned location doesn't help. I have never seen an
> >image whose L2 entries contained valid entries, except for the least
> >significant few bits. If your table is corrupted, it's usually garbage
> >from start to end.
> 
> Probably so.
> 
> >I think the only sane option is to drop the entries. The big question
> >is what should be used to replace them.
> 
> /dev/random of course. There is a chance we are correct…
> 
> Seriously speaking, well, unmap them? Zero clusters don't feel right to me.

Actually, I have a feeling that zero clusters are better because they
are obviously wrong when you debug a problem in the guest. Falling back
to the backing file may result in patterns that don't look obviously
corrupted, or even expose information to users that shouldn't have
permission to read it.

Kevin

Re: [Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Max Reitz]

On 2015-01-20 at 09:00, Kevin Wolf wrote:
> Am 20.01.2015 um 14:49 hat Max Reitz geschrieben:
>> On 2015-01-20 at 05:09, Kevin Wolf wrote:
>>> Am 19.01.2015 um 22:09 hat Max Reitz geschrieben:
>>>> On 2015-01-19 at 16:04, Eric Blake wrote:
>>>>> On 01/19/2015 01:49 PM, Max Reitz wrote:
>>>>>> With the series adding unalignment checks and the series reworking the
>>>>>> zero cluster expansion code overlapping, the unalignment checks have not
>>>>>> been implemented in the latter code.
>>>>>>
>>>>>> This series fixes it.
>>>>>>
>>>>>> There are other places which would require unalignment checks, like the
>>>>>> offsets of L1 tables, especially for snapshots; but because it would be
>>>>>> best to add these checks in the function which reads the snapshot table,
>>>>>> this would make images with broken snapshots completely unusable, which
>>>>>> is something I opted to avoid for now.
>>> That's something I noticed, too: For qemu-img check, our qcow2_open()
>>> checks may be to strict. With a corrupted snapshot table, it won't even
>>> run. Perhaps we should be laxer with some checks with BDRV_O_CHECK, and
>>> for example just set s->nb_snapshots = 0 if loading the table fails.
>>>
>>>>>> Ideally, we need to make the qcow2 repair function repair such cases,
>>>>>> but until that is done there is not much we can do about them.
>>>>> What's the best repair?
>>>> That's what I was asking myself...
>>>>
>>>>> Read the data from the unaligned location, and
>>>>> write a fresh copy into a new aligned allocation?
>>>> Maybe. Maybe there is no way of repairing them and the only way is
>>>> asking the user whether it's fine to delete the snapshot (qemu-img
>>>> check -r make_consistent_whatever_it_takes).
>>>>
>>>> Also, the question remains for every unaligned data structure: L2
>>>> tables, data clusters… The refcount structures are the only ones
>>>> that can be easily recovered. For data clusters, reading from the
>>>> unaligned location would probably be best; for L2 tables… maybe the
>>>> same, then run a consistency check on the data and if it seems off™,
>>>> throw the whole L2 table away.
>>> Reading from the unaligned location doesn't help. I have never seen an
>>> image whose L2 entries contained valid entries, except for the least
>>> significant few bits. If your table is corrupted, it's usually garbage
>> >from start to end.
>>
>> Probably so.
>>
>>> I think the only sane option is to drop the entries. The big question
>>> is what should be used to replace them.
>> /dev/random of course. There is a chance we are correct…
>>
>> Seriously speaking, well, unmap them? Zero clusters don't feel right to me.
> Actually, I have a feeling that zero clusters are better because they
> are obviously wrong when you debug a problem in the guest. Falling back
> to the backing file may result in patterns that don't look obviously
> corrupted, or even expose information to users that shouldn't have
> permission to read it.

That's a good point. Very well then, we only need to remember this 
thread when we get to the implementation of all this...

Max

Re: [Qemu-devel] [PATCH 0/2] qcow2: Add two more unalignment checks

[Kevin Wolf]

Am 19.01.2015 um 21:49 hat Max Reitz geschrieben:
> With the series adding unalignment checks and the series reworking the
> zero cluster expansion code overlapping, the unalignment checks have not
> been implemented in the latter code.
> 
> This series fixes it.
> 
> There are other places which would require unalignment checks, like the
> offsets of L1 tables, especially for snapshots; but because it would be
> best to add these checks in the function which reads the snapshot table,
> this would make images with broken snapshots completely unusable, which
> is something I opted to avoid for now.
> 
> Ideally, we need to make the qcow2 repair function repair such cases,
> but until that is done there is not much we can do about them.

Thanks, applied to the block branch.

Kevin