[PATCH v2 0/3] Handle memmap and mem kernel options in boot stage kaslr

[PATCH v2 0/3] Handle memmap and mem kernel options in boot stage kaslr

[Baoquan He]

People reported kernel panic occurs during system boots up with mem boot option.
After checking code, several problems are found about memmap= and mem= in boot stage
kaslr.

*) In commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision"), only one memmap
   entry is considered and only the last one if multiple memmap entries are specified.

*) mem= and memmap=nn[KMG] are not considered yet. They are used to limit max address
   of system. Kernel can't be randomized to be above the limit.

*) kernel-parameters.txt doesn't tell the updated behaviour of memmap=.

This patchset tries to solve above issues.

Changelog:
v1->v2

a)
  The original patch 1/4 has been put in tip:x86/boot and no update,
  so it's not included in this post.

b)
  Use patch log Ingo reorganized.

c)
  lib/ctype.c and lib/cmdline.c are needed for kaslr.c, while those
  EXPORT_SYMBOL(x) contained caused failure of build on 32-bit allmodconfig:
  ...... 
  ld: -r and -shared may not be used together
  scripts/Makefile.build:294: recipe for target 'arch/x86/boot/compressed/kaslr.o' failed
  ......
  Disabling the symbol exporting removes the build failure.

d)
  Use dynamic allocation to allocate memory to contain copied kernel cmdline
  buffer, it's implemented in include/linux/decompress/mm.h.

e)
  This patchset sits on top of tip:x86/boot branch.

Baoquan He (3):
  KASLR: Parse all memmap entries in cmdline
  KASLR: Handle memory limit specified by memmap and mem option
  Documentation/kernel-parameters.txt: Update 'memmap=' option
    description

 Documentation/admin-guide/kernel-parameters.txt |   9 ++
 arch/x86/boot/compressed/cmdline.c              |   2 +-
 arch/x86/boot/compressed/kaslr.c                | 185 +++++++++++++++---------
 arch/x86/boot/string.c                          |   8 +
 4 files changed, 138 insertions(+), 66 deletions(-)

-- 
2.5.5

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

[Baoquan He]

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   | 127 +++++++++++++++++++++----------------
 arch/x86/boot/string.c             |   8 +++
 3 files changed, 82 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..6649ecd 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
+
+/*
+ * 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)
+
 #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>
 #include <generated/utsrelease.h>
 
 /* Simplified build-specific string for starting entropy. */
@@ -61,6 +75,9 @@ struct mem_vector {
 #define MAX_MEMMAP_REGIONS	4
 
 static bool memmap_too_large;
+int mem_avoid_memmap_index;
+extern unsigned long get_cmd_line_ptr(void);
+
 
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
@@ -85,49 +102,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 +124,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 +137,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];
 	int rc;
-	int i;
-	char *str;
+	int i = mem_avoid_memmap_index;
 
-	/* 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;
@@ -196,12 +172,55 @@ static void mem_avoid_memmap(void)
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
 		i++;
 	}
+	mem_avoid_memmap_index = i;
 
 	/* More than 4 memmaps, fail kaslr */
 	if ((i >= MAX_MEMMAP_REGIONS) && str)
 		memmap_too_large = true;
 }
 
+
+/* Macros used by the included decompressor code below. */
+#define STATIC
+#include <linux/decompress/mm.h>
+
+#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;
+
+	tmp_cmdline = malloc(COMMAND_LINE_SIZE);
+	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 +342,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();
 
 #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

[PATCH v2 2/3] KASLR: Handle memory limit specified by memmap and mem option

[Baoquan He]

Option mem= will limit the max address a system can use and any memory
region above the limit will be removed.

Furthermore, memmap=nn[KMG] which has no offset specified has the same
behaviour as mem=.

KASLR needs to consider this when choosing the random position for
decompressing the kernel.

This patch implements that.

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/kaslr.c | 60 ++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 12 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 6649ecd..34f66a5 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -79,6 +79,10 @@ int mem_avoid_memmap_index;
 extern unsigned long get_cmd_line_ptr(void);
 
 
+/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
+unsigned long long mem_limit = ULLONG_MAX;
+
+
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
 	MEM_AVOID_INITRD,
@@ -129,16 +133,22 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 		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;
+	case '@':
+		/* memmap=nn@ss specifies usable region, should be skipped */
+		*size = 0;
+	default:
+		/*
+		 * If w/o offset, only size specified, memmap=nn[KMG] has the
+		 * same behaviour as mem=nn[KMG]. It limits the max address
+		 * system can use. Region above the limit should be avoided.
+		 */
+		*start = 0;
+		return 0;
 	}
 
 	return -EINVAL;
@@ -164,9 +174,14 @@ static void mem_avoid_memmap(char *str)
 		if (rc < 0)
 			break;
 		str = k;
-		/* A usable region that should not be skipped */
-		if (size == 0)
+
+		if (start == 0) {
+			/* Store the specified memory limit if size > 0 */
+			if (size > 0)
+				mem_limit = size;
+
 			continue;
+		}
 
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
 		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
@@ -191,6 +206,7 @@ static int handle_mem_memmap(void)
 	size_t len = strlen((char *)args);
 	char *tmp_cmdline;
 	char *param, *val;
+	u64 mem_size;
 
 	tmp_cmdline = malloc(COMMAND_LINE_SIZE);
 	if (!tmp_cmdline )
@@ -213,8 +229,20 @@ static int handle_mem_memmap(void)
 			return -1;
 		}
 
-		if (!strcmp(param, "memmap"))
+		if (!strcmp(param, "memmap")) {
 			mem_avoid_memmap(val);
+		} else if (!strcmp(param, "mem")) {
+			char *p = val;
+
+			if (!strcmp(p, "nopentium"))
+				continue;
+			mem_size = memparse(p, &p);
+			if (mem_size == 0) {
+				free(tmp_cmdline);
+				return -EINVAL;
+			}
+			mem_limit = mem_size;
+		}
 	}
 
 	free(tmp_cmdline);
@@ -451,7 +479,8 @@ static void process_e820_entry(struct boot_e820_entry *entry,
 {
 	struct mem_vector region, overlap;
 	struct slot_area slot_area;
-	unsigned long start_orig;
+	unsigned long start_orig, end;
+	struct boot_e820_entry cur_entry;
 
 	/* Skip non-RAM entries. */
 	if (entry->type != E820_TYPE_RAM)
@@ -465,8 +494,15 @@ static void process_e820_entry(struct boot_e820_entry *entry,
 	if (entry->addr + entry->size < minimum)
 		return;
 
-	region.start = entry->addr;
-	region.size = entry->size;
+	/* Ignore entries above memory limit */
+	end = min(entry->size + entry->addr, mem_limit);
+	if (entry->addr >= end)
+		return;
+	cur_entry.addr = entry->addr;
+	cur_entry.size = end - entry->addr;
+
+	region.start = cur_entry.addr;
+	region.size = cur_entry.size;
 
 	/* Give up if slot area array is full. */
 	while (slot_area_index < MAX_SLOT_AREA) {
@@ -480,7 +516,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
 		region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
 
 		/* Did we raise the address above this e820 region? */
-		if (region.start > entry->addr + entry->size)
+		if (region.start > cur_entry.addr + cur_entry.size)
 			return;
 
 		/* Reduce size by any delta from the original address. */
-- 
2.5.5

[PATCH v2 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description

[Baoquan He]

In commit:

  9710f581bb4c ("x86, mm: Let "memmap=" take more entries one time")

... 'memmap=' was changed to adopt multiple, comma delimited values in a
single entry, so update the related description.

In the special case of only specifying size value without an offset,
like memmap=nn[KMG], memmap behaves similarly to mem=nn[KMG], so update
it too here.

Furthermore, for memmap=nn[KMG]$ss[KMG], an escape character needs be added
before '$' for some bootloaders. E.g in grub2, if we specify memmap=100M$5G
as suggested by the documentation, "memmap=100MG" gets passed to the kernel.

Clarify all this.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-doc@vger.kernel.org
---
 Documentation/admin-guide/kernel-parameters.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 986e443..4054328 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2097,6 +2097,12 @@
 	memmap=nn[KMG]@ss[KMG]
 			[KNL] Force usage of a specific region of memory.
 			Region of memory to be used is from ss to ss+nn.
+			If @ss[KMG] is ommited, it equals to mem=nn[KMG]
+			which limits max address as nn[KMG].
+			Multiple different options can be put into one entry
+			with comma delimited to save space:
+			Example:
+				memmap=100M@2G,100M#3G,1G!1024G
 
 	memmap=nn[KMG]#ss[KMG]
 			[KNL,ACPI] Mark specific memory as ACPI data.
@@ -2109,6 +2115,9 @@
 			         memmap=64K$0x18690000
 			         or
 			         memmap=0x10000$0x18690000
+			Some bootloaders may need escape character before '$',
+			like in grub2, otherwise '$' and the following number
+			will be eaten.
 
 	memmap=nn[KMG]!ss[KMG]
 			[KNL,X86] Mark specific memory as protected.
-- 
2.5.5

Re: [PATCH v2 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description

[Dou Liyang]

At 04/24/2017 10:40 AM, Baoquan He wrote:
> In commit:
>
>   9710f581bb4c ("x86, mm: Let "memmap=" take more entries one time")
>
> ... 'memmap=' was changed to adopt multiple, comma delimited values in a
> single entry, so update the related description.
>
> In the special case of only specifying size value without an offset,
> like memmap=nn[KMG], memmap behaves similarly to mem=nn[KMG], so update
> it too here.
>
> Furthermore, for memmap=nn[KMG]$ss[KMG], an escape character needs be added
> before '$' for some bootloaders. E.g in grub2, if we specify memmap=100M$5G
> as suggested by the documentation, "memmap=100MG" gets passed to the kernel.
>
> Clarify all this.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-doc@vger.kernel.org
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 986e443..4054328 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2097,6 +2097,12 @@
>  	memmap=nn[KMG]@ss[KMG]
>  			[KNL] Force usage of a specific region of memory.
>  			Region of memory to be used is from ss to ss+nn.
> +			If @ss[KMG] is ommited, it equals to mem=nn[KMG]

s/ommited/omitted/

Thanks,
	Liyang
> +			which limits max address as nn[KMG].
> +			Multiple different options can be put into one entry
> +			with comma delimited to save space:
> +			Example:
> +				memmap=100M@2G,100M#3G,1G!1024G
>
>  	memmap=nn[KMG]#ss[KMG]
>  			[KNL,ACPI] Mark specific memory as ACPI data.
> @@ -2109,6 +2115,9 @@
>  			         memmap=64K$0x18690000
>  			         or
>  			         memmap=0x10000$0x18690000
> +			Some bootloaders may need escape character before '$',
> +			like in grub2, otherwise '$' and the following number
> +			will be eaten.
>
>  	memmap=nn[KMG]!ss[KMG]
>  			[KNL,X86] Mark specific memory as protected.
>

Re: [PATCH v2 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description

[Baoquan He]

On 04/24/17 at 11:53am, Dou Liyang wrote:
> 
> 
> At 04/24/2017 10:40 AM, Baoquan He wrote:
> > In commit:
> > 
> >   9710f581bb4c ("x86, mm: Let "memmap=" take more entries one time")
> > 
> > ... 'memmap=' was changed to adopt multiple, comma delimited values in a
> > single entry, so update the related description.
> > 
> > In the special case of only specifying size value without an offset,
> > like memmap=nn[KMG], memmap behaves similarly to mem=nn[KMG], so update
> > it too here.
> > 
> > Furthermore, for memmap=nn[KMG]$ss[KMG], an escape character needs be added
> > before '$' for some bootloaders. E.g in grub2, if we specify memmap=100M$5G
> > as suggested by the documentation, "memmap=100MG" gets passed to the kernel.
> > 
> > Clarify all this.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: linux-doc@vger.kernel.org
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 986e443..4054328 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2097,6 +2097,12 @@
> >  	memmap=nn[KMG]@ss[KMG]
> >  			[KNL] Force usage of a specific region of memory.
> >  			Region of memory to be used is from ss to ss+nn.
> > +			If @ss[KMG] is ommited, it equals to mem=nn[KMG]
> 
> s/ommited/omitted/

Right, thanks. Will see if there's other comment and update.

> 
> > +			which limits max address as nn[KMG].
> > +			Multiple different options can be put into one entry
> > +			with comma delimited to save space:
> > +			Example:
> > +				memmap=100M@2G,100M#3G,1G!1024G
> > 
> >  	memmap=nn[KMG]#ss[KMG]
> >  			[KNL,ACPI] Mark specific memory as ACPI data.
> > @@ -2109,6 +2115,9 @@
> >  			         memmap=64K$0x18690000
> >  			         or
> >  			         memmap=0x10000$0x18690000
> > +			Some bootloaders may need escape character before '$',
> > +			like in grub2, otherwise '$' and the following number
> > +			will be eaten.
> > 
> >  	memmap=nn[KMG]!ss[KMG]
> >  			[KNL,X86] Mark specific memory as protected.
> > 
> 
> 

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

[Dou Liyang]

Hi Baoquan,

At 04/24/2017 10:40 AM, Baoquan He 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.
>
[...]
>
> -static void mem_avoid_memmap(void)
> +static void mem_avoid_memmap(char *str)
>  {
> -	char arg[128];
>  	int rc;
> -	int i;
> -	char *str;
> +	int i = mem_avoid_memmap_index;

Is it better that we make that variable *static* to remove the global
variable(mem_avoid_memmap_index)?

-       int i = mem_avoid_memmap_index;
+       static int i;

         if (i >= MAX_MEMMAP_REGIONS)
                 return;
@@ -172,7 +172,6 @@ static void mem_avoid_memmap(char *str)
                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
                 i++;
         }
-       mem_avoid_memmap_index = i;

         /* More than 4 memmaps, fail kaslr */
         if ((i >= MAX_MEMMAP_REGIONS) && 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 guess we just parsed and handled 4 MEMMAP_REGIONS and might ignore
the following in the whole cmdline.

Is it reasonable?  Is there any priority? The smaller the size, the more 
priority?

Thanks,
	Liyang

> -	i = 0;
> -	str = arg;
>  	while (str && (i < MAX_MEMMAP_REGIONS)) {
>  		int rc;
>  		unsigned long long start, size;
> @@ -196,12 +172,55 @@ static void mem_avoid_memmap(void)
>  		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
>  		i++;
>  	}
> +	mem_avoid_memmap_index = i;
>
>  	/* More than 4 memmaps, fail kaslr */
>  	if ((i >= MAX_MEMMAP_REGIONS) && str)
>  		memmap_too_large = true;
>  }
>
> +
> +/* Macros used by the included decompressor code below. */
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#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;
> +
> +	tmp_cmdline = malloc(COMMAND_LINE_SIZE);
> +	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 +342,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();
>
>  #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
>

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

[kbuild test robot]

Hi Baoquan,

[auto build test WARNING on next-20170421]
[also build test WARNING on v4.11-rc8]
[cannot apply to tip/x86/core linus/master linux/master v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Baoquan-He/Handle-memmap-and-mem-kernel-options-in-boot-stage-kaslr/20170424-104809
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> arch/x86/boot/compressed/kaslr.c:78:5: sparse: symbol 'mem_avoid_memmap_index' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] KASLR: mem_avoid_memmap_index can be static

[kbuild test robot]

Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 kaslr.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 6649ecd..7190d35 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -75,7 +75,7 @@ struct mem_vector {
 #define MAX_MEMMAP_REGIONS	4
 
 static bool memmap_too_large;
-int mem_avoid_memmap_index;
+static int mem_avoid_memmap_index;
 extern unsigned long get_cmd_line_ptr(void);
 
 

Re: [RFC PATCH] KASLR: mem_avoid_memmap_index can be static

[Baoquan He]

Hi Fengguang,

Thanks for pointing it out!

I am fine with defining mem_avoid_memmap_index it as static variable.
While Liyang suggested using a local static inside mem_avoid_memmap,
then mem_avoid_memmap_index is not needed any more. Do you think it's OK
to you?

Thanks
Baoquan

On 04/24/17 at 04:48pm, kbuild test robot wrote:
> 
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
>  kaslr.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 6649ecd..7190d35 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -75,7 +75,7 @@ struct mem_vector {
>  #define MAX_MEMMAP_REGIONS	4
>  
>  static bool memmap_too_large;
> -int mem_avoid_memmap_index;
> +static int mem_avoid_memmap_index;
>  extern unsigned long get_cmd_line_ptr(void);
>  
>  

Re: [kbuild-all] [RFC PATCH] KASLR: mem_avoid_memmap_index can be static

[Fengguang Wu]

Hi Baoquan,

On Mon, Apr 24, 2017 at 05:00:40PM +0800, Baoquan He wrote:
>Hi Fengguang,
>
>Thanks for pointing it out!
>
>I am fine with defining mem_avoid_memmap_index it as static variable.
>While Liyang suggested using a local static inside mem_avoid_memmap,
>then mem_avoid_memmap_index is not needed any more. Do you think it's OK
>to you?

Yes, that'd be a better solution. It's a robot generated patch and is
open for optimization or correction.

Thanks,
Fengguang

>On 04/24/17 at 04:48pm, kbuild test robot wrote:
>>
>> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>> ---
>>  kaslr.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 6649ecd..7190d35 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -75,7 +75,7 @@ struct mem_vector {
>>  #define MAX_MEMMAP_REGIONS	4
>>
>>  static bool memmap_too_large;
>> -int mem_avoid_memmap_index;
>> +static int mem_avoid_memmap_index;
>>  extern unsigned long get_cmd_line_ptr(void);
>>
>>
>_______________________________________________
>kbuild-all mailing list
>kbuild-all@lists.01.org
>https://lists.01.org/mailman/listinfo/kbuild-all

Re: [kbuild-all] [RFC PATCH] KASLR: mem_avoid_memmap_index can be static

[Baoquan He]

On 04/24/17 at 05:04pm, Fengguang Wu wrote:
> Hi Baoquan,
> 
> On Mon, Apr 24, 2017 at 05:00:40PM +0800, Baoquan He wrote:
> > Hi Fengguang,
> > 
> > Thanks for pointing it out!
> > 
> > I am fine with defining mem_avoid_memmap_index it as static variable.
> > While Liyang suggested using a local static inside mem_avoid_memmap,
> > then mem_avoid_memmap_index is not needed any more. Do you think it's OK
> > to you?
> 
> Yes, that'd be a better solution. It's a robot generated patch and is
> open for optimization or correction.

Oh, the tip robot is so great! Thanks for telling, then let me repost.

> 
> > On 04/24/17 at 04:48pm, kbuild test robot wrote:
> > > 
> > > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> > > ---
> > >  kaslr.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > > index 6649ecd..7190d35 100644
> > > --- a/arch/x86/boot/compressed/kaslr.c
> > > +++ b/arch/x86/boot/compressed/kaslr.c
> > > @@ -75,7 +75,7 @@ struct mem_vector {
> > >  #define MAX_MEMMAP_REGIONS	4
> > > 
> > >  static bool memmap_too_large;
> > > -int mem_avoid_memmap_index;
> > > +static int mem_avoid_memmap_index;
> > >  extern unsigned long get_cmd_line_ptr(void);
> > > 
> > > 
> > _______________________________________________
> > kbuild-all mailing list
> > kbuild-all@lists.01.org
> > https://lists.01.org/mailman/listinfo/kbuild-all

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

[Baoquan He]

On 04/24/17 at 04:00pm, Dou Liyang wrote:
> Hi Baoquan,
> 
> At 04/24/2017 10:40 AM, Baoquan He 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.
> > 
> [...]
> > 
> > -static void mem_avoid_memmap(void)
> > +static void mem_avoid_memmap(char *str)
> >  {
> > -	char arg[128];
> >  	int rc;
> > -	int i;
> > -	char *str;
> > +	int i = mem_avoid_memmap_index;
> 
> Is it better that we make that variable *static* to remove the global
> variable(mem_avoid_memmap_index)?

Yeah, I am fine with it. Can change lik this:

> 
> -       int i = mem_avoid_memmap_index;
> +       static int i;
> 
>         if (i >= MAX_MEMMAP_REGIONS)
>                 return;
> @@ -172,7 +172,6 @@ static void mem_avoid_memmap(char *str)
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
>                 i++;
>         }
> -       mem_avoid_memmap_index = i;
> 
>         /* More than 4 memmaps, fail kaslr */
>         if ((i >= MAX_MEMMAP_REGIONS) && 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 guess we just parsed and handled 4 MEMMAP_REGIONS and might ignore
> the following in the whole cmdline.

Right
> 
> Is it reasonable?  Is there any priority? The smaller the size, the more
> priority?

It's on purpose. Please see the discussion during Dave Jiang's patch
posting.

commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision")
> 
> 
> > -	i = 0;
> > -	str = arg;
> >  	while (str && (i < MAX_MEMMAP_REGIONS)) {
> >  		int rc;
> >  		unsigned long long start, size;
> > @@ -196,12 +172,55 @@ static void mem_avoid_memmap(void)
> >  		mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> >  		i++;
> >  	}
> > +	mem_avoid_memmap_index = i;
> > 
> >  	/* More than 4 memmaps, fail kaslr */
> >  	if ((i >= MAX_MEMMAP_REGIONS) && str)
> >  		memmap_too_large = true;
> >  }
> > 
> > +
> > +/* Macros used by the included decompressor code below. */
> > +#define STATIC
> > +#include <linux/decompress/mm.h>
> > +
> > +#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;
> > +
> > +	tmp_cmdline = malloc(COMMAND_LINE_SIZE);
> > +	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 +342,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();
> > 
> >  #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
> > 
> 
> 

Re: [PATCH v2 2/3] KASLR: Handle memory limit specified by memmap and mem option

[Kees Cook]

On Sun, Apr 23, 2017 at 7:40 PM, Baoquan He <bhe@redhat.com> wrote:
> Option mem= will limit the max address a system can use and any memory
> region above the limit will be removed.
>
> Furthermore, memmap=nn[KMG] which has no offset specified has the same
> behaviour as mem=.
>
> KASLR needs to consider this when choosing the random position for
> decompressing the kernel.
>
> This patch implements that.
>
> 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/kaslr.c | 60 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 48 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 6649ecd..34f66a5 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -79,6 +79,10 @@ int mem_avoid_memmap_index;
>  extern unsigned long get_cmd_line_ptr(void);
>
>
> +/* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
> +unsigned long long mem_limit = ULLONG_MAX;
> +
> +
>  enum mem_avoid_index {
>         MEM_AVOID_ZO_RANGE = 0,
>         MEM_AVOID_INITRD,
> @@ -129,16 +133,22 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>                 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;
> +       case '@':
> +               /* memmap=nn@ss specifies usable region, should be skipped */
> +               *size = 0;
> +       default:
> +               /*
> +                * If w/o offset, only size specified, memmap=nn[KMG] has the
> +                * same behaviour as mem=nn[KMG]. It limits the max address
> +                * system can use. Region above the limit should be avoided.
> +                */
> +               *start = 0;
> +               return 0;
>         }
>
>         return -EINVAL;
> @@ -164,9 +174,14 @@ static void mem_avoid_memmap(char *str)
>                 if (rc < 0)
>                         break;
>                 str = k;
> -               /* A usable region that should not be skipped */
> -               if (size == 0)
> +
> +               if (start == 0) {
> +                       /* Store the specified memory limit if size > 0 */
> +                       if (size > 0)
> +                               mem_limit = size;
> +
>                         continue;
> +               }
>
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start;
>                 mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size;
> @@ -191,6 +206,7 @@ static int handle_mem_memmap(void)
>         size_t len = strlen((char *)args);
>         char *tmp_cmdline;
>         char *param, *val;
> +       u64 mem_size;
>
>         tmp_cmdline = malloc(COMMAND_LINE_SIZE);
>         if (!tmp_cmdline )
> @@ -213,8 +229,20 @@ static int handle_mem_memmap(void)
>                         return -1;
>                 }
>
> -               if (!strcmp(param, "memmap"))
> +               if (!strcmp(param, "memmap")) {
>                         mem_avoid_memmap(val);
> +               } else if (!strcmp(param, "mem")) {
> +                       char *p = val;
> +
> +                       if (!strcmp(p, "nopentium"))
> +                               continue;
> +                       mem_size = memparse(p, &p);
> +                       if (mem_size == 0) {
> +                               free(tmp_cmdline);
> +                               return -EINVAL;
> +                       }
> +                       mem_limit = mem_size;
> +               }
>         }
>
>         free(tmp_cmdline);
> @@ -451,7 +479,8 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>  {
>         struct mem_vector region, overlap;
>         struct slot_area slot_area;
> -       unsigned long start_orig;
> +       unsigned long start_orig, end;
> +       struct boot_e820_entry cur_entry;
>
>         /* Skip non-RAM entries. */
>         if (entry->type != E820_TYPE_RAM)
> @@ -465,8 +494,15 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>         if (entry->addr + entry->size < minimum)
>                 return;
>
> -       region.start = entry->addr;
> -       region.size = entry->size;
> +       /* Ignore entries above memory limit */
> +       end = min(entry->size + entry->addr, mem_limit);
> +       if (entry->addr >= end)
> +               return;
> +       cur_entry.addr = entry->addr;
> +       cur_entry.size = end - entry->addr;
> +
> +       region.start = cur_entry.addr;
> +       region.size = cur_entry.size;
>
>         /* Give up if slot area array is full. */
>         while (slot_area_index < MAX_SLOT_AREA) {
> @@ -480,7 +516,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
>                 region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
>
>                 /* Did we raise the address above this e820 region? */
> -               if (region.start > entry->addr + entry->size)
> +               if (region.start > cur_entry.addr + cur_entry.size)
>                         return;
>
>                 /* Reduce size by any delta from the original address. */
> --
> 2.5.5
>

Thanks! I find this much more readable. :)

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Pixel Security