[PATCH qemu] block: Stricter checks of VMDK descriptors but allow unquoted createdType

Christian Biere posted 1 patch 1 month, 2 weeks ago
Failed in applying to current master (apply log)
[PATCH qemu] block: Stricter checks of VMDK descriptors but allow unquoted createdType
Posted by Christian Biere 1 month, 2 weeks ago
Veeam, for example, creates VMDK exports where the line declaring the 
createType property isn't surrounded by double-quotes. The resulting 
error message is cryptic but indicates that the parser of qemu-img 
doesn't check if createType is follow by a '=' character but just skips 
over it, the same goes for the initial quote. The patch adds checks for 
the '=' character for all properties but relaxes the requirement to quote 
the value of createType. I came across this due to a post in the Proxmox 
forum where the issue was reported 10 years ago before.

https://forum.proxmox.com/threads/import-disk-vmdk-qcow2-unsupported-image-type-onolithicflat.135903/

Signed-off-by: Christian Biere <christianbiere@gmx.de>
diff --git a/block/vmdk.c b/block/vmdk.c
index cd8b4ec7c8..8153e007b9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -305,7 +305,6 @@ vmdk_read_cid(BlockDriverState *bs, int parent, 
uint32_t *pcid)
     char *desc;
     uint32_t cid;
     const char *p_name, *cid_str;
-    size_t cid_str_size;
     BDRVVmdkState *s = bs->opaque;
     int ret;

@@ -316,11 +315,9 @@ vmdk_read_cid(BlockDriverState *bs, int parent, 
uint32_t *pcid)
     }

     if (parent) {
-        cid_str = "parentCID";
-        cid_str_size = sizeof("parentCID");
+        cid_str = "parentCID=";
     } else {
-        cid_str = "CID";
-        cid_str_size = sizeof("CID");
+        cid_str = "CID=";
     }

     desc[DESC_SIZE - 1] = '\0';
@@ -329,7 +326,7 @@ vmdk_read_cid(BlockDriverState *bs, int parent, 
uint32_t *pcid)
         ret = -EINVAL;
         goto out;
     }
-    p_name += cid_str_size;
+    p_name += strlen(cid_str);
     if (sscanf(p_name, "%" SCNx32, &cid) != 1) {
         ret = -EINVAL;
         goto out;
@@ -345,6 +342,7 @@ out:
static int coroutine_fn GRAPH_RDLOCK
vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
{
+    const char *substr;
     char *desc, *tmp_desc;
     char *p_name, *tmp_str;
     BDRVVmdkState *s = bs->opaque;
@@ -370,16 +368,18 @@ vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
     }

     desc[desc_buf_size - 1] = '\0';
-    tmp_str = strstr(desc, "parentCID");
+    substr = "parentCID=";
+    tmp_str = strstr(desc, substr);
     if (tmp_str == NULL) {
         ret = -EINVAL;
         goto out;
     }

     pstrcpy(tmp_desc, desc_buf_size, tmp_str);
-    p_name = strstr(desc, "CID");
+    substr = "CID=";
+    p_name = strstr(desc, substr);
     if (p_name != NULL) {
-        p_name += sizeof("CID");
+        p_name += strlen(substr);
         snprintf(p_name, desc_buf_size - (p_name - desc), "%" PRIx32 
"\n", cid);
         pstrcat(desc, desc_buf_size, tmp_desc);
     }
@@ -485,6 +485,7 @@ static void vmdk_reopen_abort(BDRVReopenState *state)

static int GRAPH_RDLOCK vmdk_parent_open(BlockDriverState *bs)
{
+    const char *substr;
     char *p_name;
     char *desc;
     BDRVVmdkState *s = bs->opaque;
@@ -496,11 +497,12 @@ static int GRAPH_RDLOCK 
vmdk_parent_open(BlockDriverState *bs)
         goto out;
     }

-    p_name = strstr(desc, "parentFileNameHint");
+    substr = "parentFileNameHint=\"";
+    p_name = strstr(desc, substr);
     if (p_name != NULL) {
         char *end_name;

-        p_name += sizeof("parentFileNameHint") + 1;
+        p_name += strlen(substr);
         end_name = strchr(p_name, '\"');
         if (end_name == NULL) {
             ret = -EINVAL;
@@ -516,6 +518,8 @@ static int GRAPH_RDLOCK 
vmdk_parent_open(BlockDriverState *bs)
                 bs->auto_backing_file);
         pstrcpy(bs->backing_format, sizeof(bs->backing_format),
                 "vmdk");
+    } else {
+        ret = -EINVAL;
     }

out:
@@ -1096,14 +1100,30 @@ static int vmdk_parse_description(const char 
*desc, const char *opt_name,
     if (!opt_pos) {
         return VMDK_ERROR;
     }
-    /* Skip "=\"" following opt_name */
-    opt_pos += strlen(opt_name) + 2;
-    if (opt_pos >= end) {
+    opt_pos += strlen(opt_name);
+    if (*opt_pos++ != '=') {
+        /* Skip "=" following opt_name */
         return VMDK_ERROR;
     }
-    opt_end = opt_pos;
-    while (opt_end < end && *opt_end != '"') {
-        opt_end++;
+    if (*opt_pos == '"') {
+        /* Skip "=" following opt_name */
+        opt_end = ++opt_pos;
+        while (opt_end < end && *opt_end != '"') {
+            opt_end++;
+        }
+    } else {
+        /* Assume the value is unquoted but line-terminated */
+        opt_end = opt_pos;
+        while (opt_end < end && *opt_end != '\n') {
+            opt_end++;
+        }
+        if (*opt_end == '\n' && opt_end[-1] == '\r') {
+            /* skip '\r' if windows line endings used. */
+            opt_end--;
+        }
+    }
+    if (opt_end >= end) {
+        return VMDK_ERROR;
     }
     if (opt_end == end || buf_size < opt_end - opt_pos + 1) {
         return VMDK_ERROR;