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, ',');
> +

[PATCH v5] x86: fix kaslr and memmap collision

[Dave Jiang]

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, ',');
+
+   if (k)
+   *k++ = 0;
+
+   rc = parse_memmap(str, , );
+   if (rc < 0)
+   break;
+   str =