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

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

[Vlad Lungu]

get_opt_value() truncates the value at the first comma
Use memcpy() instead

Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
---
  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);
+    memcpy(b, cmdline, strlen(cmdline) + 1);
      s->offset_cmdlines += strlen(b) + 1;
      return s->mb_buf_phys + p;
  }
-- 
1.9.1

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 <vlad.lungu@windriver.com>

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

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

[Vlad Lungu]

get_opt_value() truncates the value at the first comma.
Use memcpy() instead.

Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
---
 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);
+    memcpy(b, cmdline, strlen(cmdline) + 1);
     s->offset_cmdlines += strlen(b) + 1;
     return s->mb_buf_phys + p;
 }
-- 
1.9.1

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

[Paolo Bonzini]

On 14/12/2016 17:19, Vlad Lungu wrote:
> get_opt_value() truncates the value at the first comma.
> Use memcpy() instead.

Looks good since get_opt_value is already used by the caller.  Have you
tested this with multiple initrd modules too?

Paolo

> Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
> ---
>  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);
> +    memcpy(b, cmdline, strlen(cmdline) + 1);
>      s->offset_cmdlines += strlen(b) + 1;
>      return s->mb_buf_phys + p;
>  }
> 

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

[Eduardo Habkost]

On Wed, Dec 14, 2016 at 05:55:07PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/12/2016 17:19, Vlad Lungu wrote:
> > get_opt_value() truncates the value at the first comma.
> > Use memcpy() instead.
> 
> Looks good since get_opt_value is already used by the caller.  Have you
> tested this with multiple initrd modules too?

get_opt_value() is used by the caller, but with NULL buf
argument. This means the caller doesn't handle ",," escaping.
(See my reply to the previous submission of this patch)

However this only means the caller is buggy, not
mb_add_cmdline(). So the change still looks good to me.

> 
> Paolo
> 
> > Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
> > ---
> >  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);
> > +    memcpy(b, cmdline, strlen(cmdline) + 1);
> >      s->offset_cmdlines += strlen(b) + 1;
> >      return s->mb_buf_phys + p;
> >  }
> > 

-- 
Eduardo

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

[Paolo Bonzini]

On 14/12/2016 18:00, Eduardo Habkost wrote:
> On Wed, Dec 14, 2016 at 05:55:07PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 14/12/2016 17:19, Vlad Lungu wrote:
>>> get_opt_value() truncates the value at the first comma.
>>> Use memcpy() instead.
>>
>> Looks good since get_opt_value is already used by the caller.  Have you
>> tested this with multiple initrd modules too?
> 
> get_opt_value() is used by the caller, but with NULL buf
> argument. This means the caller doesn't handle ",," escaping.
> (See my reply to the previous submission of this patch)

Hmm, wait.  When NULL is passed, ",," escaping is handled correctly in
that next_initrd points to the next lone comma.  The lone comma is
replaced with a '\0' by the caller.

So you need to use get_opt_value again in mb_add_cmdline to do the
unescaping, because mb_add_cmdline only receives double commas.

This was actually my first reaction to the patch, and it was correct.
Then I overthought it. :)

So the patch is wrong.

Paolo

> However this only means the caller is buggy, not
> mb_add_cmdline(). So the change still looks good to me.
> 
>>
>> Paolo
>>
>>> Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
>>> ---
>>>  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);
>>> +    memcpy(b, cmdline, strlen(cmdline) + 1);
>>>      s->offset_cmdlines += strlen(b) + 1;
>>>      return s->mb_buf_phys + p;
>>>  }
>>>
> 

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

[Eduardo Habkost]

On Wed, Dec 14, 2016 at 06:20:46PM +0100, Paolo Bonzini wrote:
> On 14/12/2016 18:00, Eduardo Habkost wrote:
> > On Wed, Dec 14, 2016 at 05:55:07PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 14/12/2016 17:19, Vlad Lungu wrote:
> >>> get_opt_value() truncates the value at the first comma.
> >>> Use memcpy() instead.
> >>
> >> Looks good since get_opt_value is already used by the caller.  Have you
> >> tested this with multiple initrd modules too?
> > 
> > get_opt_value() is used by the caller, but with NULL buf
> > argument. This means the caller doesn't handle ",," escaping.
> > (See my reply to the previous submission of this patch)
> 
> Hmm, wait.  When NULL is passed, ",," escaping is handled correctly in
> that next_initrd points to the next lone comma.  The lone comma is
> replaced with a '\0' by the caller.

It is handled correctly when splitting the string, but not when opening the
file.

> 
> So you need to use get_opt_value again in mb_add_cmdline to do the
> unescaping, because mb_add_cmdline only receives double commas.
> 
> This was actually my first reaction to the patch, and it was correct.
> Then I overthought it. :)
> 
> So the patch is wrong.

Except that the caller is already broken when using ",," for a
different reason: it calls get_image_size(initrd_filename) and
load_image(initrd_filename, ...) directly. So comma-escaping
never worked anyway:

  $ qemu-system-x86_64 -kernel ~/Downloads/gnumach -initrd /tmp/one,,file,/tmp/another,,file
  Failed to open file '/tmp/one,,file'

The right fix for comma-escaping would require calling
get_opt_value() with non-NULL buf outside mb_add_cmdline(),
because the mb_add_cmdline(&mbs, kcmdline) call do NOT need
get_opt_value() to be called.

In other words: this fixes the mb_add_cmdline(kcmdline) case, and
doesn't break comma escaping on the initrd case (because it was
already broken). I don't see a problem with this patch.

> 
> Paolo
> 
> > However this only means the caller is buggy, not
> > mb_add_cmdline(). So the change still looks good to me.
> > 
> >>
> >> Paolo
> >>
> >>> Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
> >>> ---
> >>>  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);
> >>> +    memcpy(b, cmdline, strlen(cmdline) + 1);
> >>>      s->offset_cmdlines += strlen(b) + 1;
> >>>      return s->mb_buf_phys + p;
> >>>  }
> >>>
> > 

-- 
Eduardo

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

[Paolo Bonzini]

----- Original Message -----
> From: "Eduardo Habkost" <ehabkost@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Vlad Lungu" <vlad.lungu@windriver.com>, qemu-devel@nongnu.org, rth@twiddle.net
> Sent: Wednesday, December 14, 2016 6:51:44 PM
> Subject: Re: [PATCH] multiboot: copy the cmdline verbatim
> 
> On Wed, Dec 14, 2016 at 06:20:46PM +0100, Paolo Bonzini wrote:
> > On 14/12/2016 18:00, Eduardo Habkost wrote:
> > > On Wed, Dec 14, 2016 at 05:55:07PM +0100, Paolo Bonzini wrote:
> > >>
> > >>
> > >> On 14/12/2016 17:19, Vlad Lungu wrote:
> > >>> get_opt_value() truncates the value at the first comma.
> > >>> Use memcpy() instead.
> > >>
> > >> Looks good since get_opt_value is already used by the caller.  Have you
> > >> tested this with multiple initrd modules too?
> > > 
> > > get_opt_value() is used by the caller, but with NULL buf
> > > argument. This means the caller doesn't handle ",," escaping.
> > > (See my reply to the previous submission of this patch)
> > 
> > Hmm, wait.  When NULL is passed, ",," escaping is handled correctly in
> > that next_initrd points to the next lone comma.  The lone comma is
> > replaced with a '\0' by the caller.
> 
> It is handled correctly when splitting the string, but not when opening the
> file.
> 
> > 
> > So you need to use get_opt_value again in mb_add_cmdline to do the
> > unescaping, because mb_add_cmdline only receives double commas.
> > 
> > This was actually my first reaction to the patch, and it was correct.
> > Then I overthought it. :)
> > 
> > So the patch is wrong.
> 
> Except that the caller is already broken when using ",," for a
> different reason: it calls get_image_size(initrd_filename) and
> load_image(initrd_filename, ...) directly. So comma-escaping
> never worked anyway:
> 
>   $ qemu-system-x86_64 -kernel ~/Downloads/gnumach -initrd
>   /tmp/one,,file,/tmp/another,,file
>   Failed to open file '/tmp/one,,file'
> 
> The right fix for comma-escaping would require calling
> get_opt_value() with non-NULL buf outside mb_add_cmdline(),
> because the mb_add_cmdline(&mbs, kcmdline) call do NOT need
> get_opt_value() to be called.

For filenames containing commas you're right, but...

> In other words: this fixes the mb_add_cmdline(kcmdline) case, and
> doesn't break comma escaping on the initrd case (because it was
> already broken). I don't see a problem with this patch.

... there is one case of comma escaping that wasn't broken:

    $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one arg,,with,,commas,/tmp/another arg,,with,,commas'

And presumably this is what Vlad was trying to do.

Paolo

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

[Eduardo Habkost]

On Wed, Dec 14, 2016 at 04:38:07PM -0500, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Eduardo Habkost" <ehabkost@redhat.com>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>
> > Cc: "Vlad Lungu" <vlad.lungu@windriver.com>, qemu-devel@nongnu.org, rth@twiddle.net
> > Sent: Wednesday, December 14, 2016 6:51:44 PM
> > Subject: Re: [PATCH] multiboot: copy the cmdline verbatim
> > 
> > On Wed, Dec 14, 2016 at 06:20:46PM +0100, Paolo Bonzini wrote:
> > > On 14/12/2016 18:00, Eduardo Habkost wrote:
> > > > On Wed, Dec 14, 2016 at 05:55:07PM +0100, Paolo Bonzini wrote:
> > > >>
> > > >>
> > > >> On 14/12/2016 17:19, Vlad Lungu wrote:
> > > >>> get_opt_value() truncates the value at the first comma.
> > > >>> Use memcpy() instead.
> > > >>
> > > >> Looks good since get_opt_value is already used by the caller.  Have you
> > > >> tested this with multiple initrd modules too?
> > > > 
> > > > get_opt_value() is used by the caller, but with NULL buf
> > > > argument. This means the caller doesn't handle ",," escaping.
> > > > (See my reply to the previous submission of this patch)
> > > 
> > > Hmm, wait.  When NULL is passed, ",," escaping is handled correctly in
> > > that next_initrd points to the next lone comma.  The lone comma is
> > > replaced with a '\0' by the caller.
> > 
> > It is handled correctly when splitting the string, but not when opening the
> > file.
> > 
> > > 
> > > So you need to use get_opt_value again in mb_add_cmdline to do the
> > > unescaping, because mb_add_cmdline only receives double commas.
> > > 
> > > This was actually my first reaction to the patch, and it was correct.
> > > Then I overthought it. :)
> > > 
> > > So the patch is wrong.
> > 
> > Except that the caller is already broken when using ",," for a
> > different reason: it calls get_image_size(initrd_filename) and
> > load_image(initrd_filename, ...) directly. So comma-escaping
> > never worked anyway:
> > 
> >   $ qemu-system-x86_64 -kernel ~/Downloads/gnumach -initrd
> >   /tmp/one,,file,/tmp/another,,file
> >   Failed to open file '/tmp/one,,file'
> > 
> > The right fix for comma-escaping would require calling
> > get_opt_value() with non-NULL buf outside mb_add_cmdline(),
> > because the mb_add_cmdline(&mbs, kcmdline) call do NOT need
> > get_opt_value() to be called.
> 
> For filenames containing commas you're right, but...
> 
> > In other words: this fixes the mb_add_cmdline(kcmdline) case, and
> > doesn't break comma escaping on the initrd case (because it was
> > already broken). I don't see a problem with this patch.
> 
> ... there is one case of comma escaping that wasn't broken:
> 
>     $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one arg,,with,,commas,/tmp/another arg,,with,,commas'
> 

Oh, I didn't notice the whitespace-based split for initrd
arguments.

This is messier than I thought. Maybe the simplest solution is to
inline mb_add_cmdline() at both callers, and change the kcmdline
one to use memcpy().

> And presumably this is what Vlad was trying to do.

I believe he was trying to fix -append, not -initrd. The patch
fixes -append, but breaks comma-escaping on -initrd.

-- 
Eduardo

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

[Paolo Bonzini]

> > > In other words: this fixes the mb_add_cmdline(kcmdline) case, and
> > > doesn't break comma escaping on the initrd case (because it was
> > > already broken). I don't see a problem with this patch.
> > 
> > ... there is one case of comma escaping that wasn't broken:
> > 
> >     $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one
> >     arg,,with,,commas,/tmp/another arg,,with,,commas'
> > 
> 
> Oh, I didn't notice the whitespace-based split for initrd
> arguments.
> 
> This is messier than I thought. Maybe the simplest solution is to
> inline mb_add_cmdline() at both callers, and change the kcmdline
> one to use memcpy().

Yes, I agree.

Paolo

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

[Vlad Lungu]

On 12/15/2016 12:32 AM, Paolo Bonzini wrote:
>>>> In other words: this fixes the mb_add_cmdline(kcmdline) case, and
>>>> doesn't break comma escaping on the initrd case (because it was
>>>> already broken). I don't see a problem with this patch.
>>> ... there is one case of comma escaping that wasn't broken:
>>>
>>>     $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one
>>>     arg,,with,,commas,/tmp/another arg,,with,,commas'
>>>
>> Oh, I didn't notice the whitespace-based split for initrd
>> arguments.
So that's how it works :-)
>> This is messier than I thought. Maybe the simplest solution is to
>> inline mb_add_cmdline() at both callers, and change the kcmdline
>> one to use memcpy().
>

OK, I have a new version that does with memcpy for the cmdline, with
get_opt_value() for the modules and also unescapes the filename for
get_image_size()/load_image() . You still can't have spaces in filenames,
so maybe a new scheme should be devised for this.

Regards,
Vlad