diff mbox

[v2,11/12] qcow2/overlaps: Protect inactive L2 tables

Message ID 1416844620-17717-12-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Nov. 24, 2014, 3:56 p.m. UTC 
Keep track of the inactive L2 tables in the metadata list to protect
them against accidental modifications.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 20 ++++++++++++++++++++
 block/qcow2-snapshot.c | 41 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 59 insertions(+), 2 deletions(-)

Comments

Stefan Hajnoczi Jan. 21, 2015, 3:23 p.m. UTC | #1 
On Mon, Nov 24, 2014 at 04:56:59PM +0100, Max Reitz wrote:
> @@ -136,6 +138,34 @@ int qcow2_read_snapshots(BlockDriverState *bs)
>                                    size_to_clusters(s, sn->l1_size *
>                                                        sizeof(uint64_t)),
>                                    QCOW2_OL_INACTIVE_L1);
> +
> +        if (!(s->overlap_check & QCOW2_OL_INACTIVE_L2)) {
> +            continue;
> +        }
> +
> +        l1_table = qemu_try_blockalign(bs->file,
> +                                       sn->l1_size * sizeof(uint64_t));

At this point we haven't validated sn->l1_size <= QCOW_MAX_L1_SIZE.

A bogus l1_size means we do a huge read and add junk into the metadata
list.  I think it would be best to check the value here.
Max Reitz Jan. 21, 2015, 4:07 p.m. UTC | #2 
On 2015-01-21 at 10:23, Stefan Hajnoczi wrote:
> On Mon, Nov 24, 2014 at 04:56:59PM +0100, Max Reitz wrote:
>> @@ -136,6 +138,34 @@ int qcow2_read_snapshots(BlockDriverState *bs)
>>                                     size_to_clusters(s, sn->l1_size *
>>                                                         sizeof(uint64_t)),
>>                                     QCOW2_OL_INACTIVE_L1);
>> +
>> +        if (!(s->overlap_check & QCOW2_OL_INACTIVE_L2)) {
>> +            continue;
>> +        }
>> +
>> +        l1_table = qemu_try_blockalign(bs->file,
>> +                                       sn->l1_size * sizeof(uint64_t));
> At this point we haven't validated sn->l1_size <= QCOW_MAX_L1_SIZE.
>
> A bogus l1_size means we do a huge read and add junk into the metadata
> list.  I think it would be best to check the value here.

Right, will do.

Thanks for reviewing!

Max
diff mbox

Patch

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 94fe515..c7096b7 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1042,8 +1042,28 @@  int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                         s->cluster_bits, addend, QCOW2_DISCARD_SNAPSHOT);
                 if (addend < 0) {
                     if (active_l1) {
+                        /* This is easy */
                         qcow2_metadata_list_remove(bs, l2_offset, 1,
                                                    QCOW2_OL_ACTIVE_L2);
+                    } else {
+                        /* If refcount == 0, this is, too. If refcount > 1, we
+                         * know that there must be some other inactive L2
+                         * reference; and for refcount == 1, if this is an
+                         * active L2 table, this was the last inactive L2
+                         * reference. */
+                        bool remove;
+                        if (refcount == 0) {
+                            remove = true;
+                        } else if (refcount == 1) {
+                            remove = qcow2_check_metadata_overlap(bs,
+                                ~QCOW2_OL_ACTIVE_L2, l2_offset,s->cluster_size);
+                        } else {
+                            remove = false;
+                        }
+                        if (remove) {
+                            qcow2_metadata_list_remove(bs, l2_offset, 1,
+                                                       QCOW2_OL_INACTIVE_L2);
+                        }
                     }
                 }
             } else {
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index b3122d8..800c7d3 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -46,9 +46,10 @@  int qcow2_read_snapshots(BlockDriverState *bs)
     QCowSnapshotHeader h;
     QCowSnapshotExtraData extra;
     QCowSnapshot *sn;
-    int i, id_str_size, name_size;
+    int i, j, id_str_size, name_size;
     int64_t offset;
     uint32_t extra_data_size;
+    uint64_t *l1_table;
     int ret;
 
     if (!s->nb_snapshots) {
@@ -122,7 +123,8 @@  int qcow2_read_snapshots(BlockDriverState *bs)
             goto fail;
         }
 
-        if (!(s->overlap_check & QCOW2_OL_INACTIVE_L1)) {
+        if (!(s->overlap_check & (QCOW2_OL_INACTIVE_L1 | QCOW2_OL_INACTIVE_L2)))
+        {
             continue;
         }
 
@@ -136,6 +138,34 @@  int qcow2_read_snapshots(BlockDriverState *bs)
                                   size_to_clusters(s, sn->l1_size *
                                                       sizeof(uint64_t)),
                                   QCOW2_OL_INACTIVE_L1);
+
+        if (!(s->overlap_check & QCOW2_OL_INACTIVE_L2)) {
+            continue;
+        }
+
+        l1_table = qemu_try_blockalign(bs->file,
+                                       sn->l1_size * sizeof(uint64_t));
+        if (!l1_table) {
+            /* Do not fail opening the image just because a snapshot's L2 tables
+             * cannot be covered by the overlap checks */
+            continue;
+        }
+
+        ret = bdrv_pread(bs->file, sn->l1_table_offset, l1_table,
+                         sn->l1_size * sizeof(uint64_t));
+        if (ret < 0) {
+            qemu_vfree(l1_table);
+            continue;
+        }
+        for (j = 0; j < sn->l1_size; j++) {
+            uint64_t l2_offset = be64_to_cpu(l1_table[j]) & L1E_OFFSET_MASK;
+            if (l2_offset) {
+                qcow2_metadata_list_enter(bs, l2_offset, 1,
+                                          QCOW2_OL_INACTIVE_L2);
+            }
+        }
+
+        qemu_vfree(l1_table);
     }
 
     assert(offset - s->snapshots_offset <= INT_MAX);
@@ -436,6 +466,13 @@  int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
                                                   sizeof(uint64_t)),
                               QCOW2_OL_INACTIVE_L1);
 
+    for (i = 0; i < s->l1_size; i++) {
+        uint64_t l2_offset = s->l1_table[i] & L1E_OFFSET_MASK;
+        if (l2_offset) {
+            qcow2_metadata_list_enter(bs, l2_offset, 1, QCOW2_OL_INACTIVE_L2);
+        }
+    }
+
     /*
      * Increase the refcounts of all clusters and make sure everything is
      * stable on disk before updating the snapshot table to contain a pointer