Re: [PATCH v5] x86: fix kaslr and memmap collision

[Kees Cook]

On Thu, Jan 5, 2017 at 6:44 PM, Baoquan He  wrote:
> Add Kees to let him have a look at this too.
>
> On 01/05/17 at 05:21pm, Baoquan He wrote:
>> On 01/04/17 at 11:29am, Dave Jiang wrote:
>> > CONFIG_RANDOMIZE_BASE relocates the kernel to a random base address.
>> > However it does not take into account the memmap= parameter passed in from
>> > the kernel cmdline. This results in the kernel sometimes being put in
>> > the middle of memmap. Teaching kaslr to not insert the kernel in
>> > memmap defined regions. We will support up to 4 memmap regions. Any
>> > additional regions will cause kaslr to disable. The mem_avoid set has
>> > been augmented to add up to 4 unusable regions of memmaps provided by the
>> > user to exclude those regions from the set of valid address range to insert
>> > the uncompressed kernel image. The nn@ss ranges will be skipped by the
>> > mem_avoid set since it indicates memory useable.
>> >
>> > Signed-off-by: Dave Jiang 
>> > ---
>> >
>> > v2:
>> > Addressing comments from Ingo.
>> > - Handle entire list of memmaps
>> > v3:
>> > Fix 32bit build issue
>> > v4:
>> > Addressing comments from Baoquan
>> > - Not exclude nn@ss ranges
>> > v5:
>> > Addressing additional comments from Baoquan
>> > - Update commit header and various coding style changes
>> >
>> > diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
>> > index e5612f3..59c2075 100644
>> > --- a/arch/x86/boot/boot.h
>> > +++ b/arch/x86/boot/boot.h
>> > @@ -332,7 +332,10 @@ int strncmp(const char *cs, const char *ct, size_t 
>> > count);
>> >  size_t strnlen(const char *s, size_t maxlen);
>> >  unsigned int atou(const char *s);
>> >  unsigned long long simple_strtoull(const char *cp, char **endp, unsigned 
>> > int base);
>> > +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int 
>> > base);
>> > +long simple_strtol(const char *cp, char **endp, unsigned int base);
>> >  size_t strlen(const char *s);
>> > +char *strchr(const char *s, int c);
>> >
>> >  /* tty.c */
>> >  void puts(const char *);
>> > diff --git a/arch/x86/boot/compressed/kaslr.c 
>> > b/arch/x86/boot/compressed/kaslr.c
>> > index a66854d..036b514 100644
>> > --- a/arch/x86/boot/compressed/kaslr.c
>> > +++ b/arch/x86/boot/compressed/kaslr.c
>> > @@ -11,6 +11,7 @@
>> >   */
>> >  #include "misc.h"
>> >  #include "error.h"
>> > +#include "../boot.h"
>> >
>> >  #include 
>> >  #include 
>> > @@ -56,11 +57,16 @@ struct mem_vector {
>> > unsigned long size;
>> >  };
>> >
>> > +/* only supporting at most 4 unusable memmap regions with kaslr */
>> > +#define MAX_MEMMAP_REGIONS 4
>> > +
>> >  enum mem_avoid_index {
>> > MEM_AVOID_ZO_RANGE = 0,
>> > MEM_AVOID_INITRD,
>> > MEM_AVOID_CMDLINE,
>> > MEM_AVOID_BOOTPARAMS,
>> > +   MEM_AVOID_MEMMAP_BEGIN,
>> > +   MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
>> > MEM_AVOID_MAX,
>> >  };
>> >
>> > @@ -77,6 +83,121 @@ 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 *endptr;   /* local pointer to end of parsed string */
>> > +
>> > +   unsigned long long ret = simple_strtoull(ptr, , 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;
>> > +}
>> > +
>> > +static int
>> > +parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>> > +{
>> > +   char *oldp;
>> > +
>> > +   if (!p)
>> > +   return -EINVAL;
>> > +
>> > +   /* we don't care about this option here */
>> > +   if (!strncmp(p, "exactmap", 8))
>> > +   return -EINVAL;
>> > +
>> > +   oldp = p;
>> > +   *size = _memparse(p, );
>> > +   if (p == oldp)
>> > +   return -EINVAL;
>> > +
>> > +   switch (*p) {
>> > +   case '@':
>> > +   /* skip this region, usable */
>> > +   *start = 0;
>> > +   *size = 0;
>> > +   return 0;
>> > +   case '#':
>> > +   case '$':
>> > +   case '!':
>> > +   *start = _memparse(p + 1, );
>> > +   return 0;
>> 

Re: [PATCH v5] x86: fix kaslr and memmap collision

[Baoquan He]

Add Kees to let him have a look at this too.

On 01/05/17 at 05:21pm, Baoquan He wrote:
> On 01/04/17 at 11:29am, Dave Jiang wrote:
> > CONFIG_RANDOMIZE_BASE relocates the kernel to a random base address.
> > However it does not take into account the memmap= parameter passed in from
> > the kernel cmdline. This results in the kernel sometimes being put in
> > the middle of memmap. Teaching kaslr to not insert the kernel in
> > memmap defined regions. We will support up to 4 memmap regions. Any
> > additional regions will cause kaslr to disable. The mem_avoid set has
> > been augmented to add up to 4 unusable regions of memmaps provided by the
> > user to exclude those regions from the set of valid address range to insert
> > the uncompressed kernel image. The nn@ss ranges will be skipped by the
> > mem_avoid set since it indicates memory useable.
> > 
> > Signed-off-by: Dave Jiang 
> > ---
> > 
> > v2:
> > Addressing comments from Ingo.
> > - Handle entire list of memmaps
> > v3:
> > Fix 32bit build issue
> > v4:
> > Addressing comments from Baoquan
> > - Not exclude nn@ss ranges
> > v5:
> > Addressing additional comments from Baoquan
> > - Update commit header and various coding style changes
> > 
> > diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
> > index e5612f3..59c2075 100644
> > --- a/arch/x86/boot/boot.h
> > +++ b/arch/x86/boot/boot.h
> > @@ -332,7 +332,10 @@ int strncmp(const char *cs, const char *ct, size_t 
> > count);
> >  size_t strnlen(const char *s, size_t maxlen);
> >  unsigned int atou(const char *s);
> >  unsigned long long simple_strtoull(const char *cp, char **endp, unsigned 
> > int base);
> > +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int 
> > base);
> > +long simple_strtol(const char *cp, char **endp, unsigned int base);
> >  size_t strlen(const char *s);
> > +char *strchr(const char *s, int c);
> >  
> >  /* tty.c */
> >  void puts(const char *);
> > diff --git a/arch/x86/boot/compressed/kaslr.c 
> > b/arch/x86/boot/compressed/kaslr.c
> > index a66854d..036b514 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -11,6 +11,7 @@
> >   */
> >  #include "misc.h"
> >  #include "error.h"
> > +#include "../boot.h"
> >  
> >  #include 
> >  #include 
> > @@ -56,11 +57,16 @@ struct mem_vector {
> > unsigned long size;
> >  };
> >  
> > +/* only supporting at most 4 unusable memmap regions with kaslr */
> > +#define MAX_MEMMAP_REGIONS 4
> > +
> >  enum mem_avoid_index {
> > MEM_AVOID_ZO_RANGE = 0,
> > MEM_AVOID_INITRD,
> > MEM_AVOID_CMDLINE,
> > MEM_AVOID_BOOTPARAMS,
> > +   MEM_AVOID_MEMMAP_BEGIN,
> > +   MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
> > MEM_AVOID_MAX,
> >  };
> >  
> > @@ -77,6 +83,121 @@ 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 *endptr;   /* local pointer to end of parsed string */
> > +
> > +   unsigned long long ret = simple_strtoull(ptr, , 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;
> > +}
> > +
> > +static int
> > +parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> > +{
> > +   char *oldp;
> > +
> > +   if (!p)
> > +   return -EINVAL;
> > +
> > +   /* we don't care about this option here */
> > +   if (!strncmp(p, "exactmap", 8))
> > +   return -EINVAL;
> > +
> > +   oldp = p;
> > +   *size = _memparse(p, );
> > +   if (p == oldp)
> > +   return -EINVAL;
> > +
> > +   switch (*p) {
> > +   case '@':
> > +   /* skip this region, usable */
> > +   *start = 0;
> > +   *size = 0;
> > +   return 0;
> > +   case '#':
> > +   case '$':
> > +   case '!':
> > +   *start = _memparse(p + 1, );
> > +   return 0;
> > +   }
> > +
> > +   return -EINVAL;
> > +}
> > +
> > +static int mem_avoid_memmap(void)
> > +{
> > +   char arg[128];
> > +   int rc = 0;
> > +
> > +   /* see if we have any memmap areas */
> > +   if 

Re: [PATCH v5] x86: fix kaslr and memmap collision

[Baoquan He]

On 01/04/17 at 11:29am, Dave Jiang wrote:
> CONFIG_RANDOMIZE_BASE relocates the kernel to a random base address.
> However it does not take into account the memmap= parameter passed in from
> the kernel cmdline. This results in the kernel sometimes being put in
> the middle of memmap. Teaching kaslr to not insert the kernel in
> memmap defined regions. We will support up to 4 memmap regions. Any
> additional regions will cause kaslr to disable. The mem_avoid set has
> been augmented to add up to 4 unusable regions of memmaps provided by the
> user to exclude those regions from the set of valid address range to insert
> the uncompressed kernel image. The nn@ss ranges will be skipped by the
> mem_avoid set since it indicates memory useable.
> 
> Signed-off-by: Dave Jiang 
> ---
> 
> v2:
> Addressing comments from Ingo.
> - Handle entire list of memmaps
> v3:
> Fix 32bit build issue
> v4:
> Addressing comments from Baoquan
> - Not exclude nn@ss ranges
> v5:
> Addressing additional comments from Baoquan
> - Update commit header and various coding style changes
> 
> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
> index e5612f3..59c2075 100644
> --- a/arch/x86/boot/boot.h
> +++ b/arch/x86/boot/boot.h
> @@ -332,7 +332,10 @@ int strncmp(const char *cs, const char *ct, size_t 
> count);
>  size_t strnlen(const char *s, size_t maxlen);
>  unsigned int atou(const char *s);
>  unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int 
> base);
> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
> +long simple_strtol(const char *cp, char **endp, unsigned int base);
>  size_t strlen(const char *s);
> +char *strchr(const char *s, int c);
>  
>  /* tty.c */
>  void puts(const char *);
> diff --git a/arch/x86/boot/compressed/kaslr.c 
> b/arch/x86/boot/compressed/kaslr.c
> index a66854d..036b514 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -11,6 +11,7 @@
>   */
>  #include "misc.h"
>  #include "error.h"
> +#include "../boot.h"
>  
>  #include 
>  #include 
> @@ -56,11 +57,16 @@ struct mem_vector {
>   unsigned long size;
>  };
>  
> +/* only supporting at most 4 unusable memmap regions with kaslr */
> +#define MAX_MEMMAP_REGIONS   4
> +
>  enum mem_avoid_index {
>   MEM_AVOID_ZO_RANGE = 0,
>   MEM_AVOID_INITRD,
>   MEM_AVOID_CMDLINE,
>   MEM_AVOID_BOOTPARAMS,
> + MEM_AVOID_MEMMAP_BEGIN,
> + MEM_AVOID_MEMMAP_END = MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1,
>   MEM_AVOID_MAX,
>  };
>  
> @@ -77,6 +83,121 @@ 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 *endptr;   /* local pointer to end of parsed string */
> +
> + unsigned long long ret = simple_strtoull(ptr, , 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;
> +}
> +
> +static int
> +parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
> +{
> + char *oldp;
> +
> + if (!p)
> + return -EINVAL;
> +
> + /* we don't care about this option here */
> + if (!strncmp(p, "exactmap", 8))
> + return -EINVAL;
> +
> + oldp = p;
> + *size = _memparse(p, );
> + if (p == oldp)
> + return -EINVAL;
> +
> + switch (*p) {
> + case '@':
> + /* skip this region, usable */
> + *start = 0;
> + *size = 0;
> + return 0;
> + case '#':
> + case '$':
> + case '!':
> + *start = _memparse(p + 1, );
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int mem_avoid_memmap(void)
> +{
> + char arg[128];
> + int rc = 0;
> +
> + /* see if we have any memmap areas */
> + if (cmdline_find_option("memmap", arg, sizeof(arg)) > 0) {
> + int i = 0;
> + char *str = arg;
> +
> + while (str && (i < MAX_MEMMAP_REGIONS)) {
> + unsigned long long start, size;
> + char *k = strchr(str, ',');
> +