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

[Michael S. Tsirkin]

On Sat, Feb 25, 2023 at 04:27:57PM -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.
> 
> Fixes: eac7a7791b ("x86: don't let decompressed kernel image clobber 
> setup_data")
> 
> Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>

Hmm. My tree has a commit reverting this one.

I will send a pull request with that RSN, then ping you
to rebase and re-test. Thanks!


> ---
>  hw/i386/microvm.c | 3 ++-
>  hw/i386/x86.c     | 9 +++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 29f30dd6d3..587ca53def 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..ac6d921a6b 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -827,6 +827,15 @@ 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 no kernel cmdline was passed as a parameter, 
> machine->kernel_cmdline is
> +     * blank but fw_cfg is undefined. The microvm cmdline fiddling hack 
> requires
> +     * it to be defined, even if it is empty, as g_strndup(NULL) == NULL and 
> +     * g_strndup("") != NULL
> +     */
> +    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, 
> strlen(machine->kernel_cmdline) + 1);
> +    fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline, 
> strlen(machine->kernel_cmdline));
> +
>      /* load the kernel header */
>      f = fopen(kernel_filename, "rb");
>      if (!f) {
> -- 
> 2.37.2