Re: [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline

[Baoquan He <bhe@redhat.com>]

On 04/28/17 at 12:37pm, Kees Cook wrote:
> On Wed, Apr 26, 2017 at 3:16 AM, Baoquan He <bhe@redhat.com> wrote:
> > In commit:
> >
> >   f28442497b5c ("x86/boot: Fix KASLR and memmap= collision")
> >
> > ... the memmap= option is parsed so that KASLR can avoid those reserved
> > regions. It uses cmdline_find_option() to get the value if memmap=
> > is specified, however the problem is that cmdline_find_option() can only
> > find the last entry if multiple memmap entries are provided. This
> > is not correct.
> >
> > In this patch, the whole cmdline will be scanned to search each
> > memmap, all of them will be parsed and handled.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: x86@kernel.org
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Yinghai Lu <yinghai@kernel.org>
> > Cc: Borislav Petkov <bp@suse.de>
> > ---
> >  arch/x86/boot/compressed/cmdline.c |   2 +-
> >  arch/x86/boot/compressed/kaslr.c   | 125 +++++++++++++++++++++----------------
> >  arch/x86/boot/string.c             |   8 +++
> >  3 files changed, 80 insertions(+), 55 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
> > index 73ccf63..9dc1ce6 100644
> > --- a/arch/x86/boot/compressed/cmdline.c
> > +++ b/arch/x86/boot/compressed/cmdline.c
> > @@ -13,7 +13,7 @@ static inline char rdfs8(addr_t addr)
> >         return *((char *)(fs + addr));
> >  }
> >  #include "../cmdline.c"
> > -static unsigned long get_cmd_line_ptr(void)
> > +unsigned long get_cmd_line_ptr(void)
> >  {
> >         unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > index 6d9a546..53a06ec 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -9,14 +9,28 @@
> >   * contain the entire properly aligned running kernel image.
> >   *
> >   */
> > +
> > +/*
> > + * linux/ctype.h is expected, while conflicts with boot/ctype.h
> > + * which is useless here. Do not include it.
> > + */
> > +#define BOOT_CTYPE_H
> 
> Which include includes this?

Thanks for your reviewing, Kees!

misc.h includes it.

Function isdigit is implemented in boot/ctype.h. And with <linux/ctype.h>,
they will conflict. While next_args() need isspace() in linux/ctype.h to
filter out "space/lf/tab".

> 
> > +
> > +/*
> > + * Exporting symbol in boot stage is meaningless, and will trigger
> > + * compiling error in some cases. Disable it here.
> > + */
> > +#define _LINUX_EXPORT_H
> > +#define EXPORT_SYMBOL(sym)
> 
> And this?

If I want isspace() in linux/ctype.h, _ctype[] in lib/ctype.c is needed.
Meanwhile since next_args() is moved to lib/cmdline.c, including
lib/cmdline.c will bring EXPORT_SYMBOL. And including lib/cmdline.c can
provide a ready-made memparse() which makes us remove the redundent one
in kaslr.c.

> 
> > +
> >  #include "misc.h"
> >  #include "error.h"
> > -#include "../boot.h"
> >
> >  #include <generated/compile.h>
> >  #include <linux/module.h>
> >  #include <linux/uts.h>
> >  #include <linux/utsname.h>
> > +#include <linux/ctype.h>
> 
> Why is misc.h's ctype insufficient?

misc.h's ctype doesn't provide a isspace which next_args() need. There's
a myisspace in boot/cmdline.c, I think it's insufficient, it only check
space.

static inline int myisspace(u8 c)
{
        return c <= ' ';        /* Close enough approximation */
}

> 
> >  #include <generated/utsrelease.h>
> >
> >  /* Simplified build-specific string for starting entropy. */
> > @@ -61,6 +75,8 @@ struct mem_vector {
> >  #define MAX_MEMMAP_REGIONS     4
> >
> >  static bool memmap_too_large;
> > +extern unsigned long get_cmd_line_ptr(void);
> 
> Can we avoid an extern in .c? Seems like this should be defined in
> misc.h or similar?

Yes, it shoud be put in msic.h. I thought putting it here can avoid
polluting the name space. Now I think you are right though now it's only
used by kaslr.c and compressed/cmdline.c where it's defined.

> 
> > +
> >
> >  enum mem_avoid_index {
> >         MEM_AVOID_ZO_RANGE = 0,
> > @@ -85,49 +101,14 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
> >         return true;
> >  }
> >
> > -/**
> > - *     _memparse - Parse a string with mem suffixes into a number
> > - *     @ptr: Where parse begins
> > - *     @retptr: (output) Optional pointer to next char after parse completes
> > - *
> > - *     Parses a string into a number.  The number stored at @ptr is
> > - *     potentially suffixed with K, M, G, T, P, E.
> > - */
> > -static unsigned long long _memparse(const char *ptr, char **retptr)
> > +char *skip_spaces(const char *str)
> >  {
> > -       char *endptr;   /* Local pointer to end of parsed string */
> > -
> > -       unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
> > -
> > -       switch (*endptr) {
> > -       case 'E':
> > -       case 'e':
> > -               ret <<= 10;
> > -       case 'P':
> > -       case 'p':
> > -               ret <<= 10;
> > -       case 'T':
> > -       case 't':
> > -               ret <<= 10;
> > -       case 'G':
> > -       case 'g':
> > -               ret <<= 10;
> > -       case 'M':
> > -       case 'm':
> > -               ret <<= 10;
> > -       case 'K':
> > -       case 'k':
> > -               ret <<= 10;
> > -               endptr++;
> > -       default:
> > -               break;
> > -       }
> > -
> > -       if (retptr)
> > -               *retptr = endptr;
> > -
> > -       return ret;
> > +       while (isspace(*str))
> > +               ++str;
> > +       return (char *)str;
> >  }
> > +#include "../../../../lib/ctype.c"
> > +#include "../../../../lib/cmdline.c"
> >
> >  static int
> >  parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> > @@ -142,7 +123,7 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >                 return -EINVAL;
> >
> >         oldp = p;
> > -       *size = _memparse(p, &p);
> > +       *size = memparse(p, &p);
> >         if (p == oldp)
> >                 return -EINVAL;
> >
> > @@ -155,27 +136,21 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> >         case '#':
> >         case '$':
> >         case '!':
> > -               *start = _memparse(p + 1, &p);
> > +               *start = memparse(p + 1, &p);
> >                 return 0;
> >         }
> >
> >         return -EINVAL;
> >  }
> >
> > -static void mem_avoid_memmap(void)
> > +static void mem_avoid_memmap(char *str)
> >  {
> > -       char arg[128];
> > +       static int i;
> >         int rc;
> > -       int i;
> > -       char *str;
> >
> > -       /* See if we have any memmap areas */
> > -       rc = cmdline_find_option("memmap", arg, sizeof(arg));
> > -       if (rc <= 0)
> > +       if (i >= MAX_MEMMAP_REGIONS)
> >                 return;
> >
> > -       i = 0;
> > -       str = arg;
> >         while (str && (i < MAX_MEMMAP_REGIONS)) {
> >                 int rc;
> >                 unsigned long long start, size;
> > @@ -202,6 +177,48 @@ static void mem_avoid_memmap(void)
> >                 memmap_too_large = true;
> >  }
> >
> > +
> > +/* Macros used by the included decompressor code below. */
> > +#define STATIC
> > +#include <linux/decompress/mm.h>
> 
> Can this be moved to the top of the file? (Also, I wonder if maybe it
> should be moved into misc.h?)

Yes, it can be moved to the top of kaslr.c

In misc.c those lib/decompress_xxx.c include linux/decompress/mm.h
already, and define STATIC as static. But I think it's fine to put it in
misc.h. Let me try.

> 
> > +
> > +#define COMMAND_LINE_SIZE 256
> > +static int handle_mem_memmap(void)
> > +{
> > +       char *args = (char *)get_cmd_line_ptr();
> > +       size_t len = strlen((char *)args);
> > +       char *tmp_cmdline;
> > +       char *param, *val;
> > +
> 
> Here to optimize for the common case, please do something like:
> 
> if (!strstr("memmap=", args))
>     return 0;

Hmm, handle_mem_memmap will be a common handler for memmap= and mem=
options. If here we use strstr to check it, do we need to check "mem="
too?

> 
> > +       tmp_cmdline = malloc(COMMAND_LINE_SIZE);
> 
> Instead of COMMAND_LINE_SIZE hard-limit, please attempt a malloc of
> len + 1. Then you don't have to play games with len adjustments,
> truncation, etc.

Sure, will do.

> 
> > +       if (!tmp_cmdline )
> > +               error("Failed to allocate space for tmp_cmdline");
> > +
> > +       len = (len >= COMMAND_LINE_SIZE) ? COMMAND_LINE_SIZE - 1 : len;
> > +       memcpy(tmp_cmdline, args, len);
> > +       tmp_cmdline[len] = 0;
> > +       args = tmp_cmdline;
> > +
> > +       /* Chew leading spaces */
> > +       args = skip_spaces(args);
> > +
> > +       while (*args) {
> > +               args = next_arg(args, &param, &val);
> > +               /* Stop at -- */
> > +               if (!val && strcmp(param, "--") == 0) {
> > +                       warn("Only '--' specified in cmdline");
> > +                       free(tmp_cmdline);
> > +                       return -1;
> > +               }
> > +
> > +               if (!strcmp(param, "memmap"))
> > +                       mem_avoid_memmap(val);
> > +       }
> > +
> > +       free(tmp_cmdline);
> > +       return 0;
> > +}
> > +
> >  /*
> >   * In theory, KASLR can put the kernel anywhere in the range of [16M, 64T).
> >   * The mem_avoid array is used to store the ranges that need to be avoided
> > @@ -323,7 +340,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >         /* We don't need to set a mapping for setup_data. */
> >
> >         /* Mark the memmap regions we need to avoid */
> > -       mem_avoid_memmap();
> > +       handle_mem_memmap();
> 
> The first "mem" in this name seems redundant.

Hmm, in this patch handle_mem_memmap only handles memmap= option, in
patch 2/3 it will handle mem= too. Do you think I should name it
handle_memmap() here, and rename it to handle_mem_memmap() in patch 2/3?

> 
> >
> >  #ifdef CONFIG_X86_VERBOSE_BOOTUP
> >         /* Make sure video RAM can be used. */
> > diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
> > index 5457b02..630e366 100644
> > --- a/arch/x86/boot/string.c
> > +++ b/arch/x86/boot/string.c
> > @@ -122,6 +122,14 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
> >         return result;
> >  }
> >
> > +long simple_strtol(const char *cp, char **endp, unsigned int base)
> > +{
> > +       if (*cp == '-')
> > +               return -simple_strtoull(cp + 1, endp, base);
> > +
> > +       return simple_strtoull(cp, endp, base);
> > +}
> > +
> >  /**
> >   * strlen - Find the length of a string
> >   * @s: The string to be sized
> > --
> > 2.5.5
> >
> 
> Looks like it's making progress, thanks!
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security