Re: [Qemu-devel] [PATCH] multiboot: copy the cmdline verbatim

[Eduardo Habkost]

On Wed, Dec 14, 2016 at 03:35:29PM +0200, Vlad Lungu wrote:
> 
> get_opt_value() truncates the value at the first comma
> Use memcpy() instead
> 
> Signed-off-by: Vlad Lungu <address@hidden>

Your patch is corrupted. I suggest using git-send-email instead
of Thunderbird, but you might want to take a look at:
http://kb.mozillazine.org/Plain_text_e-mail_-_Thunderbird#Completely_plain_email

> ---
>  hw/i386/multiboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 387caa6..b4495ad 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -109,7 +109,7 @@ static uint32_t mb_add_cmdline(MultibootState *s, const
> char *cmdline)
>      hwaddr p = s->offset_cmdlines;
>      char *b = (char *)s->mb_buf + p;
> 
> -    get_opt_value(b, strlen(cmdline) + 1, cmdline);

One caller of mb_add_cmdline() relies on get_opt_value() to allow
multiple initrd modules to be specified on the command-line. But:
* The caller already splits the string manually by adding a null
  terminator;
* The caller is already broken if filenames contain "," (because
  it doesn't handle ",," escaping)
so it won't be affected by the change, and it would need to
handle escaping before calling mb_add_cmdline() anyway.

The other caller of mb_add_cmdline() (kcmdline) doesn't need
option splitting/parsing.

So, not using get_opt_value() here makes sense.

But:

> +    memcpy(b, cmdline, strlen(cmdline) + 1);

This is equivalent to strcpy(), and it is ignoring the size of
mb_buf.

The existing code looks very dangerous: the size of mb_buf is
calculated based on the image contents, and then it doesn't check
the buffer size before writing to it.

>      s->offset_cmdlines += strlen(b) + 1;
>      return s->mb_buf_phys + p;
>  }
> -- 
> 1.9.1
> 
> 

-- 
Eduardo