Patchwork [v6,09/20] block: vhdx - add region overlap detection for image files

login
register
mail settings
Submitter Jeff Cody
Date Sept. 25, 2013, 9:02 p.m.
Message ID <b8729fff8a974301d14588372934ff53ed3c5f10.1380141614.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/278015/
State New
Headers show

Comments

Jeff Cody - Sept. 25, 2013, 9:02 p.m.
Regions in the image file cannot overlap - the log, region tables,
and metdata must all be unique and non-overlapping.

This adds region checking by means of a QLIST; there can be a variable
number of regions and metadata (there may be metadata or region tables
that we do not recognize / know about, but are not required).

This adds the capability to register a region for later checking, and
to check against registered regions for any overlap.

Also, if neither the BAT or Metadata region tables are found, return
error.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vhdx.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/vhdx.h |  8 ++++++
 2 files changed, 91 insertions(+)
Stefan Hajnoczi - Oct. 1, 2013, 11:42 a.m.
On Wed, Sep 25, 2013 at 05:02:54PM -0400, Jeff Cody wrote:
> +/* Check for region overlaps inside the VHDX image */
> +static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
> +{
> +    int ret = 0;
> +    uint64_t end;
> +    VHDXRegionEntry *r;
> +
> +    end = start + length;
> +    QLIST_FOREACH(r, &s->regions, entries) {
> +        if ((start >= r->start && start <  r->end) ||
> +            (end   >  r->start && end   <= r->end)) {
> +            ret = -EINVAL;
> +            goto exit;
> +        }
> +    }
> +
> +exit:
> +    return ret;
> +}

This check does not catch a region that spans existing regions:

|----------------| new
     |-----|       existing

This will catch all cases:

QLIST_FOREACH(r, &s->regions, entries) {
    if (!((start >= r->end) || (end <= r->start))) {
        return -EINVAL;
    }
}
return 0;

> @@ -451,6 +499,15 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
>          le32_to_cpus(&rt_entry.length);
>          le32_to_cpus(&rt_entry.data_bits);
>  
> +        /* check for region overlap between these entries, and any
> +         * other memory regions in the file */
> +        ret = vhdx_region_check(s, rt_entry.file_offset, rt_entry.length);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +
> +        vhdx_region_register(s, rt_entry.file_offset, rt_entry.length);
> +
>          /* see if we recognize the entry */
>          if (guid_eq(rt_entry.guid, bat_guid)) {
>              /* must be unique; if we have already found it this is invalid */
> @@ -481,6 +538,12 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
>              goto fail;
>          }
>      }
> +
> +    if (!bat_rt_found || !metadata_rt_found) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      ret = 0;
>  
>  fail:

Another reason to avoid opening region tables before reading the
journal: we'll add regions twice if the journal had to be flushed.
Jeff Cody - Oct. 1, 2013, 1:49 p.m.
On Tue, Oct 01, 2013 at 01:42:18PM +0200, Stefan Hajnoczi wrote:
> On Wed, Sep 25, 2013 at 05:02:54PM -0400, Jeff Cody wrote:
> > +/* Check for region overlaps inside the VHDX image */
> > +static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
> > +{
> > +    int ret = 0;
> > +    uint64_t end;
> > +    VHDXRegionEntry *r;
> > +
> > +    end = start + length;
> > +    QLIST_FOREACH(r, &s->regions, entries) {
> > +        if ((start >= r->start && start <  r->end) ||
> > +            (end   >  r->start && end   <= r->end)) {
> > +            ret = -EINVAL;
> > +            goto exit;
> > +        }
> > +    }
> > +
> > +exit:
> > +    return ret;
> > +}
> 
> This check does not catch a region that spans existing regions:
> 
> |----------------| new
>      |-----|       existing
> 
> This will catch all cases:
> 
> QLIST_FOREACH(r, &s->regions, entries) {
>     if (!((start >= r->end) || (end <= r->start))) {
>         return -EINVAL;
>     }
> }
> return 0;
> 

You are right, thanks.

> > @@ -451,6 +499,15 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
> >          le32_to_cpus(&rt_entry.length);
> >          le32_to_cpus(&rt_entry.data_bits);
> >  
> > +        /* check for region overlap between these entries, and any
> > +         * other memory regions in the file */
> > +        ret = vhdx_region_check(s, rt_entry.file_offset, rt_entry.length);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +
> > +        vhdx_region_register(s, rt_entry.file_offset, rt_entry.length);
> > +
> >          /* see if we recognize the entry */
> >          if (guid_eq(rt_entry.guid, bat_guid)) {
> >              /* must be unique; if we have already found it this is invalid */
> > @@ -481,6 +538,12 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
> >              goto fail;
> >          }
> >      }
> > +
> > +    if (!bat_rt_found || !metadata_rt_found) {
> > +        ret = -EINVAL;
> > +        goto fail;
> > +    }
> > +
> >      ret = 0;
> >  
> >  fail:
> 
> Another reason to avoid opening region tables before reading the
> journal: we'll add regions twice if the journal had to be flushed.

Yes, good point - I'll need to think of a good solution to this, if I
want to be able to check table overlaps prior to flushing the log.
Alternatively, just not worry about that and go ahead and flush the
log, and detect overlaps afterwards.  Maybe we'd get lucky and the log
flush would fix the overlaps :)

Patch

diff --git a/block/vhdx.c b/block/vhdx.c
index 18cba21..a0a01ec 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -203,6 +203,51 @@  void vhdx_guid_generate(MSGUID *guid)
     memcpy(guid, uuid, sizeof(MSGUID));
 }
 
+/* Check for region overlaps inside the VHDX image */
+static int vhdx_region_check(BDRVVHDXState *s, uint64_t start, uint64_t length)
+{
+    int ret = 0;
+    uint64_t end;
+    VHDXRegionEntry *r;
+
+    end = start + length;
+    QLIST_FOREACH(r, &s->regions, entries) {
+        if ((start >= r->start && start <  r->end) ||
+            (end   >  r->start && end   <= r->end)) {
+            ret = -EINVAL;
+            goto exit;
+        }
+    }
+
+exit:
+    return ret;
+}
+
+/* Register a region for future checks */
+static void vhdx_region_register(BDRVVHDXState *s,
+                                 uint64_t start, uint64_t length)
+{
+    VHDXRegionEntry *r;
+
+    r = g_malloc0(sizeof(*r));
+
+    r->start = start;
+    r->end = start + length;
+
+    QLIST_INSERT_HEAD(&s->regions, r, entries);
+}
+
+/* Free all registered regions */
+static void vhdx_region_unregister_all(BDRVVHDXState *s)
+{
+    VHDXRegionEntry *r, *r_next;
+
+    QLIST_FOREACH_SAFE(r, &s->regions, entries, r_next) {
+        QLIST_REMOVE(r, entries);
+        g_free(r);
+    }
+}
+
 /*
  * Per the MS VHDX Specification, for every VHDX file:
  *      - The header section is fixed size - 1 MB
@@ -388,6 +433,9 @@  static int vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s)
         }
     }
 
+    vhdx_region_register(s, s->headers[s->curr_header]->log_offset,
+                            s->headers[s->curr_header]->log_length);
+
     ret = 0;
 
     goto exit;
@@ -451,6 +499,15 @@  static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
         le32_to_cpus(&rt_entry.length);
         le32_to_cpus(&rt_entry.data_bits);
 
+        /* check for region overlap between these entries, and any
+         * other memory regions in the file */
+        ret = vhdx_region_check(s, rt_entry.file_offset, rt_entry.length);
+        if (ret < 0) {
+            goto fail;
+        }
+
+        vhdx_region_register(s, rt_entry.file_offset, rt_entry.length);
+
         /* see if we recognize the entry */
         if (guid_eq(rt_entry.guid, bat_guid)) {
             /* must be unique; if we have already found it this is invalid */
@@ -481,6 +538,12 @@  static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
             goto fail;
         }
     }
+
+    if (!bat_rt_found || !metadata_rt_found) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
     ret = 0;
 
 fail:
@@ -743,6 +806,7 @@  static void vhdx_close(BlockDriverState *bs)
     qemu_vfree(s->bat);
     qemu_vfree(s->parent_entries);
     qemu_vfree(s->log.hdr);
+    vhdx_region_unregister_all(s);
 }
 
 static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
@@ -761,6 +825,7 @@  static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
     s->log.write = s->log.read = 0;
 
     qemu_co_mutex_init(&s->lock);
+    QLIST_INIT(&s->regions);
 
     /* validate the file signature */
     ret = bdrv_pread(bs->file, 0, &signature, sizeof(uint64_t));
@@ -847,8 +912,26 @@  static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    uint64_t payblocks = s->chunk_ratio;
+    /* endian convert, and verify populated BAT field file offsets against
+     * region table and log entries */
     for (i = 0; i < s->bat_entries; i++) {
         le64_to_cpus(&s->bat[i]);
+        if (payblocks--) {
+            /* payload bat entries */
+            if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
+                    PAYLOAD_BLOCK_FULL_PRESENT) {
+                ret = vhdx_region_check(s, s->bat[i] & VHDX_BAT_FILE_OFF_MASK,
+                                        s->block_size);
+                if (ret < 0) {
+                    goto fail;
+                }
+            }
+        } else {
+            payblocks = s->chunk_ratio;
+            /* Once differencing files are supported, verify sector bitmap
+             * blocks here */
+        }
     }
 
     if (flags & BDRV_O_RDWR) {
diff --git a/block/vhdx.h b/block/vhdx.h
index dfb3ed9..831aa13 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -230,6 +230,7 @@  typedef struct QEMU_PACKED VHDXLogDataSector {
    other bits are reserved */
 #define VHDX_BAT_STATE_BIT_MASK 0x07
 #define VHDX_BAT_FILE_OFF_BITS (64 - 44)
+#define VHDX_BAT_FILE_OFF_MASK  0xFFFFFFFFFFF00000 /* upper 44 bits */
 typedef uint64_t VHDXBatEntry;
 
 /* ---- METADATA REGION STRUCTURES ---- */
@@ -334,6 +335,12 @@  typedef struct VHDXLogEntries {
     uint32_t tail;
 } VHDXLogEntries;
 
+typedef struct VHDXRegionEntry {
+    uint64_t start;
+    uint64_t end;
+    QLIST_ENTRY(VHDXRegionEntry) entries;
+} VHDXRegionEntry;
+
 typedef struct BDRVVHDXState {
     CoMutex lock;
 
@@ -373,6 +380,7 @@  typedef struct BDRVVHDXState {
     VHDXParentLocatorHeader parent_header;
     VHDXParentLocatorEntry *parent_entries;
 
+    QLIST_HEAD(VHDXRegionHead, VHDXRegionEntry) regions;
 } BDRVVHDXState;
 
 void vhdx_guid_generate(MSGUID *guid);