[PATCH] hw/i386: fix microvm segfault with virtio cmdline

[PATCH] hw/i386: fix microvm segfault with virtio cmdline

[Daniel Hoffman]

The 'microvm' machine type allows for disabling ACPI, in which case
the VirtIO device configuration is passed via appending it to the
kernel cmdline.

If no cmdline parameter was passed, then a null pointer is dereferenced when
the new cmdline is copied back. A solution is to always define the cmdline
in the fw_cfg so the read to append happens before the first write in the
multiboot case, and to explcitly re-write the value to update the length.

Fixes: eac7a7791b

Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
---
 hw/i386/microvm.c | 3 ++-
 hw/i386/x86.c     | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 29f30dd6d3..be64280530 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -417,7 +417,8 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
     if (len > VIRTIO_CMDLINE_TOTAL_MAX_LEN + strlen(existing_cmdline)) {
         fprintf(stderr, "qemu: virtio mmio cmdline too large, skipping\n");
     } else {
-        memcpy(existing_cmdline, cmdline, len + 1);
+	fw_cfg_modify_i32(x86ms->fw_cfg, FW_CFG_CMDLINE_SIZE, len + 1);
+	fw_cfg_modify_string(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA, cmdline);
     }
     g_free(cmdline);
 }
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index eaff4227bd..7dd02b7409 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -827,6 +827,10 @@ void x86_load_linux(X86MachineState *x86ms,
     /* Make a copy, since we might append arbitrary bytes to it later. */
     kernel_cmdline = g_strndup(machine->kernel_cmdline, cmdline_size);
 
+    /* If the cmdline is undefined, set it as an empty allocated value */
+    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, cmdline_size);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline, cmdline_size);
+
     /* load the kernel header */
     f = fopen(kernel_filename, "rb");
     if (!f) {
-- 
2.37.2

Re: [PATCH] hw/i386: fix microvm segfault with virtio cmdline

[Michael S. Tsirkin]

didn't read the patch yet but just formatting comments:

On Wed, Feb 22, 2023 at 10:39:10PM -0800, Daniel Hoffman wrote:
> The 'microvm' machine type allows for disabling ACPI, in which case
> the VirtIO device configuration is passed via appending it to the
> kernel cmdline.
> 
> If no cmdline parameter was passed, then a null pointer is dereferenced when
> the new cmdline is copied back. A solution is to always define the cmdline
> in the fw_cfg so the read to append happens before the first write in the
> multiboot case, and to explcitly re-write the value to update the length.

explicitly

> 
> Fixes: eac7a7791b

format is:

Fixes: hash ("subject")

> 
> Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
> ---
>  hw/i386/microvm.c | 3 ++-
>  hw/i386/x86.c     | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 29f30dd6d3..be64280530 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -417,7 +417,8 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
>      if (len > VIRTIO_CMDLINE_TOTAL_MAX_LEN + strlen(existing_cmdline)) {
>          fprintf(stderr, "qemu: virtio mmio cmdline too large, skipping\n");
>      } else {
> -        memcpy(existing_cmdline, cmdline, len + 1);
> +	fw_cfg_modify_i32(x86ms->fw_cfg, FW_CFG_CMDLINE_SIZE, len + 1);
> +	fw_cfg_modify_string(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA, cmdline);

Pls use spaces not tabs same as surrounding code.

>      }
>      g_free(cmdline);
>  }
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index eaff4227bd..7dd02b7409 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -827,6 +827,10 @@ void x86_load_linux(X86MachineState *x86ms,
>      /* Make a copy, since we might append arbitrary bytes to it later. */
>      kernel_cmdline = g_strndup(machine->kernel_cmdline, cmdline_size);
>  
> +    /* If the cmdline is undefined, set it as an empty allocated value */
> +    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, cmdline_size);
> +    fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline, cmdline_size);
> +
>      /* load the kernel header */
>      f = fopen(kernel_filename, "rb");
>      if (!f) {
> -- 
> 2.37.2