[PATCH] pass kernel command line as verbatim

[PATCH] pass kernel command line as verbatim

[Michael Chang]

The command line has been processed by grub shell, then the result is expected
to be passed to kernel command line as verbatim according to the grub manual
[1][2].

This patch removes extra escape character added as it helps nothing but only
creates trouble as you want them to be literal. Besides the surrounding
double-quotes added is kept as it used to protect space.

[1] https://www.gnu.org/software/grub/manual/grub/html_node/linux.html#linux
[2] https://www.gnu.org/software/grub/manual/grub/html_node/xen_005fhypervisor.html

Signed-off-by: Michael Chang <mchang@suse.com>
---
 grub-core/lib/cmdline.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/grub-core/lib/cmdline.c b/grub-core/lib/cmdline.c
index d5e10ee87..26b8131c8 100644
--- a/grub-core/lib/cmdline.c
+++ b/grub-core/lib/cmdline.c
@@ -27,9 +27,7 @@ static unsigned int check_arg (char *c, int *has_space)
 
   while (*c)
     {
-      if (*c == '\\' || *c == '\'' || *c == '"')
-	size++;
-      else if (*c == ' ')
+      if (*c == ' ')
 	space = 1;
 
       size++;
@@ -84,13 +82,7 @@ int grub_create_loader_cmdline (int argc, char *argv[], char *buf,
 	*buf++ = '"';
 
       while (*c)
-	{
-	  if (*c == '\\' || *c == '\'' || *c == '"')
-	    *buf++ = '\\';
-
-	  *buf++ = *c;
-	  c++;
-	}
+	*buf++ = *c++;
 
       if (space)
 	*buf++ = '"';
-- 
2.16.1

Re: [PATCH] pass kernel command line as verbatim

[Michael Chang]

And this bug report seems relevant ..

https://savannah.gnu.org/bugs/?49937

On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> The command line has been processed by grub shell, then the result is expected
> to be passed to kernel command line as verbatim according to the grub manual
> [1][2].
> 
> This patch removes extra escape character added as it helps nothing but only
> creates trouble as you want them to be literal. Besides the surrounding
> double-quotes added is kept as it used to protect space.
> 
> [1] https://www.gnu.org/software/grub/manual/grub/html_node/linux.html#linux
> [2] https://www.gnu.org/software/grub/manual/grub/html_node/xen_005fhypervisor.html
> 
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  grub-core/lib/cmdline.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/grub-core/lib/cmdline.c b/grub-core/lib/cmdline.c
> index d5e10ee87..26b8131c8 100644
> --- a/grub-core/lib/cmdline.c
> +++ b/grub-core/lib/cmdline.c
> @@ -27,9 +27,7 @@ static unsigned int check_arg (char *c, int *has_space)
>  
>    while (*c)
>      {
> -      if (*c == '\\' || *c == '\'' || *c == '"')
> -	size++;
> -      else if (*c == ' ')
> +      if (*c == ' ')
>  	space = 1;
>  
>        size++;
> @@ -84,13 +82,7 @@ int grub_create_loader_cmdline (int argc, char *argv[], char *buf,
>  	*buf++ = '"';
>  
>        while (*c)
> -	{
> -	  if (*c == '\\' || *c == '\'' || *c == '"')
> -	    *buf++ = '\\';
> -
> -	  *buf++ = *c;
> -	  c++;
> -	}
> +	*buf++ = *c++;
>  
>        if (space)
>  	*buf++ = '"';
> -- 
> 2.16.1

Re: [PATCH] pass kernel command line as verbatim

[Daniel Kiper]

On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> And this bug report seems relevant ..
>
> https://savannah.gnu.org/bugs/?49937
>
> On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > The command line has been processed by grub shell, then the result is expected
> > to be passed to kernel command line as verbatim according to the grub manual
> > [1][2].
> >
> > This patch removes extra escape character added as it helps nothing but only
> > creates trouble as you want them to be literal. Besides the surrounding
> > double-quotes added is kept as it used to protect space.

CC-ing Vladmir.

Hmmm... Have you tested this patch on all platforms supported by GRUB2?
I understand that current behavior is not accepted on some but I have
a feeling that somebody make it work in that way due to some reason.
Sadly I cannot find anything about that in git log. So, I have to think
how to cope with that. Or...

Vladmir, do you know why command line is processed in that way?

Daniel

Re: [PATCH] pass kernel command line as verbatim

[Michael Chang]

On Tue, Apr 17, 2018 at 06:37:06PM +0200, Daniel Kiper wrote:
> On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> > And this bug report seems relevant ..
> >
> > https://savannah.gnu.org/bugs/?49937
> >
> > On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > > The command line has been processed by grub shell, then the result is expected
> > > to be passed to kernel command line as verbatim according to the grub manual
> > > [1][2].
> > >
> > > This patch removes extra escape character added as it helps nothing but only
> > > creates trouble as you want them to be literal. Besides the surrounding
> > > double-quotes added is kept as it used to protect space.
> 
> CC-ing Vladmir.
> 
> Hmmm... Have you tested this patch on all platforms supported by GRUB2?

AFAICS, it affects boot protocols of which grub supports in commands like
linux, multiboot, xen_hypervisor and so on. Non of them will intepret escape
sequence and are pretty straightforward in composition -- In general, use space
to delimit options and quote the options by double quotes once they contain any
space.

> I understand that current behavior is not accepted on some but I have
> a feeling that somebody make it work in that way due to some reason.
> Sadly I cannot find anything about that in git log. So, I have to think
> how to cope with that. Or...

As a side note, we had a down stream bug report that udev generates symlink
names containing '\' character, and with the current grub command processing
it's never possible to use those symlinks as kernel command line option, as the
character is either stripped or doubled.

Thanks,
Michael

> 
> Vladmir, do you know why command line is processed in that way?
> 
> Daniel

Re: [PATCH] pass kernel command line as verbatim

[Daniel Kiper]

On Wed, Apr 18, 2018 at 03:07:15PM +0800, Michael Chang wrote:
> On Tue, Apr 17, 2018 at 06:37:06PM +0200, Daniel Kiper wrote:
> > On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> > > And this bug report seems relevant ..
> > >
> > > https://savannah.gnu.org/bugs/?49937
> > >
> > > On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > > > The command line has been processed by grub shell, then the result is expected
> > > > to be passed to kernel command line as verbatim according to the grub manual
> > > > [1][2].
> > > >
> > > > This patch removes extra escape character added as it helps nothing but only
> > > > creates trouble as you want them to be literal. Besides the surrounding
> > > > double-quotes added is kept as it used to protect space.
> >
> > CC-ing Vladmir.
> >
> > Hmmm... Have you tested this patch on all platforms supported by GRUB2?
>
> AFAICS, it affects boot protocols of which grub supports in commands like
> linux, multiboot, xen_hypervisor and so on. Non of them will intepret escape
> sequence and are pretty straightforward in composition -- In general, use space
> to delimit options and quote the options by double quotes once they contain any
> space.
>
> > I understand that current behavior is not accepted on some but I have
> > a feeling that somebody make it work in that way due to some reason.
> > Sadly I cannot find anything about that in git log. So, I have to think
> > how to cope with that. Or...
>
> As a side note, we had a down stream bug report that udev generates symlink
> names containing '\' character, and with the current grub command processing
> it's never possible to use those symlinks as kernel command line option, as the
> character is either stripped or doubled.

I understand your problem but I am afraid about compatibility. I would
agree if you change the behavior here and introduce a shell variable to
disable that change. If nobody will complain for some time then we can
drop that variable a leave new desired behavior. Does it make sense?

And I have a question about space (what about tab?): why it is quoted
if, AIUI, kernel command line should not be processed at this point?
Should not it be created earlier properly? I mean with required quotes.

Hmmm... What about modules command lines?

Daniel

Re: [PATCH] pass kernel command line as verbatim

[Michael Chang]

On Tue, Apr 24, 2018 at 12:03:21PM +0200, Daniel Kiper wrote:
> On Wed, Apr 18, 2018 at 03:07:15PM +0800, Michael Chang wrote:
> > On Tue, Apr 17, 2018 at 06:37:06PM +0200, Daniel Kiper wrote:
> > > On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> > > > And this bug report seems relevant ..
> > > >
> > > > https://savannah.gnu.org/bugs/?49937
> > > >
> > > > On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > > > > The command line has been processed by grub shell, then the result is expected
> > > > > to be passed to kernel command line as verbatim according to the grub manual
> > > > > [1][2].
> > > > >
> > > > > This patch removes extra escape character added as it helps nothing but only
> > > > > creates trouble as you want them to be literal. Besides the surrounding
> > > > > double-quotes added is kept as it used to protect space.
> > >
> > > CC-ing Vladmir.
> > >
> > > Hmmm... Have you tested this patch on all platforms supported by GRUB2?
> >
> > AFAICS, it affects boot protocols of which grub supports in commands like
> > linux, multiboot, xen_hypervisor and so on. Non of them will intepret escape
> > sequence and are pretty straightforward in composition -- In general, use space
> > to delimit options and quote the options by double quotes once they contain any
> > space.
> >
> > > I understand that current behavior is not accepted on some but I have
> > > a feeling that somebody make it work in that way due to some reason.
> > > Sadly I cannot find anything about that in git log. So, I have to think
> > > how to cope with that. Or...
> >
> > As a side note, we had a down stream bug report that udev generates symlink
> > names containing '\' character, and with the current grub command processing
> > it's never possible to use those symlinks as kernel command line option, as the
> > character is either stripped or doubled.
> 
> I understand your problem but I am afraid about compatibility. I would
> agree if you change the behavior here and introduce a shell variable to
> disable that change. If nobody will complain for some time then we can
> drop that variable a leave new desired behavior. Does it make sense?

Yes, it makes sense. But here I have another idea that can be helpful if we
want to have one grub.cfg working on all versions. With the shell variable is
not possible as the result is either breaking in one way or not.

It borrows the same idea of using feature variable to determine what current
build can support at run time, and thus allow to use another set of settings
like GRUB_CMDLINE_LINUX_V2 to override cmdline for the new sytax. It is fine to
not specify GRUB_CMDLINE_LINUX_V2, in that we will use GRUB_CMDLINE_LINUX.

> 
> And I have a question about space (what about tab?): why it is quoted
> if, AIUI, kernel command line should not be processed at this point?
> Should not it be created earlier properly? I mean with required quotes.

The command line arguments will be processed by grub shell before passing them
to kernel command line. They will evaluate quotes (among other things) and
extract string from them. As a helper, the result can be observed by "echo"
command.

  grub> echo abc="foo bar"
  abc=foo bar

As you can see, we have to add the quotes back here then it can be passed to
kernel command line as one option.

  "abc=foo bar"

There could have other cases needs additional processing, but as a rule of
thumb I think we should avoid them as much as possible unless strickly
necessary. We need a "preditable" result that stays the same by means of echo
command's output.

For the question of tab, I did not investigate. I think it will be the same
with other characters unless it possessed special meaning to grub's shell
syntax...

> 
> Hmmm... What about modules command lines?

I think it in common with others, Why is it special ?

Thanks,
Michael

> 
> Daniel

Re: [PATCH] pass kernel command line as verbatim

[Daniel Kiper]

On Thu, Apr 26, 2018 at 04:42:30PM +0800, Michael Chang wrote:
> On Tue, Apr 24, 2018 at 12:03:21PM +0200, Daniel Kiper wrote:
> > On Wed, Apr 18, 2018 at 03:07:15PM +0800, Michael Chang wrote:
> > > On Tue, Apr 17, 2018 at 06:37:06PM +0200, Daniel Kiper wrote:
> > > > On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> > > > > And this bug report seems relevant ..
> > > > >
> > > > > https://savannah.gnu.org/bugs/?49937
> > > > >
> > > > > On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > > > > > The command line has been processed by grub shell, then the result is expected
> > > > > > to be passed to kernel command line as verbatim according to the grub manual
> > > > > > [1][2].
> > > > > >
> > > > > > This patch removes extra escape character added as it helps nothing but only
> > > > > > creates trouble as you want them to be literal. Besides the surrounding
> > > > > > double-quotes added is kept as it used to protect space.
> > > >
> > > > CC-ing Vladmir.
> > > >
> > > > Hmmm... Have you tested this patch on all platforms supported by GRUB2?
> > >
> > > AFAICS, it affects boot protocols of which grub supports in commands like
> > > linux, multiboot, xen_hypervisor and so on. Non of them will intepret escape
> > > sequence and are pretty straightforward in composition -- In general, use space
> > > to delimit options and quote the options by double quotes once they contain any
> > > space.
> > >
> > > > I understand that current behavior is not accepted on some but I have
> > > > a feeling that somebody make it work in that way due to some reason.
> > > > Sadly I cannot find anything about that in git log. So, I have to think
> > > > how to cope with that. Or...
> > >
> > > As a side note, we had a down stream bug report that udev generates symlink
> > > names containing '\' character, and with the current grub command processing
> > > it's never possible to use those symlinks as kernel command line option, as the
> > > character is either stripped or doubled.
> >
> > I understand your problem but I am afraid about compatibility. I would
> > agree if you change the behavior here and introduce a shell variable to
> > disable that change. If nobody will complain for some time then we can
> > drop that variable a leave new desired behavior. Does it make sense?
>
> Yes, it makes sense. But here I have another idea that can be helpful if we
> want to have one grub.cfg working on all versions. With the shell variable is
> not possible as the result is either breaking in one way or not.
>
> It borrows the same idea of using feature variable to determine what current
> build can support at run time, and thus allow to use another set of settings
> like GRUB_CMDLINE_LINUX_V2 to override cmdline for the new sytax. It is fine to
> not specify GRUB_CMDLINE_LINUX_V2, in that we will use GRUB_CMDLINE_LINUX.

I prefer something which is common for all boot protocols instead of
something specific for a given one, in this case Linux one.

> > And I have a question about space (what about tab?): why it is quoted
> > if, AIUI, kernel command line should not be processed at this point?
> > Should not it be created earlier properly? I mean with required quotes.
>
> The command line arguments will be processed by grub shell before passing them
> to kernel command line. They will evaluate quotes (among other things) and
> extract string from them. As a helper, the result can be observed by "echo"
> command.
>
>   grub> echo abc="foo bar"
>   abc=foo bar

Sure thing. So, I think that you have to escape all special characters
for GRUB shell before putting a given command line into grub.cfg.
I understand that it can be difficult in current shell based config
generator but you should try...

> As you can see, we have to add the quotes back here then it can be passed to
> kernel command line as one option.
>
>   "abc=foo bar"

"abc=foo bar" != abc="foo bar". This behavior can potentially create
issues on systems which use measurement/signing to protect the configs,
etc. So, I think that command lines should be properly escaped before
putting them into grub.cfg and GRUB should not be allowed to do anything
with them including quoting.

> There could have other cases needs additional processing, but as a rule of
> thumb I think we should avoid them as much as possible unless strickly
> necessary. We need a "preditable" result that stays the same by means of echo
> command's output.

Exactly!

> For the question of tab, I did not investigate. I think it will be the same
> with other characters unless it possessed special meaning to grub's shell
> syntax...

Probably requires the same escaping/quoting as space. Of course before
putting it into grub.cfg. Please look above for more details.

> > Hmmm... What about modules command lines?
>
> I think it in common with others, Why is it special ?

I am asking because some boot protocols, e.g. Multiboot family, allows
passing some arguments to modules too. So, these arguments should be
treated in the same way as the kernel arguments are treated.

Daniel

Re: [PATCH] pass kernel command line as verbatim

[Michael Chang]

On Wed, May 09, 2018 at 03:23:56PM +0200, Daniel Kiper wrote:
> On Thu, Apr 26, 2018 at 04:42:30PM +0800, Michael Chang wrote:
> > On Tue, Apr 24, 2018 at 12:03:21PM +0200, Daniel Kiper wrote:
> > > On Wed, Apr 18, 2018 at 03:07:15PM +0800, Michael Chang wrote:
> > > > On Tue, Apr 17, 2018 at 06:37:06PM +0200, Daniel Kiper wrote:
> > > > > On Wed, Apr 11, 2018 at 05:17:03PM +0800, Michael Chang wrote:
> > > > > > And this bug report seems relevant ..
> > > > > >
> > > > > > https://savannah.gnu.org/bugs/?49937
> > > > > >
> > > > > > On Wed, Apr 11, 2018 at 04:58:54PM +0800, Michael Chang wrote:
> > > > > > > The command line has been processed by grub shell, then the result is expected
> > > > > > > to be passed to kernel command line as verbatim according to the grub manual
> > > > > > > [1][2].
> > > > > > >
> > > > > > > This patch removes extra escape character added as it helps nothing but only
> > > > > > > creates trouble as you want them to be literal. Besides the surrounding
> > > > > > > double-quotes added is kept as it used to protect space.
> > > > >
> > > > > CC-ing Vladmir.
> > > > >
> > > > > Hmmm... Have you tested this patch on all platforms supported by GRUB2?
> > > >
> > > > AFAICS, it affects boot protocols of which grub supports in commands like
> > > > linux, multiboot, xen_hypervisor and so on. Non of them will intepret escape
> > > > sequence and are pretty straightforward in composition -- In general, use space
> > > > to delimit options and quote the options by double quotes once they contain any
> > > > space.
> > > >
> > > > > I understand that current behavior is not accepted on some but I have
> > > > > a feeling that somebody make it work in that way due to some reason.
> > > > > Sadly I cannot find anything about that in git log. So, I have to think
> > > > > how to cope with that. Or...
> > > >
> > > > As a side note, we had a down stream bug report that udev generates symlink
> > > > names containing '\' character, and with the current grub command processing
> > > > it's never possible to use those symlinks as kernel command line option, as the
> > > > character is either stripped or doubled.
> > >
> > > I understand your problem but I am afraid about compatibility. I would
> > > agree if you change the behavior here and introduce a shell variable to
> > > disable that change. If nobody will complain for some time then we can
> > > drop that variable a leave new desired behavior. Does it make sense?
> >
> > Yes, it makes sense. But here I have another idea that can be helpful if we
> > want to have one grub.cfg working on all versions. With the shell variable is
> > not possible as the result is either breaking in one way or not.
> >
> > It borrows the same idea of using feature variable to determine what current
> > build can support at run time, and thus allow to use another set of settings
> > like GRUB_CMDLINE_LINUX_V2 to override cmdline for the new sytax. It is fine to
> > not specify GRUB_CMDLINE_LINUX_V2, in that we will use GRUB_CMDLINE_LINUX.
> 
> I prefer something which is common for all boot protocols instead of
> something specific for a given one, in this case Linux one.

I think the same idea can extend to other protocol family's GRUB_CMDLINE_*
simple config settings, say GRUB_CMDLINE_XEN, GRUB_CMDLINE_NETBSD and so on,
here Linux is used as an example.

We probably need to consider whereas the grub.cfg could be sourced with
different versions of grub by which the flag is not intepreted.

> 
> > > And I have a question about space (what about tab?): why it is quoted
> > > if, AIUI, kernel command line should not be processed at this point?
> > > Should not it be created earlier properly? I mean with required quotes.
> >
> > The command line arguments will be processed by grub shell before passing them
> > to kernel command line. They will evaluate quotes (among other things) and
> > extract string from them. As a helper, the result can be observed by "echo"
> > command.
> >
> >   grub> echo abc="foo bar"
> >   abc=foo bar
> 
> Sure thing. So, I think that you have to escape all special characters
> for GRUB shell before putting a given command line into grub.cfg.
> I understand that it can be difficult in current shell based config
> generator but you should try...

Yes, we can accept it this way because we know what's happening. But for most
other people don't expect this to be true especially when the document is also
misleading. Also linux(bash) shell processing has added more complexity making
more confuse.

Simpl put, The abc="foo bar" (compared to abc=\"foo bar\") has been in
wildspread use as it's quite common in specifying command lines, changing it
here would likely introduce more impact for what has been deployed and in use
..

> 
> > As you can see, we have to add the quotes back here then it can be passed to
> > kernel command line as one option.
> >
> >   "abc=foo bar"
> 
> "abc=foo bar" != abc="foo bar". This behavior can potentially create
> issues on systems which use measurement/signing to protect the configs,
> etc. So, I think that command lines should be properly escaped before
> putting them into grub.cfg and GRUB should not be allowed to do anything
> with them including quoting.

Good catch. I think it is suffice to overwhelmed me on the attempt trying to
get less people impacted through keeping the behavior of automatic quoting
strings with space. 

> 
> > There could have other cases needs additional processing, but as a rule of
> > thumb I think we should avoid them as much as possible unless strickly
> > necessary. We need a "preditable" result that stays the same by means of echo
> > command's output.
> 
> Exactly!

Please note there's "as much as possible unless strictly necessary " in above
sentense but we may agree to have no exception here. :)

> 
> > For the question of tab, I did not investigate. I think it will be the same
> > with other characters unless it possessed special meaning to grub's shell
> > syntax...
> 
> Probably requires the same escaping/quoting as space. Of course before
> putting it into grub.cfg. Please look above for more details.

Yes.

> 
> > > Hmmm... What about modules command lines?
> >
> > I think it in common with others, Why is it special ?
> 
> I am asking because some boot protocols, e.g. Multiboot family, allows
> passing some arguments to modules too. So, these arguments should be
> treated in the same way as the kernel arguments are treated.

Yes, use Xen as an example, the module(xen kernel) shares the same
GRUB_CMDLINE_LINUX with linux kernel unless you're overriding them with
GRUB_CMDLINE_LINUX_XEN_REPLACE. There could have been other modules doing it
differently but I am not aware of ...

Thanks,
Michael

> 
> Daniel