Skip to contentSkip to conversation

i386: fix handling of multiboot modules

PATCH In review Daniel P. Berrangé 5 participants 17 messages May 14, 2018 – Aug 16, 2018

lore
Share & tools
lore
#0 lore raw

cover i386: fix handling of multiboot modules

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0
#1 lore raw

PATCH 1/3 i386: fix regression parsing multiboot initrd modules

The logic for parsing the multiboot initrd modules was messed up in

commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100

opts: don't silently truncate long option values

Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to get_opt_value() which was not robust at accepting a NULL value.

---
 hw/i386/multiboot.c | 3 +--
 util/qemu-option.c  | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)
hw/i386/multiboot.c +1 -2
index 7a2953e26f..8e26545814 100644
@@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     cmdline_len += strlen(kernel_cmdline) + 1;
     if (initrd_filename) {
         const char *r = get_opt_value(initrd_filename, NULL);
-        cmdline_len += strlen(r) + 1;
-        mbs.mb_mods_avail = 1;
+        cmdline_len += strlen(initrd_filename) + 1;
         while (1) {
             mbs.mb_mods_avail++;
             r = get_opt_value(r, NULL);
util/qemu-option.c +3 -1
index 58d1c23893..8a68bc2314 100644
@@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)
     size_t capacity = 0, length;
     const char *offset;
 
-    *value = NULL;
+    if (value) {
+        *value = NULL;
+    }
     while (1) {
         offset = strchr(p, ',');
         if (!offset) {
Signature · 1 line
2.17.0
#2 lore raw

Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules

Quoted 37 lines · Daniel P. Berrangé

The logic for parsing the multiboot initrd modules was messed up in

commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100

opts: don't silently truncate long option values

Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to get_opt_value() which was not robust at accepting a NULL value.

---
 hw/i386/multiboot.c | 3 +--
 util/qemu-option.c  | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)
hw/i386/multiboot.c +1 -2
index 7a2953e26f..8e26545814 100644
@@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     cmdline_len += strlen(kernel_cmdline) + 1;
     if (initrd_filename) {
         const char *r = get_opt_value(initrd_filename, NULL);
-        cmdline_len += strlen(r) + 1;
-        mbs.mb_mods_avail = 1;
+        cmdline_len += strlen(initrd_filename) + 1;
         while (1) {
             mbs.mb_mods_avail++;
             r = get_opt_value(r, NULL);
util/qemu-option.c +0 -0
index 58d1c23893..8a68bc2314 100644

util/qemu-option.c const char *get_opt_value(const char *p, char **value) · line 83

Quoted 10 lines
util/qemu-option.c +3 -1
@@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)
     size_t capacity = 0, length;
     const char *offset;
 
-    *value = NULL;
+    if (value) {
+        *value = NULL;
+    }
     while (1) {
         offset = strchr(p, ',');
         if (!offset) {

Don't we delete this check again in patch 3? If we're going to fix this by making multiboot.c not pass in NULL pointers, is there a reason not to simply do that?

thanks
-- PMM

#3 lore raw

Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules

Quoted 41 lines · Peter Maydell
Quoted 40 lines · Daniel P. Berrangé

The logic for parsing the multiboot initrd modules was messed up in

commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100

opts: don't silently truncate long option values

Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to get_opt_value() which was not robust at accepting a NULL value.

---
 hw/i386/multiboot.c | 3 +--
 util/qemu-option.c  | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)
hw/i386/multiboot.c +1 -2
index 7a2953e26f..8e26545814 100644
@@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     cmdline_len += strlen(kernel_cmdline) + 1;
     if (initrd_filename) {
         const char *r = get_opt_value(initrd_filename, NULL);
-        cmdline_len += strlen(r) + 1;
-        mbs.mb_mods_avail = 1;
+        cmdline_len += strlen(initrd_filename) + 1;
         while (1) {
             mbs.mb_mods_avail++;
             r = get_opt_value(r, NULL);
util/qemu-option.c +0 -0
index 58d1c23893..8a68bc2314 100644
@@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)
     size_t capacity = 0, length;
     const char *offset;
 
Quoted 11 lines

util/qemu-option.c const char *get_opt_value(const char *p, char **value) · line 83

Quoted 7 lines
util/qemu-option.c +3 -1
-    *value = NULL;
+    if (value) {
+        *value = NULL;
+    }
     while (1) {
         offset = strchr(p, ',');
         if (!offset) {

Don't we delete this check again in patch 3? If we're going to fix this by making multiboot.c not pass in NULL pointers, is there a reason not to simply do that?

That is correct, however, I wanted to do a really simple fix that addressed the crasher regression, so that aspect of the fix wasn't obscured by the bigger refactoring I do in 2 & 3.

Regards,
Daniel

Signature · 3 lines
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Reviewed-by on v1 1/3
#4 lore raw

Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules

In reply to Daniel P. Berrangé
Quoted 6 lines · Daniel P. Berrangé

The logic for parsing the multiboot initrd modules was messed up in

commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100

Quoted 7 lines

opts: don't silently truncate long option values

Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to get_opt_value() which was not robust at accepting a NULL value.

Queueing on x86-next. Thanks.

Signature · 1 line
Eduardo
#5 lore raw

PATCH 2/3 i386: only parse the initrd_filename once for multiboot modules

In reply to Daniel P. Berrangé

The multiboot code parses the initrd_filename twice, first to count how many entries there are, and second to process each entry. This changes the first loop to store the parse module names in a list, and the second loop can now use these names. This avoids having to pass NULL to the get_opt_value() method which means it can safely assume a non-NULL param.

---
 hw/i386/multiboot.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)
hw/i386/multiboot.c +15 -17
index 8e26545814..d519e206c5 100644
@@ -161,6 +161,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     uint8_t bootinfo[MBI_SIZE];
     uint8_t *mb_bootinfo_data;
     uint32_t cmdline_len;
+    GList *mods = NULL;
 
     /* Ok, let's see if it is a multiboot image.
        The header is 12x32bit long, so the latest entry may be 8192 - 48. */
@@ -291,15 +292,16 @@ int load_multiboot(FWCfgState *fw_cfg,
     cmdline_len = strlen(kernel_filename) + 1;
     cmdline_len += strlen(kernel_cmdline) + 1;
     if (initrd_filename) {
-        const char *r = get_opt_value(initrd_filename, NULL);
+        const char *r = initrd_filename;
         cmdline_len += strlen(initrd_filename) + 1;
-        while (1) {
+        while (*r) {
+            char *value;
+            r = get_opt_value(r, &value);
             mbs.mb_mods_avail++;
-            r = get_opt_value(r, NULL);
-            if (!*r) {
-                break;
+            mods = g_list_append(mods, value);
+            if (*r) {
+                r++;
             }
-            r++;
         }
     }
 
@@ -314,20 +316,16 @@ int load_multiboot(FWCfgState *fw_cfg,
     mbs.offset_cmdlines   = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
     mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
 
-    if (initrd_filename) {
-        const char *next_initrd;
-        char not_last;
-        char *one_file = NULL;
-
+    if (mods) {
+        GList *tmpl = mods;
         mbs.offset_mods = mbs.mb_buf_size;
 
-        do {
+        while (tmpl) {
             char *next_space;
             int mb_mod_length;
             uint32_t offs = mbs.mb_buf_size;
+            char *one_file = tmpl->data;
 
-            next_initrd = get_opt_value(initrd_filename, &one_file);
-            not_last = *next_initrd;
             /* if a space comes after the module filename, treat everything
                after that as parameters */
             hwaddr c = mb_add_cmdline(&mbs, one_file);
@@ -352,10 +350,10 @@ int load_multiboot(FWCfgState *fw_cfg,
             mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx,
                      (char *)mbs.mb_buf + offs,
                      (char *)mbs.mb_buf + offs + mb_mod_length, c);
-            initrd_filename = next_initrd+1;
             g_free(one_file);
-            one_file = NULL;
-        } while (not_last);
+            tmpl = tmpl->next;
+        }
+        g_list_free(mods);
     }
 
     /* Commandline support */
Signature · 1 line
2.17.0
Reviewed-by on v1 2/3
#6 lore raw

Re: [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules

Quoted 7 lines · Daniel P. Berrangé

The multiboot code parses the initrd_filename twice, first to count how many entries there are, and second to process each entry. This changes the first loop to store the parse module names in a list, and the second loop can now use these names. This avoids having to pass NULL to the get_opt_value() method which means it can safely assume a non-NULL param.

[...]

hw/i386/multiboot.c int load_multiboot(FWCfgState *fw_cfg, · line 356

Quoted 11 lines
hw/i386/multiboot.c +3 -3
@@ -352,10 +350,10 @@ int load_multiboot(FWCfgState *fw_cfg,
             mb_debug("mod_start: %p\nmod_end:   %p\n  cmdline: "TARGET_FMT_plx,
                      (char *)mbs.mb_buf + offs,
                      (char *)mbs.mb_buf + offs + mb_mod_length, c);
-            initrd_filename = next_initrd+1;
             g_free(one_file);
-            one_file = NULL;
-        } while (not_last);
+            tmpl = tmpl->next;
+        }
+        g_list_free(mods);

I thought this would cause double free of tmpl->data, but g_list_free() won't free the elements' data.

Queueing on x86-next.

Signature · 1 line
Eduardo
#7 lore raw

PATCH 3/3 opts: remove redundant check for NULL parameter

In reply to Daniel P. Berrangé

No callers of get_opt_value() pass in a NULL for the "value" parameter, so the check is redundant.

---
 util/qemu-option.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)
util/qemu-option.c +4 -8
index 8a68bc2314..a1ff682aac 100644
@@ -75,9 +75,7 @@ const char *get_opt_value(const char *p, char **value)
     size_t capacity = 0, length;
     const char *offset;
 
-    if (value) {
-        *value = NULL;
-    }
+    *value = NULL;
     while (1) {
         offset = strchr(p, ',');
         if (!offset) {
@@ -88,11 +86,9 @@ const char *get_opt_value(const char *p, char **value)
         if (*offset != '\0' && *(offset + 1) == ',') {
             length++;
         }
-        if (value) {
-            *value = g_renew(char, *value, capacity + length + 1);
-            strncpy(*value + capacity, p, length);
-            (*value)[capacity + length] = '\0';
-        }
+        *value = g_renew(char, *value, capacity + length + 1);
+        strncpy(*value + capacity, p, length);
+        (*value)[capacity + length] = '\0';
         capacity += length;
         if (*offset == '\0' ||
             *(offset + 1) != ',') {
Signature · 1 line
2.17.0
#9 lore raw
In reply to Daniel P. Berrangé

ping...

Quoted 15 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0

Regards,
Daniel

Signature · 3 lines
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
#10 lore raw
Quoted 17 lines · Daniel P. Berrangé

ping...

Quoted 14 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0

Any reason this hasn't been merged yet?

This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.

Thanks,
Roman.

#11 lore raw
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé

ping...

Quoted 8 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0

Any reason this hasn't been merged yet?

Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.

Quoted 3 lines

This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.

As it is a bug fix, I will try to review merge it for the next rc.

Signature · 1 line
Eduardo
#12 lore raw
Quoted 26 lines · Eduardo Habkost
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé

ping...

Quoted 8 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0

Any reason this hasn't been merged yet?

Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.

Quoted 1 line
Quoted 5 lines
Quoted 2 lines

This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.

As it is a bug fix, I will try to review merge it for the next rc.

Looks like it ended up missing 3.0, so kvm-unit-tests still segfault with the released version of QEMU :(

Roman.

#13 lore raw
Quoted 27 lines · Roman Kagan
Quoted 26 lines · Eduardo Habkost
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé

ping...

Quoted 8 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0

Any reason this hasn't been merged yet?

Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.

Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.

As it is a bug fix, I will try to review merge it for the next rc.

Looks like it ended up missing 3.0, so kvm-unit-tests still segfault with the released version of QEMU :(

Please check again, as I see it merged before 3.0:

commit 59b5552f020b739e273e969a0933c23d8f4e2284
Merge: ccf02d73d1 dfaa7d50b0
Author: Peter Maydell <[email protected]>
Date: Tue Jul 17 17:06:32 2018 +0100

Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging

Bug fixes.

# gpg: Signature made Tue 17 Jul 2018 16:06:07 BST
# gpg: using RSA key BFFBD25F78C7AE83
# gpg: Good signature from "Paolo Bonzini <[email protected]>"
# gpg: aka "Paolo Bonzini <[email protected]>"
# Primary key fingerprint: 46F5 9FBD 57D6 12E7 BFD4 E2F7 7E15 100C CD36 69B1
# Subkey fingerprint: F133 3857 4B66 2389 866C 7682 BFFB D25F 78C7 AE83

* remotes/bonzini/tags/for-upstream:
Document command line options with single dash
opts: remove redundant check for NULL parameter
i386: only parse the initrd_filename once for multiboot modules
i386: fix regression parsing multiboot initrd modules
virtio-scsi: fix hotplug ->reset() vs event race
qdev: add HotplugHandler->post_plug() callback
hw/char/serial: retry write if EAGAIN
PC Chipset: Improve serial divisor calculation
vhost-user-test: added proper TestServer *dest initialization in test_migrate()
hyperv: ensure VP index equal to QEMU cpu_index
hyperv: rename vcpu_id to vp_index
accel: Fix typo and grammar in comment
dump: add kernel_gs_base to QEMU CPU state

$ git describe  59b5552f020b739e273e969a0933c23d8f4e2284
v3.0.0-rc0-80-g59b5552f02

Regards,
Daniel

Signature · 3 lines
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
#14 lore raw
Quoted 31 lines · Daniel P. Berrangé
Quoted 30 lines · Roman Kagan
Quoted 29 lines · Eduardo Habkost
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé

ping...

Quoted 8 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0

Any reason this hasn't been merged yet?

Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.

Quoted 3 lines

This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

As it is a bug fix, I will try to review merge it for the next rc.

Looks like it ended up missing 3.0, so kvm-unit-tests still segfault with the released version of QEMU :(

Please check again, as I see it merged before 3.0:

Oh, you're right, I just ran the tests against a wrong QEMU.

It's OK now indeed.

Sorry for the noise,
Roman.

Tested-by on the v1 series
#15 lore raw
In reply to Daniel P. Berrangé
Quoted 2 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

This flaw also makes QEMU segfault when running kvm-unit-tests, and the series fixes it.

So

Wondering if anyone else runs kvm-unit-tests with QEMU master, Roman.

Quoted 14 lines

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0
#16 lore raw
Quoted 7 lines · Roman Kagan
Quoted 2 lines · Daniel P. Berrangé

This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.

This flaw also makes QEMU segfault when running kvm-unit-tests, and the series fixes it.

Quoted 6 lines

So

Wondering if anyone else runs kvm-unit-tests with QEMU master, Roman.

It might be a good idea to make it a submodule, and use it for unit tests. I'll take a look.

Quoted 14 lines
Quoted 14 lines

Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter

 hw/i386/multiboot.c | 35 ++++++++++++++++-------------------
 util/qemu-option.c  |  8 +++-----
 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0