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

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

[Max Reitz]

The image fuzzer from Maria exposed a lot of assertions which might fail
in qemu when fed with a broken qcow2 image. Some of them are related to
qemu trusting the offsets given in the L1, L2 and refcount tables to
always be properly aligned on cluster boundaries (e.g.
https://bugs.launchpad.net/qemu/+bug/1354529).

This series fixes this by verifying (hopefully) all data read from L1,
L2 and refcount tables accordingly; if the offsets are not aligned on
cluster boundaries, an error message is emitted and the image is marked
corrupt unless it has been opened read-only.


Max Reitz (4):
  qcow2: Add qcow2_signal_corruption()
  qcow2: Use qcow2_signal_corruption() for overlaps
  iotests: Fix output of 060
  qcow2: Check L1/L2/reftable entries for alignment

 block/qcow2-cluster.c      | 27 ++++++++++++++++++++-
 block/qcow2-refcount.c     | 59 +++++++++++++++++++++++++++++-----------------
 block/qcow2.c              | 28 ++++++++++++++++++++++
 block/qcow2.h              |  4 ++++
 tests/qemu-iotests/060.out | 10 ++++----
 5 files changed, 100 insertions(+), 28 deletions(-)

-- 
2.0.4

[Qemu-devel] [PATCH 1/4] qcow2: Add qcow2_signal_corruption()

[Max Reitz]

Add a helper function for easily marking an image corrupt while
outputting an informative message to stderr and via QAPI.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 28 ++++++++++++++++++++++++++++
 block/qcow2.h |  4 ++++
 2 files changed, 32 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 435e0e1..ef2c931 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -30,6 +30,8 @@
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
+#include "qapi/qmp/types.h"
+#include "qapi-event.h"
 #include "trace.h"
 #include "qemu/option_int.h"
 
@@ -2378,6 +2380,32 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
     return 0;
 }
 
+void qcow2_signal_corruption(BlockDriverState *bs, int64_t offset, int64_t size,
+                             const char *message_format, ...)
+{
+    char *message;
+    va_list ap;
+
+    va_start(ap, message_format);
+    message = g_strdup_vprintf(message_format, ap);
+    va_end(ap);
+
+    if (bs->read_only) {
+        fprintf(stderr, "qcow2: Image is corrupt: %s\n", message);
+    } else {
+        fprintf(stderr, "qcow2: Marking image as corrupt: %s\n", message);
+        qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs), message,
+                                              offset >= 0, offset,
+                                              size >= 0, size, &error_abort);
+    }
+    g_free(message);
+
+    if (!bs->read_only) {
+        qcow2_mark_corrupt(bs);
+        bs->drv = NULL; /* make BDS unusable */
+    }
+}
+
 static QemuOptsList qcow2_create_opts = {
     .name = "qcow2-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(qcow2_create_opts.head),
diff --git a/block/qcow2.h b/block/qcow2.h
index b49424b..f66238e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -468,6 +468,10 @@ int qcow2_mark_corrupt(BlockDriverState *bs);
 int qcow2_mark_consistent(BlockDriverState *bs);
 int qcow2_update_header(BlockDriverState *bs);
 
+void qcow2_signal_corruption(BlockDriverState *bs, int64_t offset, int64_t size,
+                             const char *message_format, ...)
+                             GCC_FMT_ATTR(4, 5);
+
 /* qcow2-refcount.c functions */
 int qcow2_refcount_init(BlockDriverState *bs);
 void qcow2_refcount_close(BlockDriverState *bs);
-- 
2.0.4

Re: [Qemu-devel] [PATCH 1/4] qcow2: Add qcow2_signal_corruption()

[Kevin Wolf]

Am 16.08.2014 um 23:16 hat Max Reitz geschrieben:
> Add a helper function for easily marking an image corrupt while
> outputting an informative message to stderr and via QAPI.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 28 ++++++++++++++++++++++++++++
>  block/qcow2.h |  4 ++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 435e0e1..ef2c931 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -30,6 +30,8 @@
>  #include "qemu/error-report.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/types.h"
> +#include "qapi-event.h"
>  #include "trace.h"
>  #include "qemu/option_int.h"
>  
> @@ -2378,6 +2380,32 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
>      return 0;
>  }
>  
> +void qcow2_signal_corruption(BlockDriverState *bs, int64_t offset, int64_t size,
> +                             const char *message_format, ...)
> +{
> +    char *message;
> +    va_list ap;
> +
> +    va_start(ap, message_format);
> +    message = g_strdup_vprintf(message_format, ap);
> +    va_end(ap);
> +
> +    if (bs->read_only) {
> +        fprintf(stderr, "qcow2: Image is corrupt: %s\n", message);

The BDS isn't made unusable in read-only mode, so we can produce quite a
lot of these messages and fill up the log. Perhaps it would be better to
print the message only the first time (or the first n times) and then
tell the user that further errors won't be logged.

Also, including the block device or file name (or both) wouldn't hurt.

> +    } else {
> +        fprintf(stderr, "qcow2: Marking image as corrupt: %s\n", message);
> +        qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs), message,
> +                                              offset >= 0, offset,
> +                                              size >= 0, size, &error_abort);
> +    }
> +    g_free(message);
> +
> +    if (!bs->read_only) {
> +        qcow2_mark_corrupt(bs);
> +        bs->drv = NULL; /* make BDS unusable */
> +    }
> +}

Kevin

Re: [Qemu-devel] [PATCH 1/4] qcow2: Add qcow2_signal_corruption()

[Max Reitz]

On 20.08.2014 12:10, Kevin Wolf wrote:
> Am 16.08.2014 um 23:16 hat Max Reitz geschrieben:
>> Add a helper function for easily marking an image corrupt while
>> outputting an informative message to stderr and via QAPI.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2.c | 28 ++++++++++++++++++++++++++++
>>   block/qcow2.h |  4 ++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 435e0e1..ef2c931 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -30,6 +30,8 @@
>>   #include "qemu/error-report.h"
>>   #include "qapi/qmp/qerror.h"
>>   #include "qapi/qmp/qbool.h"
>> +#include "qapi/qmp/types.h"
>> +#include "qapi-event.h"
>>   #include "trace.h"
>>   #include "qemu/option_int.h"
>>   
>> @@ -2378,6 +2380,32 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
>>       return 0;
>>   }
>>   
>> +void qcow2_signal_corruption(BlockDriverState *bs, int64_t offset, int64_t size,
>> +                             const char *message_format, ...)
>> +{
>> +    char *message;
>> +    va_list ap;
>> +
>> +    va_start(ap, message_format);
>> +    message = g_strdup_vprintf(message_format, ap);
>> +    va_end(ap);
>> +
>> +    if (bs->read_only) {
>> +        fprintf(stderr, "qcow2: Image is corrupt: %s\n", message);
> The BDS isn't made unusable in read-only mode, so we can produce quite a
> lot of these messages and fill up the log. Perhaps it would be better to
> print the message only the first time (or the first n times) and then
> tell the user that further errors won't be logged.
>
> Also, including the block device or file name (or both) wouldn't hurt.

I'll include both in v2.

Max

>> +    } else {
>> +        fprintf(stderr, "qcow2: Marking image as corrupt: %s\n", message);
>> +        qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs), message,
>> +                                              offset >= 0, offset,
>> +                                              size >= 0, size, &error_abort);
>> +    }
>> +    g_free(message);
>> +
>> +    if (!bs->read_only) {
>> +        qcow2_mark_corrupt(bs);
>> +        bs->drv = NULL; /* make BDS unusable */
>> +    }
>> +}
> Kevin

[Qemu-devel] [PATCH 2/4] qcow2: Use qcow2_signal_corruption() for overlaps

[Max Reitz]

Use the new function in case of a failed overlap check.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3b77470..0ac1339 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -26,8 +26,6 @@
 #include "block/block_int.h"
 #include "block/qcow2.h"
 #include "qemu/range.h"
-#include "qapi/qmp/types.h"
-#include "qapi-event.h"
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size);
 static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
@@ -1838,26 +1836,11 @@ int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
         return ret;
     } else if (ret > 0) {
         int metadata_ol_bitnr = ffs(ret) - 1;
-        char *message;
-
         assert(metadata_ol_bitnr < QCOW2_OL_MAX_BITNR);
 
-        fprintf(stderr, "qcow2: Preventing invalid write on metadata (overlaps "
-                "with %s); image marked as corrupt.\n",
-                metadata_ol_names[metadata_ol_bitnr]);
-        message = g_strdup_printf("Prevented %s overwrite",
-                metadata_ol_names[metadata_ol_bitnr]);
-        qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs),
-                                              message,
-                                              true,
-                                              offset,
-                                              true,
-                                              size,
-                                              &error_abort);
-        g_free(message);
-
-        qcow2_mark_corrupt(bs);
-        bs->drv = NULL; /* make BDS unusable */
+        qcow2_signal_corruption(bs, offset, size, "Preventing invalid write on "
+                                "metadata (overlaps with %s)",
+                                metadata_ol_names[metadata_ol_bitnr]);
         return -EIO;
     }
 
-- 
2.0.4

[Qemu-devel] [PATCH 3/4] iotests: Fix output of 060

[Max Reitz]

With qcow2_pre_write_overlap_check() relying on
qcow2_signal_corruption(), the output in case of a corruption changes.
Therefore, 060's output has to be adapted accordingly.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/060.out | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index c27c952..92d567c 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -8,7 +8,7 @@ ERROR cluster 3 refcount=1 reference=3
 1 errors were found on the image.
 Data may be corrupted, or further writes to the image may corrupt it.
 incompatible_features     0x0
-qcow2: Preventing invalid write on metadata (overlaps with active L1 table); image marked as corrupt.
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with active L1 table)
 write failed: Input/output error
 incompatible_features     0x2
 qemu-io: can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write
@@ -24,7 +24,7 @@ ERROR cluster 2 refcount=1 reference=2
 2 errors were found on the image.
 Data may be corrupted, or further writes to the image may corrupt it.
 incompatible_features     0x0
-qcow2: Preventing invalid write on metadata (overlaps with refcount block); image marked as corrupt.
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with refcount block)
 write failed: Input/output error
 incompatible_features     0x2
 Repairing refcount block 0 refcount=2
@@ -56,7 +56,7 @@ Data may be corrupted, or further writes to the image may corrupt it.
 1 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 incompatible_features     0x0
-qcow2: Preventing invalid write on metadata (overlaps with inactive L2 table); image marked as corrupt.
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with inactive L2 table)
 write failed: Input/output error
 incompatible_features     0x2
 Repairing cluster 4 refcount=1 reference=2
@@ -88,7 +88,7 @@ wrote 65536/65536 bytes at offset 536870912
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 discard 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qcow2: Preventing invalid write on metadata (overlaps with active L2 table); image marked as corrupt.
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with active L2 table)
 blkdebug: Suspended request '0'
 write failed: Input/output error
 blkdebug: Resuming request '0'
@@ -99,6 +99,6 @@ aio_write failed: No medium found
 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: Preventing invalid write on metadata (overlaps with qcow2_header); image marked as corrupt.
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with qcow2_header)
 write failed: Input/output error
 *** done
-- 
2.0.4

Re: [Qemu-devel] [PATCH 3/4] iotests: Fix output of 060

[Kevin Wolf]

Am 16.08.2014 um 23:16 hat Max Reitz geschrieben:
> With qcow2_pre_write_overlap_check() relying on
> qcow2_signal_corruption(), the output in case of a corruption changes.
> Therefore, 060's output has to be adapted accordingly.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Squash this into patch 2 so that the test isn't broken at any point
during the series.

Kevin

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

[Max Reitz]

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(-)

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);
+        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);
+            qcow2_cache_put(bs, s->l2_table_cache, (void **)&l2_table);
+            return -EIO;
+        }
         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);
+        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);
+            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);
+        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;
+            }
+
              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);
+            }
         }
         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;
+            }
+
             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);
+                            ret = -EIO;
+                            goto fail;
+                        }
+
                         cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits;
                         if (!cluster_index) {
                             /* unallocated */
-- 
2.0.4

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

[Kevin Wolf]

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

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

[Max Reitz]

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

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

[Kevin Wolf]

Am 20.08.2014 um 21:26 hat Max Reitz geschrieben:
> 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(-)

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

I can see your point. This is a tough one: On the one hand, it is
undoubtedly corruption, and usually there is not just one corrupted
entry, so you want the user to check the image. On the other hand, yes,
this is merely a cluster leak and breaking the VM for that might be a
bit too much.

Still just printing a line on stderr and continuing otherwise doesn't
feel quite right, the user will usually miss the message because it ends
up in the log of a seemingly well working guest and if printed
unconditionally, it may still flood the logs.

Eric, would management be able to make something useful out of this if
we sent a QMP event?

Kevin

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

[Eric Blake]

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

On 08/21/2014 02:14 AM, Kevin Wolf wrote:

>>>> +            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.
> 
> I can see your point. This is a tough one: On the one hand, it is
> undoubtedly corruption, and usually there is not just one corrupted
> entry, so you want the user to check the image. On the other hand, yes,
> this is merely a cluster leak and breaking the VM for that might be a
> bit too much.
> 
> Still just printing a line on stderr and continuing otherwise doesn't
> feel quite right, the user will usually miss the message because it ends
> up in the log of a seemingly well working guest and if printed
> unconditionally, it may still flood the logs.
> 
> Eric, would management be able to make something useful out of this if
> we sent a QMP event?

Libvirt could certainly expose a QMP event to upper layers (oVirt, VDSM,
OpenStack, ...) for them to be made aware that "your image had a
non-fatal corruption, you may want to check if there are other problems
going on".  I don't think libvirt would do anything in particular with
this knowledge beyond passthrough, but it does sound like a reasonable
event to add.

-- 
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: 539 bytes --]