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