Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550164 > unrolled thread

[PATCH v4] x86: fix kaslr and memmap collision

Back to article view | Back to linux.kernel

Contents

  [PATCH v4] x86: fix kaslr and memmap collision Dave Jiang <dave.jiang@intel.com> - 2017-01-03 21:50 +0100
    Re: [PATCH v4] x86: fix kaslr and memmap collision Baoquan He <bhe@redhat.com> - 2017-01-04 03:40 +0100
      Re: [PATCH v4] x86: fix kaslr and memmap collision Dave Jiang <dave.jiang@intel.com> - 2017-01-04 18:10 +0100
        Re: [PATCH v4] x86: fix kaslr and memmap collision Baoquan He <bhe@redhat.com> - 2017-01-05 02:10 +0100

#1550164 — [PATCH v4] x86: fix kaslr and memmap collision

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 the user 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 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 <dave.jiang@intel.com>
---
 arch/x86/boot/boot.h             |    3 +
 arch/x86/boot/compressed/kaslr.c |  131 ++++++++++++++++++++++++++++++++++++++
 arch/x86/boot/string.c           |   38 +++++++++++
 3 files changed, 172 insertions(+)

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..f5a1c8d 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 <generated/compile.h>
 #include <linux/module.h>
@@ -61,9 +62,16 @@ enum mem_avoid_index {
 	MEM_AVOID_INITRD,
 	MEM_AVOID_CMDLINE,
 	MEM_AVOID_BOOTPARAMS,
+	MEM_AVOID_MEMMAP1,
+	MEM_AVOID_MEMMAP2,
+	MEM_AVOID_MEMMAP3,
+	MEM_AVOID_MEMMAP4,
 	MEM_AVOID_MAX,
 };
 
+/* only supporting at most 4 memmap regions with kaslr */
+#define MAX_MEMMAP_REGIONS	4
+
 static struct mem_vector mem_avoid[MEM_AVOID_MAX];
 
 static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
@@ -77,6 +85,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, &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;
+}
+
+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, &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, &p);
+		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, &start, &size);
+			if (rc < 0)
+				break;
+			str = k;
+			/* a usable region that should not be skipped */
+			if (size == 0)
+				continue;
+
+			mem_avoid[MEM_AVOID_MEMMAP1 + i].start = start;
+			mem_avoid[MEM_AVOID_MEMMAP1 + i].size = size;
+			i++;
+		}
+
+		/* more than 4 memmaps, fail kaslr */
+		if ((i >= MAX_MEMMAP_REGIONS) && str)
+			rc = -EINVAL;
+	}
+
+	return rc;
+}
+
 /*
  * 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
@@ -429,6 +552,7 @@ void choose_random_location(unsigned long input,
 			    unsigned long *virt_addr)
 {
 	unsigned long random_addr, min_addr;
+	int rc;
 
 	/* By default, keep output position unchanged. */
 	*virt_addr = *output;
@@ -438,6 +562,13 @@ void choose_random_location(unsigned long input,
 		return;
 	}
 
+	/* Mark the memmap regions we need to avoid */
+	rc = mem_avoid_memmap();
+	if (rc < 0) {
+		warn("KASLR disabled: memmap exceeds limit of 4, giving up.");
+		return;
+	}
+
 	boot_params->hdr.loadflags |= KASLR_FLAG;
 
 	/* Prepare to add new identity pagetables on demand. */
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index cc3bd58..0464aaa 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -122,6 +122,31 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
 }
 
 /**
+ * simple_strtoul - convert a string to an unsigned long
+ * @cp: The start of the string
+ * @endp: A pointer to the end of the parsed string will be placed here
+ * @base: The number base to use
+ */
+unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
+{
+	return simple_strtoull(cp, endp, base);
+}
+
+/**
+ * simple_strtol - convert a string to a signed long
+ * @cp: The start of the string
+ * @endp: A pointer to the end of the parsed string will be placed here
+ * @base: The number base to use
+ */
+long simple_strtol(const char *cp, char **endp, unsigned int base)
+{
+	if (*cp == '-')
+		return -simple_strtoul(cp + 1, endp, base);
+
+	return simple_strtoul(cp, endp, base);
+}
+
+/**
  * strlen - Find the length of a string
  * @s: The string to be sized
  */
@@ -155,3 +180,16 @@ char *strstr(const char *s1, const char *s2)
 	}
 	return NULL;
 }
+
+/**
+ * strchr - Find the first occurrence of the character c in the string s.
+ * @s: the string to be searched
+ * @c: the character to search for
+ */
+char *strchr(const char *s, int c)
+{
+	while (*s != (char)c)
+		if (*s++ == '\0')
+			return NULL;
+	return (char *)s;
+}

[toc] | [next] | [standalone]

#1550380

Hi Dave,

I have several concerns, please see the inline comments.

On 01/03/17 at 01:48pm, 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 the user memmap. Teaching kaslr to not insert the kernel in
		    ~~~~~~~~~~~
It could be better to be replaced with "user-defined physical RAM map"
or memmap directly because memmap is meaning user-defined physical RAM
map. Please check the boot info printing about them. I was confused at
first glance.
> 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 regions of memmaps provided by the user
                              ~~~
    4 un-usable memmap region need be cared, usable memory is not
concerned.
> 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 <dave.jiang@intel.com>
> ---
>  arch/x86/boot/boot.h             |    3 +
>  arch/x86/boot/compressed/kaslr.c |  131 ++++++++++++++++++++++++++++++++++++++
>  arch/x86/boot/string.c           |   38 +++++++++++
>  3 files changed, 172 insertions(+)
> 
> 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..f5a1c8d 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 <generated/compile.h>
>  #include <linux/module.h>
> @@ -61,9 +62,16 @@ enum mem_avoid_index {
>  	MEM_AVOID_INITRD,
>  	MEM_AVOID_CMDLINE,
>  	MEM_AVOID_BOOTPARAMS,
> +	MEM_AVOID_MEMMAP1,
> +	MEM_AVOID_MEMMAP2,
> +	MEM_AVOID_MEMMAP3,
> +	MEM_AVOID_MEMMAP4,

This looks not good. Could it be done like fixed_addresses?
Something like:

	MEM_AVOID_MEMMAP_BEGIN,
	MEM_AVOID_MEMMAP_BEGIN + MEM_AVOID_MAX -1,

Please point it out to me if there's some existing code in kernel like
your way, I can also accept it.

>  	MEM_AVOID_MAX,
>  };
>  
> +/* only supporting at most 4 memmap regions with kaslr */
And here, "Only supporting at most 4 un-usable memmap regions with kaslr"?
Surely this is based on if you will ignore the usable memory and do not
store it as 0. And also the log need be changed too accordingly.
> +#define MAX_MEMMAP_REGIONS	4
> +
>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>  
>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
> @@ -77,6 +85,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, &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;
> +}
> +
> +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, &p);
> +	if (p == oldp)
> +		return -EINVAL;
> +
> +	switch (*p) {
> +	case '@':
> +		/* skip this region, usable */
> +		*start = 0;
> +		*size = 0;
> +		return 0;
How about direclty return if nn@ss? Seems no need to waste one mem avoid
region slot. In fact even amount of usable memory regions are provided to
100, it won't impact that you want to specify a reserve memmap region if
you skip it direclty. Personal opinion.
> +	case '#':
> +	case '$':
> +	case '!':
> +		*start = _memparse(p + 1, &p);
> +		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, &start, &size);
> +			if (rc < 0)
> +				break;
> +			str = k;
> +			/* a usable region that should not be skipped */
> +			if (size == 0)
> +				continue;
> +
> +			mem_avoid[MEM_AVOID_MEMMAP1 + i].start = start;
> +			mem_avoid[MEM_AVOID_MEMMAP1 + i].size = size;
> +			i++;
> +		}
> +
> +		/* more than 4 memmaps, fail kaslr */
> +		if ((i >= MAX_MEMMAP_REGIONS) && str)
> +			rc = -EINVAL;
> +	}
> +
> +	return rc;
> +}
> +
>  /*
>   * 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
> @@ -429,6 +552,7 @@ void choose_random_location(unsigned long input,
>  			    unsigned long *virt_addr)
>  {
>  	unsigned long random_addr, min_addr;
> +	int rc;
>  
>  	/* By default, keep output position unchanged. */
>  	*virt_addr = *output;
> @@ -438,6 +562,13 @@ void choose_random_location(unsigned long input,
>  		return;
>  	}
>  
> +	/* Mark the memmap regions we need to avoid */
> +	rc = mem_avoid_memmap();
> +	if (rc < 0) {
Or write it like below, the rc is not needed. Personal coding style,
just talk about it.
	if (mem_avoid_memmap()) {
		warn("KASLR disabled: memmap exceeds limit of 4, giving up.");
		return;
	}
> +
>  	boot_params->hdr.loadflags |= KASLR_FLAG;
>  
>  	/* Prepare to add new identity pagetables on demand. */
> diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
> index cc3bd58..0464aaa 100644
> --- a/arch/x86/boot/string.c
> +++ b/arch/x86/boot/string.c
> @@ -122,6 +122,31 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
>  }
>  
>  /**
> + * simple_strtoul - convert a string to an unsigned long
> + * @cp: The start of the string
> + * @endp: A pointer to the end of the parsed string will be placed here
> + * @base: The number base to use
> + */
> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
> +{
> +	return simple_strtoull(cp, endp, base);
> +}
> +
> +/**
> + * simple_strtol - convert a string to a signed long
> + * @cp: The start of the string
> + * @endp: A pointer to the end of the parsed string will be placed here
> + * @base: The number base to use
> + */
> +long simple_strtol(const char *cp, char **endp, unsigned int base)
> +{
> +	if (*cp == '-')
> +		return -simple_strtoul(cp + 1, endp, base);
> +
> +	return simple_strtoul(cp, endp, base);
> +}
> +
> +/**
>   * strlen - Find the length of a string
>   * @s: The string to be sized
>   */
> @@ -155,3 +180,16 @@ char *strstr(const char *s1, const char *s2)
>  	}
>  	return NULL;
>  }
> +
> +/**
> + * strchr - Find the first occurrence of the character c in the string s.
> + * @s: the string to be searched
> + * @c: the character to search for
> + */
> +char *strchr(const char *s, int c)
> +{
> +	while (*s != (char)c)
> +		if (*s++ == '\0')
> +			return NULL;
> +	return (char *)s;
> +}
> 

[toc] | [prev] | [next] | [standalone]

#1550995

On 01/03/2017 07:37 PM, Baoquan He wrote:
> Hi Dave,
> 
> I have several concerns, please see the inline comments.
> 
> On 01/03/17 at 01:48pm, 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 the user memmap. Teaching kaslr to not insert the kernel in
> 		    ~~~~~~~~~~~
> It could be better to be replaced with "user-defined physical RAM map"
> or memmap directly because memmap is meaning user-defined physical RAM
> map. Please check the boot info printing about them. I was confused at
> first glance.

Will update

>> 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 regions of memmaps provided by the user
>                               ~~~
>     4 un-usable memmap region need be cared, usable memory is not
> concerned

Will update

.
>> 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 <dave.jiang@intel.com>
>> ---
>>  arch/x86/boot/boot.h             |    3 +
>>  arch/x86/boot/compressed/kaslr.c |  131 ++++++++++++++++++++++++++++++++++++++
>>  arch/x86/boot/string.c           |   38 +++++++++++
>>  3 files changed, 172 insertions(+)
>>
>> 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..f5a1c8d 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 <generated/compile.h>
>>  #include <linux/module.h>
>> @@ -61,9 +62,16 @@ enum mem_avoid_index {
>>  	MEM_AVOID_INITRD,
>>  	MEM_AVOID_CMDLINE,
>>  	MEM_AVOID_BOOTPARAMS,
>> +	MEM_AVOID_MEMMAP1,
>> +	MEM_AVOID_MEMMAP2,
>> +	MEM_AVOID_MEMMAP3,
>> +	MEM_AVOID_MEMMAP4,
> 
> This looks not good. Could it be done like fixed_addresses?
> Something like:
> 
> 	MEM_AVOID_MEMMAP_BEGIN,
> 	MEM_AVOID_MEMMAP_BEGIN + MEM_AVOID_MAX -1,
> 
> Please point it out to me if there's some existing code in kernel like
> your way, I can also accept it.

I think you mean:
MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1

I will change


> 
>>  	MEM_AVOID_MAX,
>>  };
>>  
>> +/* only supporting at most 4 memmap regions with kaslr */
> And here, "Only supporting at most 4 un-usable memmap regions with kaslr"?
> Surely this is based on if you will ignore the usable memory and do not
> store it as 0. And also the log need be changed too accordingly.
>> +#define MAX_MEMMAP_REGIONS	4
>> +
>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>>  
>>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>> @@ -77,6 +85,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, &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;
>> +}
>> +
>> +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, &p);
>> +	if (p == oldp)
>> +		return -EINVAL;
>> +
>> +	switch (*p) {
>> +	case '@':
>> +		/* skip this region, usable */
>> +		*start = 0;
>> +		*size = 0;
>> +		return 0;
> How about direclty return if nn@ss? Seems no need to waste one mem avoid
> region slot. In fact even amount of usable memory regions are provided to
> 100, it won't impact that you want to specify a reserve memmap region if
> you skip it direclty. Personal opinion.

We are not wasting the slot. If you look mem_avoid_memmap() where I call
the function, it will skip with a continue if size == 0 without
incrementing the 'i' counter. That will skip all the nn@ss regions
without counting against the max avoid mapping.

>> +	case '#':
>> +	case '$':
>> +	case '!':
>> +		*start = _memparse(p + 1, &p);
>> +		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, &start, &size);
>> +			if (rc < 0)
>> +				break;
>> +			str = k;
>> +			/* a usable region that should not be skipped */
>> +			if (size == 0)
>> +				continue;
>> +
>> +			mem_avoid[MEM_AVOID_MEMMAP1 + i].start = start;
>> +			mem_avoid[MEM_AVOID_MEMMAP1 + i].size = size;
>> +			i++;
>> +		}
>> +
>> +		/* more than 4 memmaps, fail kaslr */
>> +		if ((i >= MAX_MEMMAP_REGIONS) && str)
>> +			rc = -EINVAL;
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>>  /*
>>   * 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
>> @@ -429,6 +552,7 @@ void choose_random_location(unsigned long input,
>>  			    unsigned long *virt_addr)
>>  {
>>  	unsigned long random_addr, min_addr;
>> +	int rc;
>>  
>>  	/* By default, keep output position unchanged. */
>>  	*virt_addr = *output;
>> @@ -438,6 +562,13 @@ void choose_random_location(unsigned long input,
>>  		return;
>>  	}
>>  
>> +	/* Mark the memmap regions we need to avoid */
>> +	rc = mem_avoid_memmap();
>> +	if (rc < 0) {
> Or write it like below, the rc is not needed. Personal coding style,
> just talk about it.
> 	if (mem_avoid_memmap()) {
> 		warn("KASLR disabled: memmap exceeds limit of 4, giving up.");
> 		return;
> 	}

Doesn't matter to me. I'll update as you suggest.

>> +
>>  	boot_params->hdr.loadflags |= KASLR_FLAG;
>>  
>>  	/* Prepare to add new identity pagetables on demand. */
>> diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
>> index cc3bd58..0464aaa 100644
>> --- a/arch/x86/boot/string.c
>> +++ b/arch/x86/boot/string.c
>> @@ -122,6 +122,31 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
>>  }
>>  
>>  /**
>> + * simple_strtoul - convert a string to an unsigned long
>> + * @cp: The start of the string
>> + * @endp: A pointer to the end of the parsed string will be placed here
>> + * @base: The number base to use
>> + */
>> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
>> +{
>> +	return simple_strtoull(cp, endp, base);
>> +}
>> +
>> +/**
>> + * simple_strtol - convert a string to a signed long
>> + * @cp: The start of the string
>> + * @endp: A pointer to the end of the parsed string will be placed here
>> + * @base: The number base to use
>> + */
>> +long simple_strtol(const char *cp, char **endp, unsigned int base)
>> +{
>> +	if (*cp == '-')
>> +		return -simple_strtoul(cp + 1, endp, base);
>> +
>> +	return simple_strtoul(cp, endp, base);
>> +}
>> +
>> +/**
>>   * strlen - Find the length of a string
>>   * @s: The string to be sized
>>   */
>> @@ -155,3 +180,16 @@ char *strstr(const char *s1, const char *s2)
>>  	}
>>  	return NULL;
>>  }
>> +
>> +/**
>> + * strchr - Find the first occurrence of the character c in the string s.
>> + * @s: the string to be searched
>> + * @c: the character to search for
>> + */
>> +char *strchr(const char *s, int c)
>> +{
>> +	while (*s != (char)c)
>> +		if (*s++ == '\0')
>> +			return NULL;
>> +	return (char *)s;
>> +}
>>

[toc] | [prev] | [next] | [standalone]

#1551539

On 01/04/17 at 10:06am, Dave Jiang wrote:
> On 01/03/2017 07:37 PM, Baoquan He wrote:
> >>  #include <generated/compile.h>
> >>  #include <linux/module.h>
> >> @@ -61,9 +62,16 @@ enum mem_avoid_index {
> >>  	MEM_AVOID_INITRD,
> >>  	MEM_AVOID_CMDLINE,
> >>  	MEM_AVOID_BOOTPARAMS,
> >> +	MEM_AVOID_MEMMAP1,
> >> +	MEM_AVOID_MEMMAP2,
> >> +	MEM_AVOID_MEMMAP3,
> >> +	MEM_AVOID_MEMMAP4,
> > 
> > This looks not good. Could it be done like fixed_addresses?
> > Something like:
> > 
> > 	MEM_AVOID_MEMMAP_BEGIN,
> > 	MEM_AVOID_MEMMAP_BEGIN + MEM_AVOID_MAX -1,
> > 
> > Please point it out to me if there's some existing code in kernel like
> > your way, I can also accept it.
> 
> I think you mean:
> MEM_AVOID_MEMMAP_BEGIN + MAX_MEMMAP_REGIONS - 1

Ah, yes. Sorry for this.

> 
> I will change
> 
> 
> > 
> >>  	MEM_AVOID_MAX,
> >>  };
> >>  
> >> +/* only supporting at most 4 memmap regions with kaslr */
> > And here, "Only supporting at most 4 un-usable memmap regions with kaslr"?
> > Surely this is based on if you will ignore the usable memory and do not
> > store it as 0. And also the log need be changed too accordingly.
> >> +#define MAX_MEMMAP_REGIONS	4
...
> >> +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, &p);
> >> +	if (p == oldp)
> >> +		return -EINVAL;
> >> +
> >> +	switch (*p) {
> >> +	case '@':
> >> +		/* skip this region, usable */
> >> +		*start = 0;
> >> +		*size = 0;
> >> +		return 0;
> > How about direclty return if nn@ss? Seems no need to waste one mem avoid
> > region slot. In fact even amount of usable memory regions are provided to
> > 100, it won't impact that you want to specify a reserve memmap region if
> > you skip it direclty. Personal opinion.
> 
> We are not wasting the slot. If you look mem_avoid_memmap() where I call
> the function, it will skip with a continue if size == 0 without
> incrementing the 'i' counter. That will skip all the nn@ss regions
> without counting against the max avoid mapping.

Yes, indeed. Sorry, I didn't read the code carefully.

Thanks
Baoquan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web