Re: [PATCH v1] fix kernel cmdline corruption

[Michael Chang]

Hi Olaf,

The patch rang a bell to me and eventually I figured out that I had
similar patch posted.

https://lists.gnu.org/archive/html/grub-devel/2018-04/msg00038.html

In short, the issue is not only in the fix itself, but also to take care
and provide a measure to avoid incompatible grub.cfg before and after
the patch.

To recap, there were two options provided to deal with the compatibilty
issue.

1. Introduce a shell variable to disable the change, whenever anything
goes wrong.

2. Use readonly feature variable similar to $feature_menuentry_id that
can be used to detect capability and apply settings accordingly, thus
still fulfill the requirement of one grub.cfg can work for all versions
IMHO.

I am in favour of #2, but still they didn't seem to have upstream's
consent yet. 

Thanks,
Michael

On Wed, Mar 18, 2020 at 12:13:12PM +0100, Olaf Hering wrote:
> The functions grub_create_loader_cmdline and its sibling
> grub_loader_cmdline_size add extra, unexpected characters to the kernel
> cmdline. Both functions are supposed to copy each argv[] element
> verbatim. It is up to the caller of the kernel command ('linux' for
> example) to add desired quoting or escaping characters for the consumers
> of the kernel command line. The built-in grub shell gives enough
> opportunities to construct such argv[] elements.
> 
> After this change, these arguments result in the following kernel cmdline:
> 
>   var='val' var="val" var=\'v1 v2\' var=\"v1 v2\" var=\\\"v1 v2\\\" x
>   var=val var=val var='v1 v2' var="v1 v2" var=\"v1 v2\" x
> 
> Fixes commit 25953e10553dad2e378541a68686fc094603ec54
> 
> Signed-off-by: Olaf Hering <address@hidden>
> ---
>  grub-core/lib/cmdline.c | 58 
> ++++++++-----------------------------------------
>  1 file changed, 9 insertions(+), 49 deletions(-)
> 
> diff --git a/grub-core/lib/cmdline.c b/grub-core/lib/cmdline.c
> index ed0b149dc..b572a367f 100644
> --- a/grub-core/lib/cmdline.c
> +++ b/grub-core/lib/cmdline.c
> @@ -20,31 +20,6 @@
>  #include <grub/lib/cmdline.h>
>  #include <grub/misc.h>
>  
> -static unsigned int check_arg (char *c, int *has_space)
> -{
> -  int space = 0;
> -  unsigned int size = 0;
> -
> -  while (*c)
> -    {
> -      if (*c == '\\' || *c == '\'' || *c == '"')
> -     size++;
> -      else if (*c == ' ')
> -     space = 1;
> -
> -      size++;
> -      c++;
> -    }
> -
> -  if (space)
> -    size += 2;
> -
> -  if (has_space)
> -    *has_space = space;
> -
> -  return size;
> -}
> -
>  unsigned int grub_loader_cmdline_size (int argc, char *argv[])
>  {
>    int i;
> @@ -52,7 +27,7 @@ unsigned int grub_loader_cmdline_size (int argc, char 
> *argv[])
>  
>    for (i = 0; i < argc; i++)
>      {
> -      size += check_arg (argv[i], 0);
> +      size += grub_strlen(argv[i]);
>        size++; /* Separator space or NULL.  */
>      }
>  
> @@ -66,36 +41,21 @@ grub_err_t
>  grub_create_loader_cmdline (int argc, char *argv[], char *buf,
>                           grub_size_t size, enum grub_verify_string_type type)
>  {
> -  int i, space;
> +  int i;
>    unsigned int arg_size;
> -  char *c, *orig_buf = buf;
> +  char *orig_buf = buf;
>  
>    for (i = 0; i < argc; i++)
>      {
> -      c = argv[i];
> -      arg_size = check_arg(argv[i], &space);
> -      arg_size++; /* Separator space or NULL.  */
> +      arg_size = grub_strlen(argv[i]);
>  
> -      if (size < arg_size)
> +      /* Separator space or NULL.  */
> +      if (size < arg_size + 1)
>       break;
>  
> -      size -= arg_size;
> -
> -      if (space)
> -     *buf++ = '"';
> -
> -      while (*c)
> -     {
> -       if (*c == '\\' || *c == '\'' || *c == '"')
> -         *buf++ = '\\';
> -
> -       *buf++ = *c;
> -       c++;
> -     }
> -
> -      if (space)
> -     *buf++ = '"';
> -
> +      grub_memcpy(buf, argv[i], arg_size);
> +      size -= arg_size + 1;
> +      buf += arg_size;
>        *buf++ = ' ';
>      }
>  
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel