[PATCH] cmdline: pass kernel command line as verbatim

[PATCH] cmdline: 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 fixes excessive escape character (ie backslash) added to the output
of command line and the problem is making it impossible to use literal ', " and
\ as the value of parameter

In addition, specifying the value of parameter with double quotes to protect
space, like var="v1 v1", is corrected in this patch to result in identical
output. Previously it was outputting as "var=v1 v2", although the kernel seemed
to process it just fine, but semantically it could be interpreted as string
literal rather than the intended key and value pair. Given that kernel is not
the only consumer of the command line options, we'd better eliminate the
ambiguity as well.

The change is summarized in the table.

         | grub.cfg          | /proc/cmdline
-----------------------------------------------
         | var=\"v1 v2\"     | var=\"v1 v2\"
         | var=\'v1 v2\'     | var=\'v1 v2\'
         | var=v1\'v2        | var=v1\'v2
         | var=v1\"v2        | var=v1\"v2
 before  | var=v1\\v2        | var=v1\\v2
         | var="v1 v2"       | "var=v1 v2"
         | var="\\'v1 v2"    | "var=\\\'v1 v2"
         | var="\\\"v1 v2"   | "var=\\\"v1 v2"
-----------------------------------------------
         | var=\"v1 v2\"     | var="v1 v2"
         | var=\'v1 v2\'     | var='v1 v2'
         | var=v1\'v2        | var=v1'v2
         | var=v1\"v2        | var=v1"v2
 after   | var=v1\\v2        | var=v1\v2
         | var="v1 v2"       | var="v1 v2"
         | var="\\'v1 v2"    | var="\\'v1 v2"
         | var="\\\"v1 v2"   | var="\\\"v1 v2"

As long as the change could be disruptive to existing grub.cfg with above
syntax in use, we intentionally introduce it as an config option so that user
can choose to opt in. The GRUB_ENABLE_CMDLINE_VERBATIM=yes can be set in
/etc/default/grub for that matter.

[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/Makefile.core.def         |   5 ++
 grub-core/lib/cmdline.c             |  10 +++
 grub-core/lib/cmdline_verbatim.c    | 166 ++++++++++++++++++++++++++++++++++++
 grub-core/normal/main.c             |   2 +-
 include/grub/lib/cmdline_override.h |  34 ++++++++
 util/grub-mkconfig.in               |   3 +-
 util/grub.d/00_header.in            |  11 +++
 7 files changed, 229 insertions(+), 2 deletions(-)
 create mode 100644 grub-core/lib/cmdline_verbatim.c
 create mode 100644 include/grub/lib/cmdline_override.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 48b82e763..a30c3f6c6 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2513,6 +2513,11 @@ module = {
   common = lib/progress.c;
 };
 
+module = {
+  name = cmdline_verbatim;
+  common = lib/cmdline_verbatim.c;
+};
+
 module = {
   name = file;
   common = commands/file.c;
diff --git a/grub-core/lib/cmdline.c b/grub-core/lib/cmdline.c
index ed0b149dc..46728d784 100644
--- a/grub-core/lib/cmdline.c
+++ b/grub-core/lib/cmdline.c
@@ -18,8 +18,12 @@
  */
 
 #include <grub/lib/cmdline.h>
+#include <grub/lib/cmdline_override.h>
 #include <grub/misc.h>
 
+grub_create_loader_cmdline_t grub_create_loader_cmdline_override;
+grub_loader_cmdline_size_t grub_loader_cmdline_size_override;
+
 static unsigned int check_arg (char *c, int *has_space)
 {
   int space = 0;
@@ -50,6 +54,9 @@ unsigned int grub_loader_cmdline_size (int argc, char *argv[])
   int i;
   unsigned int size = 0;
 
+  if (grub_loader_cmdline_size_override)
+    return grub_loader_cmdline_size_override (argc, argv);
+
   for (i = 0; i < argc; i++)
     {
       size += check_arg (argv[i], 0);
@@ -70,6 +77,9 @@ grub_create_loader_cmdline (int argc, char *argv[], char *buf,
   unsigned int arg_size;
   char *c, *orig_buf = buf;
 
+  if (grub_create_loader_cmdline_override)
+    return grub_create_loader_cmdline_override (argc, argv, buf, size, type);
+
   for (i = 0; i < argc; i++)
     {
       c = argv[i];
diff --git a/grub-core/lib/cmdline_verbatim.c b/grub-core/lib/cmdline_verbatim.c
new file mode 100644
index 000000000..cb8de09aa
--- /dev/null
+++ b/grub-core/lib/cmdline_verbatim.c
@@ -0,0 +1,166 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006,2007,2010  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/dl.h>
+#include <grub/command.h>
+#include <grub/misc.h>
+#include <grub/i18n.h>
+#include <grub/lib/cmdline_override.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static unsigned int check_arg (char *c, int *has_space)
+{
+  int space = 0;
+  unsigned int size = 0;
+
+  if (grub_strchr (c, ' '))
+    space = 1;
+
+  while (*c)
+    {
+      if (space && (*c == '\\' || *c == '"'))
+	size++;
+
+      size++;
+      c++;
+    }
+
+  if (space)
+    size += 2;
+
+  if (has_space)
+    *has_space = space;
+
+  return size;
+}
+
+static unsigned int
+grub_loader_cmdline_size_verbatim (int argc, char *argv[])
+{
+  int i;
+  unsigned int size = 0;
+
+  for (i = 0; i < argc; i++)
+    {
+      size += check_arg (argv[i], 0);
+      size++; /* Separator space or NULL.  */
+    }
+
+  if (size == 0)
+    size = 1;
+
+  return size;
+}
+
+static grub_err_t
+grub_create_loader_cmdline_verbatim (int argc, char *argv[], char *buf,
+			    grub_size_t size, enum grub_verify_string_type type)
+{
+  int i;
+  char *orig_buf = buf;
+
+  for (i = 0; i < argc; i++)
+    {
+      char *key, *val, *c;
+      int key_space, space;
+      unsigned int arg_size;
+
+      key = grub_strdup (argv[i]);
+      val = grub_strchr (key, '=');
+
+      if (val)
+	*val++ = '\0';
+
+      arg_size = check_arg(key, &key_space);
+
+      if (val)
+	{
+	  arg_size += check_arg(val, &space);
+	  arg_size++; /* Equal sign space.  */
+	}
+
+      arg_size++; /* Separator space or NULL.  */
+
+      if (size < arg_size)
+	break;
+
+      size -= arg_size;
+
+      if (key_space)
+	*buf++ = '"';
+
+      c = key;
+
+      while (*c)
+	{
+	  if (key_space && (*c == '\\' || *c == '"'))
+	    *buf++ = '\\';
+
+	  *buf++ = *c;
+	  c++;
+	}
+
+      if (key_space)
+	*buf++ = '"';
+
+      if (val)
+	{
+	  *buf++ = '=';
+	  c = val;
+
+	  if (space)
+	    *buf++ = '"';
+
+	  while (*c)
+	    {
+	      if (space && (*c == '\\' || *c == '"'))
+		  *buf++ = '\\';
+
+	      *buf++ = *c;
+	      c++;
+	    }
+
+	  if (space)
+	    *buf++ = '"';
+	}
+
+      *buf++ = ' ';
+      grub_free (key);
+    }
+
+  /* Replace last space with null.  */
+  if (i)
+    buf--;
+
+  *buf = 0;
+
+  return grub_verify_string (orig_buf, type);
+}
+
+GRUB_MOD_INIT(cmdline_verbatim)
+{
+  grub_create_loader_cmdline_override = grub_create_loader_cmdline_verbatim;
+  grub_loader_cmdline_size_override = grub_loader_cmdline_size_verbatim;
+}
+
+GRUB_MOD_FINI(cmdline_verbatim)
+{
+  grub_create_loader_cmdline_override = NULL;
+  grub_loader_cmdline_size_override = NULL;
+}
diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
index c4ebe9e22..b7fefcf78 100644
--- a/grub-core/normal/main.c
+++ b/grub-core/normal/main.c
@@ -506,7 +506,7 @@ static const char *features[] = {
   "feature_chainloader_bpb", "feature_ntldr", "feature_platform_search_hint",
   "feature_default_font_path", "feature_all_video_module",
   "feature_menuentry_id", "feature_menuentry_options", "feature_200_final",
-  "feature_nativedisk_cmd", "feature_timeout_style"
+  "feature_nativedisk_cmd", "feature_timeout_style", "feature_cmdline_override"
 };
 
 GRUB_MOD_INIT(normal)
diff --git a/include/grub/lib/cmdline_override.h b/include/grub/lib/cmdline_override.h
new file mode 100644
index 000000000..71ff45221
--- /dev/null
+++ b/include/grub/lib/cmdline_override.h
@@ -0,0 +1,34 @@
+/* cmdline_override.h - linux command line handling */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2010  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_CMDLINE_OVERRIDE_HEADER
+#define GRUB_CMDLINE_OVERRIDE_HEADER	1
+
+#include <grub/types.h>
+#include <grub/verify.h>
+
+typedef unsigned int (*grub_loader_cmdline_size_t) (int argc, char *argv[]);
+extern grub_loader_cmdline_size_t EXPORT_VAR(grub_loader_cmdline_size_override);
+
+typedef grub_err_t (*grub_create_loader_cmdline_t) (int argc, char *argv[], char *buf,
+				       grub_size_t size, enum grub_verify_string_type type);
+
+extern grub_create_loader_cmdline_t EXPORT_VAR(grub_create_loader_cmdline_override);
+
+#endif /* ! GRUB_CMDLINE_OVERRIDE_HEADER */
diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
index d3e879b8e..7aa8327d8 100644
--- a/util/grub-mkconfig.in
+++ b/util/grub-mkconfig.in
@@ -248,7 +248,8 @@ export GRUB_DEFAULT \
   GRUB_ENABLE_CRYPTODISK \
   GRUB_BADRAM \
   GRUB_OS_PROBER_SKIP_LIST \
-  GRUB_DISABLE_SUBMENU
+  GRUB_DISABLE_SUBMENU \
+  GRUB_ENABLE_CMDLINE_VERBATIM
 
 if test "x${grub_cfg}" != "x"; then
   rm -f "${grub_cfg}.new"
diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
index 93a90233e..abd56ccd2 100644
--- a/util/grub.d/00_header.in
+++ b/util/grub.d/00_header.in
@@ -354,3 +354,14 @@ fi
 if [ "x${GRUB_BADRAM}" != "x" ] ; then
   echo "badram ${GRUB_BADRAM}"
 fi
+
+if [ "x$GRUB_ENABLE_CMDLINE_VERBATIM" = "xyes" ]; then
+    cat << EOF
+if [ x\$feature_cmdline_override = xy ] ; then
+  insmod cmdline_verbatim
+else
+  echo "Your grub is too old to support verbatim command line."
+  echo "Please use version >= 2.06."
+fi
+EOF
+fi
-- 
2.16.4

Re: [PATCH] cmdline: pass kernel command line as verbatim

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

Am Sat, 4 Apr 2020 13:31:40 +0800
schrieb Michael Chang <mchang@suse.com>:

>  7 files changed, 229 insertions(+), 2 deletions(-)

A lot of churn. It is called 'verbatim', yet it inserts extra characters in the output buffer.

I'm still stunned why there is an argument about the obvious bugfix.

Good luck.

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] cmdline: pass kernel command line as verbatim

[Michael Chang]

On Mon, Apr 06, 2020 at 09:18:26AM +0200, Olaf Hering wrote:
> Am Sat, 4 Apr 2020 13:31:40 +0800
> schrieb Michael Chang <mchang@suse.com>:
> 
> >  7 files changed, 229 insertions(+), 2 deletions(-)
> 
> A lot of churn. It is called 'verbatim', yet it inserts extra characters in the output buffer.

What verbatim means when people read that from the manual is really not
up to me. Here my attempt was merely to fix problem as you did but
mostly reflecting my own point of view. Afterall the table provided in
the description can easily be used as reason to turn down the patch if
it really not making sense on the premise that we have agreed on a real
fix would be looking like, be it yours, mine or anyone else's proposal.

> I'm still stunned why there is an argument about the obvious bugfix.

It is absolutely bugfix, however any prospective impact on grub.cfg that
a bugfix would introduce is always a thing for upstream to adopt it.

Thanks,
Michael
 
> Good luck.
> 
> Olaf

Re: [PATCH] cmdline: pass kernel command line as verbatim

[Daniel Kiper]

On Mon, Apr 06, 2020 at 04:55:52PM +0800, Michael Chang wrote:
> On Mon, Apr 06, 2020 at 09:18:26AM +0200, Olaf Hering wrote:
> > Am Sat, 4 Apr 2020 13:31:40 +0800
> > schrieb Michael Chang <mchang@suse.com>:
> >
> > >  7 files changed, 229 insertions(+), 2 deletions(-)
> >
> > A lot of churn. It is called 'verbatim', yet it inserts extra characters in the output buffer.
>
> What verbatim means when people read that from the manual is really not
> up to me. Here my attempt was merely to fix problem as you did but
> mostly reflecting my own point of view. Afterall the table provided in
> the description can easily be used as reason to turn down the patch if
> it really not making sense on the premise that we have agreed on a real
> fix would be looking like, be it yours, mine or anyone else's proposal.
>
> > I'm still stunned why there is an argument about the obvious bugfix.
>
> It is absolutely bugfix, however any prospective impact on grub.cfg that
> a bugfix would introduce is always a thing for upstream to adopt it.

Michael, thank you for preparing the patch.

Olaf, Michael, I chatted with Vladimir and he told me that the commit
introducing the "issue" was deliberate change. I asked him to explain
here what problem it fixes and how to fix the breakage introduced by the
fix... Yeah... :-) Sadly as he told me the issue is complicated and the
real fix is not easy. Anyway, I hope that he will be able to chime in
soon and shed more light at this long standing problem. So, finally we
will be able to provide the solution...

Daniel