[Qemu-devel] [PATCH 0/2] vmdk: Fix streamOptimized images

[Qemu-devel] [PATCH 0/2] vmdk: Fix streamOptimized images

[Kevin Wolf]

The other day someone turned up in IRC with a VMDK image [1] that can't be
converted (or even read). We found the problem, discussed the fix and the
reporter promised to send a fix. Well, he didn't in almost a month, so here's
my fix.

This was reported as https://bugs.launchpad.net/qemu/+bug/1028908

[1] http://downloads.puppetlabs.com/learning/learn_puppet_centos_pe2.5.1_ovf.2012.04.18.zip

Kevin Wolf (2):
  vmdk: Fix header structure
  vmdk: Read footer for streamOptimized images

 block/vmdk.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH 1/2] vmdk: Fix header structure

[Kevin Wolf]

Commit bb45ded9 swapped gd_offset and rgd_offset. This is wrong.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index daee426..9648398 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -57,8 +57,8 @@ typedef struct {
     int64_t desc_offset;
     int64_t desc_size;
     int32_t num_gtes_per_gte;
-    int64_t gd_offset;
     int64_t rgd_offset;
+    int64_t gd_offset;
     int64_t grain_offset;
     char filler[1];
     char check_bytes[4];
-- 
1.7.6.5

[Qemu-devel] [PATCH 2/2] vmdk: Read footer for streamOptimized images

[Kevin Wolf]

The footer takes precedence over the header when it exists. It contains
the real grain directory offset that is missing in the header. Without
this patch, streamOptimized images with a footer cannot be read.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 9648398..c243a96 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -35,6 +35,7 @@
 #define VMDK4_FLAG_RGD (1 << 1)
 #define VMDK4_FLAG_COMPRESS (1 << 16)
 #define VMDK4_FLAG_MARKER (1 << 17)
+#define VMDK4_GD_AT_END 0xffffffffffffffffULL
 
 typedef struct {
     uint32_t version;
@@ -451,6 +452,21 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     if (header.capacity == 0 && header.desc_offset) {
         return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
     }
+
+    if (header.gd_offset == VMDK4_GD_AT_END) {
+        /*
+         * The footer takes precedence over the header, so read it in. The
+         * footer starts at offset -1024 from the end: One sector for the
+         * footer, and another one for the end-of-stream marker.
+         */
+        ret = bdrv_pread(file,
+            bs->file->total_sectors * 512 - 1024 + sizeof(magic),
+            &header, sizeof(header));
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
                         * le64_to_cpu(header.granularity);
     if (l1_entry_sectors == 0) {
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH 2/2] vmdk: Read footer for streamOptimized images

[Stefan Hajnoczi]

On Thu, Aug 16, 2012 at 9:54 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> The footer takes precedence over the header when it exists. It contains
> the real grain directory offset that is missing in the header. Without
> this patch, streamOptimized images with a footer cannot be read.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vmdk.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 9648398..c243a96 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -35,6 +35,7 @@
>  #define VMDK4_FLAG_RGD (1 << 1)
>  #define VMDK4_FLAG_COMPRESS (1 << 16)
>  #define VMDK4_FLAG_MARKER (1 << 17)
> +#define VMDK4_GD_AT_END 0xffffffffffffffffULL
>
>  typedef struct {
>      uint32_t version;
> @@ -451,6 +452,21 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>      if (header.capacity == 0 && header.desc_offset) {
>          return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
>      }
> +
> +    if (header.gd_offset == VMDK4_GD_AT_END) {
> +        /*
> +         * The footer takes precedence over the header, so read it in. The
> +         * footer starts at offset -1024 from the end: One sector for the
> +         * footer, and another one for the end-of-stream marker.
> +         */
> +        ret = bdrv_pread(file,
> +            bs->file->total_sectors * 512 - 1024 + sizeof(magic),
> +            &header, sizeof(header));
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
>      l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
>                          * le64_to_cpu(header.granularity);
>      if (l1_entry_sectors == 0) {

I think we should check the magic number or marker before trusting the
contents of the footer.

Stefan

[Qemu-devel] [PATCH v2 2/2] vmdk: Read footer for streamOptimized images

[Kevin Wolf]

The footer takes precedence over the header when it exists. It contains
the real grain directory offset that is missing in the header. Without
this patch, streamOptimized images with a footer cannot be read.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
v2:
- Enough footer sanity checks, I hope? :-)

 block/vmdk.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 9648398..bba4c61 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -35,6 +35,7 @@
 #define VMDK4_FLAG_RGD (1 << 1)
 #define VMDK4_FLAG_COMPRESS (1 << 16)
 #define VMDK4_FLAG_MARKER (1 << 17)
+#define VMDK4_GD_AT_END 0xffffffffffffffffULL
 
 typedef struct {
     uint32_t version;
@@ -115,6 +116,13 @@ typedef struct VmdkGrainMarker {
     uint8_t  data[0];
 } VmdkGrainMarker;
 
+enum {
+    MARKER_END_OF_STREAM    = 0,
+    MARKER_GRAIN_TABLE      = 1,
+    MARKER_GRAIN_DIRECTORY  = 2,
+    MARKER_FOOTER           = 3,
+};
+
 static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     uint32_t magic;
@@ -451,6 +459,54 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     if (header.capacity == 0 && header.desc_offset) {
         return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
     }
+
+    if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) {
+        /*
+         * The footer takes precedence over the header, so read it in. The
+         * footer starts at offset -1024 from the end: One sector for the
+         * footer, and another one for the end-of-stream marker.
+         */
+        struct {
+            struct {
+                uint64_t val;
+                uint32_t size;
+                uint32_t type;
+                uint8_t pad[512 - 16];
+            } QEMU_PACKED footer_marker;
+
+            uint32_t magic;
+            VMDK4Header header;
+            uint8_t pad[512 - 4 - sizeof(VMDK4Header)];
+
+            struct {
+                uint64_t val;
+                uint32_t size;
+                uint32_t type;
+                uint8_t pad[512 - 16];
+            } QEMU_PACKED eos_marker;
+        } QEMU_PACKED footer;
+
+        ret = bdrv_pread(file,
+            bs->file->total_sectors * 512 - 1536,
+            &footer, sizeof(footer));
+        if (ret < 0) {
+            return ret;
+        }
+
+        /* Some sanity checks for the footer */
+        if (be32_to_cpu(footer.magic) != VMDK4_MAGIC ||
+            le32_to_cpu(footer.footer_marker.size) != 0  ||
+            le32_to_cpu(footer.footer_marker.type) != MARKER_FOOTER ||
+            le64_to_cpu(footer.eos_marker.val) != 0  ||
+            le32_to_cpu(footer.eos_marker.size) != 0  ||
+            le32_to_cpu(footer.eos_marker.type) != MARKER_END_OF_STREAM)
+        {
+            return -EINVAL;
+        }
+
+        header = footer.header;
+    }
+
     l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
                         * le64_to_cpu(header.granularity);
     if (l1_entry_sectors == 0) {
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH v2 2/2] vmdk: Read footer for streamOptimized images

[Stefan Hajnoczi]

On Thu, Aug 16, 2012 at 10:50 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> The footer takes precedence over the header when it exists. It contains
> the real grain directory offset that is missing in the header. Without
> this patch, streamOptimized images with a footer cannot be read.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> v2:
> - Enough footer sanity checks, I hope? :-)
>
>  block/vmdk.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v2 2/2] vmdk: Read footer for streamOptimized images

[Jeff Cody]

On 08/16/2012 05:50 AM, Kevin Wolf wrote:
> The footer takes precedence over the header when it exists. It contains
> the real grain directory offset that is missing in the header. Without
> this patch, streamOptimized images with a footer cannot be read.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> v2:
> - Enough footer sanity checks, I hope? :-)
> 
>  block/vmdk.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 9648398..bba4c61 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -35,6 +35,7 @@
>  #define VMDK4_FLAG_RGD (1 << 1)
>  #define VMDK4_FLAG_COMPRESS (1 << 16)
>  #define VMDK4_FLAG_MARKER (1 << 17)
> +#define VMDK4_GD_AT_END 0xffffffffffffffffULL
>  
>  typedef struct {
>      uint32_t version;
> @@ -115,6 +116,13 @@ typedef struct VmdkGrainMarker {
>      uint8_t  data[0];
>  } VmdkGrainMarker;
>  
> +enum {
> +    MARKER_END_OF_STREAM    = 0,
> +    MARKER_GRAIN_TABLE      = 1,
> +    MARKER_GRAIN_DIRECTORY  = 2,
> +    MARKER_FOOTER           = 3,
> +};
> +
>  static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
>  {
>      uint32_t magic;
> @@ -451,6 +459,54 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>      if (header.capacity == 0 && header.desc_offset) {
>          return vmdk_open_desc_file(bs, flags, header.desc_offset << 9);
>      }
> +
> +    if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) {
> +        /*
> +         * The footer takes precedence over the header, so read it in. The
> +         * footer starts at offset -1024 from the end: One sector for the
> +         * footer, and another one for the end-of-stream marker.
> +         */
> +        struct {
> +            struct {
> +                uint64_t val;
> +                uint32_t size;
> +                uint32_t type;
> +                uint8_t pad[512 - 16];
> +            } QEMU_PACKED footer_marker;
> +
> +            uint32_t magic;
> +            VMDK4Header header;
> +            uint8_t pad[512 - 4 - sizeof(VMDK4Header)];
> +
> +            struct {
> +                uint64_t val;
> +                uint32_t size;
> +                uint32_t type;
> +                uint8_t pad[512 - 16];
> +            } QEMU_PACKED eos_marker;
> +        } QEMU_PACKED footer;
> +
> +        ret = bdrv_pread(file,
> +            bs->file->total_sectors * 512 - 1536,
> +            &footer, sizeof(footer));
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        /* Some sanity checks for the footer */
> +        if (be32_to_cpu(footer.magic) != VMDK4_MAGIC ||
> +            le32_to_cpu(footer.footer_marker.size) != 0  ||
> +            le32_to_cpu(footer.footer_marker.type) != MARKER_FOOTER ||
> +            le64_to_cpu(footer.eos_marker.val) != 0  ||
> +            le32_to_cpu(footer.eos_marker.size) != 0  ||
> +            le32_to_cpu(footer.eos_marker.type) != MARKER_END_OF_STREAM)
> +        {
> +            return -EINVAL;
> +        }
> +
> +        header = footer.header;
> +    }
> +
>      l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gte)
>                          * le64_to_cpu(header.granularity);
>      if (l1_entry_sectors == 0) {
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>