Attachment 924360 Details for Bug 1126976 – Draft endian fix patch for VHDX

[patch] Draft endian fix patch for VHDX

0001-Big-endian-fixes-for-VHDX-driver.patch (text/plain), 6.40 KB, created by David Gibson on 2014-08-06 06:35:57 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Gibson

Created: 2014-08-06 06:35:57 UTC

Size: 6.40 KB

patch

obsolete

>From 0ed652d7c69f3e74c7b23765c96b6a381263259e Mon Sep 17 00:00:00 2001
>From: David Gibson <david@gibson.dropbear.id.au>
>Date: Wed, 6 Aug 2014 16:33:53 +1000
>Subject: [PATCH] Big-endian fixes for VHDX driver
>
>The VHDX block backend has an attempt at being (host) endian safe, but it
>doesn't quite get there.  There are two main problems:
>
>  - Some magic numbers are checked against strings with memcmp() *after*
>we've endian flipped those fields, which clearly won't work.
>
>  - Updating of CRCs is incorrectly handled.  vhdx_update_checksum()
>computed the CRC in cpu endian and expected it to be flipped later by
>vhdx_header_le_export().  But that doesn't work, because it implies we're
>computing the checksum on cpu-endian data, which will give the wrong
>result.  By contrast on the read side it was already correct, doing the
>checksum validation *before* endian import.
>
>This patch fixes both problems.  There may well be further endian bugs, but
>this is at least enough to get a trivial VHDX created on x86 to pass
>qemu-img check on ppc64 and vice versa.
>
>I also change vhdx_update_checksum() to return void, since with this change
>the returned crc value wasn't used in a meaningful capacity by any of the
>callers.
>
>Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>---
> block/vhdx.c | 37 +++++++++++++++++--------------------
> block/vhdx.h |  2 +-
> 2 files changed, 18 insertions(+), 21 deletions(-)
>
>diff --git a/block/vhdx.c b/block/vhdx.c
>index fedcf9f..9d5b80b 100644
>--- a/block/vhdx.c
>+++ b/block/vhdx.c
>@@ -135,12 +135,12 @@ typedef struct VHDXSectorInfo {
>  * buf: buffer pointer
>  * size: size of buffer (must be > crc_offset+4)
>  *
>- * Note: The resulting checksum is in the CPU endianness, not necessarily
>- *       in the file format endianness (LE).  Any header export to disk should
>- *       make sure that vhdx_header_le_export() is used to convert to the
>- *       correct endianness
>+ * Note: This expects input data in file-format endianness (LE) and
>+         likewise outputs the header in file-format endiannness.  It
>+         must be called *after* converting from CPU to file endianness
>+         with vhdx_header_le_export().
>  */
>-uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset)
>+void vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset)
> {
>     uint32_t crc;
> 
>@@ -148,10 +148,8 @@ uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset)
>     assert(size > (crc_offset + sizeof(crc)));
> 
>     memset(buf + crc_offset, 0, sizeof(crc));
>-    crc =  crc32c(0xffffffff, buf, size);
>+    crc =  cpu_to_le32(crc32c(0xffffffff, buf, size));
>     memcpy(buf + crc_offset, &crc, sizeof(crc));
>-
>-    return crc;
> }
> 
> uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
>@@ -321,11 +319,11 @@ static int vhdx_write_header(BlockDriverState *bs_file, VHDXHeader *hdr,
>     }
> 
>     /* overwrite the actual VHDXHeader portion */
>-    memcpy(buffer, hdr, sizeof(VHDXHeader));
>-    hdr->checksum = vhdx_update_checksum(buffer, VHDX_HEADER_SIZE,
>-                                         offsetof(VHDXHeader, checksum));
>     vhdx_header_le_export(hdr, &header_le);
>-    ret = bdrv_pwrite_sync(bs_file, offset, &header_le, sizeof(VHDXHeader));
>+    memcpy(buffer, &header_le, sizeof(VHDXHeader));
>+    vhdx_update_checksum(buffer, VHDX_HEADER_SIZE,
>+                         offsetof(VHDXHeader, checksum));
>+    ret = bdrv_pwrite_sync(bs_file, offset, buffer, sizeof(VHDXHeader));
> 
> exit:
>     qemu_vfree(buffer);
>@@ -435,7 +433,7 @@ static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
>     vhdx_header_le_import(header1);
> 
>     if (vhdx_checksum_is_valid(buffer, VHDX_HEADER_SIZE, 4) &&
>-        !memcmp(&header1->signature, "head", 4)             &&
>+        header1->signature == VHDX_HEADER_SIGNATURE         &&
>         header1->version == 1) {
>         h1_seq = header1->sequence_number;
>         h1_valid = true;
>@@ -450,7 +448,7 @@ static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
>     vhdx_header_le_import(header2);
> 
>     if (vhdx_checksum_is_valid(buffer, VHDX_HEADER_SIZE, 4) &&
>-        !memcmp(&header2->signature, "head", 4)             &&
>+        header2->signature == VHDX_HEADER_SIGNATURE         &&
>         header2->version == 1) {
>         h2_seq = header2->sequence_number;
>         h2_valid = true;
>@@ -523,7 +521,7 @@ static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
>     offset += sizeof(s->rt);
> 
>     if (!vhdx_checksum_is_valid(buffer, VHDX_HEADER_BLOCK_SIZE, 4) ||
>-        memcmp(&s->rt.signature, "regi", 4)) {
>+        s->rt.signature != VHDX_REGION_SIGNATURE) {
>         ret = -EINVAL;
>         goto fail;
>     }
>@@ -630,7 +628,7 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
> 
>     vhdx_metadata_header_le_import(&s->metadata_hdr);
> 
>-    if (memcmp(&s->metadata_hdr.signature, "metadata", 8)) {
>+    if (s->metadata_hdr.signature != VHDX_METADATA_SIGNATURE) {
>         ret = -EINVAL;
>         goto exit;
>     }
>@@ -1674,10 +1672,6 @@ static int vhdx_create_new_region_table(BlockDriverState *bs,
>     rt_metadata->length      = 1 * MiB; /* min size, and more than enough */
>     *metadata_offset = rt_metadata->file_offset;
> 
>-    vhdx_update_checksum(buffer, VHDX_HEADER_BLOCK_SIZE,
>-                         offsetof(VHDXRegionTableHeader, checksum));
>-
>-
>     /* The region table gives us the data we need to create the BAT,
>      * so do that now */
>     ret = vhdx_create_bat(bs, s, image_size, type, use_zero_blocks, rt_bat);
>@@ -1687,6 +1681,9 @@ static int vhdx_create_new_region_table(BlockDriverState *bs,
>     vhdx_region_entry_le_export(rt_bat);
>     vhdx_region_entry_le_export(rt_metadata);
> 
>+    vhdx_update_checksum(buffer, VHDX_HEADER_BLOCK_SIZE,
>+                         offsetof(VHDXRegionTableHeader, checksum));
>+
>     ret = bdrv_pwrite(bs, VHDX_REGION_TABLE_OFFSET, buffer,
>                       VHDX_HEADER_BLOCK_SIZE);
>     if (ret < 0) {
>diff --git a/block/vhdx.h b/block/vhdx.h
>index 5370010..6ffbd0c 100644
>--- a/block/vhdx.h
>+++ b/block/vhdx.h
>@@ -405,7 +405,7 @@ void vhdx_guid_generate(MSGUID *guid);
> int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s, bool rw,
>                         MSGUID *log_guid);
> 
>-uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset);
>+void vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset);
> uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
>                             int crc_offset);
> 
>-- 
>1.9.3
>

Actions: View | Diff

Attachments on bug 1126976: 924360