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

[PATCH v3 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, and it sits on top of
tip:x86/boot branch.

Changelog

v2->v3:
No functionality change in this round.
a)Use local static variable insted of global variable
  mem_avoid_memmap_index in patch 1/3.
b)Fix a typo in patch 3/3.

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.



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                | 183 +++++++++++++++---------
 arch/x86/boot/string.c                          |   8 ++
 4 files changed, 136 insertions(+), 66 deletions(-)

-- 
2.5.5

[PATCH v3 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   | 125 +++++++++++++++++++++----------------
 arch/x86/boot/string.c             |   8 +++
 3 files changed, 80 insertions(+), 55 deletions(-)

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index 73ccf63..9dc1ce6 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -13,7 +13,7 @@ static inline char rdfs8(addr_t addr)
 	return *((char *)(fs + addr));
 }
 #include "../cmdline.c"
-static unsigned long get_cmd_line_ptr(void)
+unsigned long get_cmd_line_ptr(void)
 {
 	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
 
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 6d9a546..53a06ec 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -9,14 +9,28 @@
  * contain the entire properly aligned running kernel image.
  *
  */
+
+/*
+ * linux/ctype.h is expected, while conflicts with boot/ctype.h
+ * which is useless here. Do not include it.
+ */
+#define BOOT_CTYPE_H
+
+/*
+ * 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,8 @@ struct mem_vector {
 #define MAX_MEMMAP_REGIONS	4
 
 static bool memmap_too_large;
+extern unsigned long get_cmd_line_ptr(void);
+
 
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
@@ -85,49 +101,14 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
 	return true;
 }
 
-/**
- *	_memparse - Parse a string with mem suffixes into a number
- *	@ptr: Where parse begins
- *	@retptr: (output) Optional pointer to next char after parse completes
- *
- *	Parses a string into a number.  The number stored at @ptr is
- *	potentially suffixed with K, M, G, T, P, E.
- */
-static unsigned long long _memparse(const char *ptr, char **retptr)
+char *skip_spaces(const char *str)
 {
-	char *endptr;	/* Local pointer to end of parsed string */
-
-	unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-	switch (*endptr) {
-	case 'E':
-	case 'e':
-		ret <<= 10;
-	case 'P':
-	case 'p':
-		ret <<= 10;
-	case 'T':
-	case 't':
-		ret <<= 10;
-	case 'G':
-	case 'g':
-		ret <<= 10;
-	case 'M':
-	case 'm':
-		ret <<= 10;
-	case 'K':
-	case 'k':
-		ret <<= 10;
-		endptr++;
-	default:
-		break;
-	}
-
-	if (retptr)
-		*retptr = endptr;
-
-	return ret;
+	while (isspace(*str))
+		++str;
+	return (char *)str;
 }
+#include "../../../../lib/ctype.c"
+#include "../../../../lib/cmdline.c"
 
 static int
 parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
@@ -142,7 +123,7 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 		return -EINVAL;
 
 	oldp = p;
-	*size = _memparse(p, &p);
+	*size = memparse(p, &p);
 	if (p == oldp)
 		return -EINVAL;
 
@@ -155,27 +136,21 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 	case '#':
 	case '$':
 	case '!':
-		*start = _memparse(p + 1, &p);
+		*start = memparse(p + 1, &p);
 		return 0;
 	}
 
 	return -EINVAL;
 }
 
-static void mem_avoid_memmap(void)
+static void mem_avoid_memmap(char *str)
 {
-	char arg[128];
+	static int i;
 	int rc;
-	int i;
-	char *str;
 
-	/* See if we have any memmap areas */
-	rc = cmdline_find_option("memmap", arg, sizeof(arg));
-	if (rc <= 0)
+	if (i >= MAX_MEMMAP_REGIONS)
 		return;
 
-	i = 0;
-	str = arg;
 	while (str && (i < MAX_MEMMAP_REGIONS)) {
 		int rc;
 		unsigned long long start, size;
@@ -202,6 +177,48 @@ static void mem_avoid_memmap(void)
 		memmap_too_large = true;
 }
 
+
+/* Macros used by the included decompressor code below. */
+#define STATIC
+#include <linux/decompress/mm.h>
+
+#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 +340,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* We don't need to set a mapping for setup_data. */
 
 	/* Mark the memmap regions we need to avoid */
-	mem_avoid_memmap();
+	handle_mem_memmap();
 
 #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 v3 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>
Acked-by: Kees Cook <keescook@chromium.org>
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 53a06ec..e5eb0c3 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -78,6 +78,10 @@ static bool memmap_too_large;
 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,
@@ -128,16 +132,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;
@@ -163,9 +173,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;
@@ -189,6 +204,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 )
@@ -211,8 +227,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);
@@ -449,7 +477,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)
@@ -463,8 +492,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) {
@@ -478,7 +514,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 v3 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..d518144 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 omitted, 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 v3 0/3] Handle memmap and mem kernel options in boot stage kaslr

[YASUAKI ISHIMATSU]

Hi Baoquan,


On 04/26/2017 06:16 AM, Baoquan He wrote:
> 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.

I confirmed that mem= works correctly.

Tested-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

Thanks,
Yasuaki Ishimatsu


> 
> *) kernel-parameters.txt doesn't tell the updated behaviour of memmap=.
> 
> This patchset tries to solve above issues, and it sits on top of
> tip:x86/boot branch.
> 
> Changelog
> 
> v2->v3:
> No functionality change in this round.
> a)Use local static variable insted of global variable
>   mem_avoid_memmap_index in patch 1/3.
> b)Fix a typo in patch 3/3.
> 
> 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.
> 
> 
> 
> 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                | 183 +++++++++++++++---------
>  arch/x86/boot/string.c                          |   8 ++
>  4 files changed, 136 insertions(+), 66 deletions(-)
> 

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

[Kees Cook]

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

Which include includes this?

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

And this?

> +
>  #include "misc.h"
>  #include "error.h"
> -#include "../boot.h"
>
>  #include <generated/compile.h>
>  #include <linux/module.h>
>  #include <linux/uts.h>
>  #include <linux/utsname.h>
> +#include <linux/ctype.h>

Why is misc.h's ctype insufficient?

>  #include <generated/utsrelease.h>
>
>  /* Simplified build-specific string for starting entropy. */
> @@ -61,6 +75,8 @@ struct mem_vector {
>  #define MAX_MEMMAP_REGIONS     4
>
>  static bool memmap_too_large;
> +extern unsigned long get_cmd_line_ptr(void);

Can we avoid an extern in .c? Seems like this should be defined in
misc.h or similar?

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

Can this be moved to the top of the file? (Also, I wonder if maybe it
should be moved into misc.h?)

> +
> +#define COMMAND_LINE_SIZE 256
> +static int handle_mem_memmap(void)
> +{
> +       char *args = (char *)get_cmd_line_ptr();
> +       size_t len = strlen((char *)args);
> +       char *tmp_cmdline;
> +       char *param, *val;
> +

Here to optimize for the common case, please do something like:

if (!strstr("memmap=", args))
    return 0;

> +       tmp_cmdline = malloc(COMMAND_LINE_SIZE);

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

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

The first "mem" in this name seems redundant.

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

Looks like it's making progress, thanks!

-Kees

-- 
Kees Cook
Pixel Security

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

[Kees Cook]

On Wed, Apr 26, 2017 at 3:16 AM, 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>
> Acked-by: Kees Cook <keescook@chromium.org>
> 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 53a06ec..e5eb0c3 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -78,6 +78,10 @@ static bool memmap_too_large;
>  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,
> @@ -128,16 +132,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;

Please explicitly add a comment like /* Fall through. */ or something
here, to help future reviewers. :)

> +       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;
> @@ -163,9 +173,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;
> @@ -189,6 +204,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 )
> @@ -211,8 +227,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);
> @@ -449,7 +477,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)
> @@ -463,8 +492,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) {
> @@ -478,7 +514,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
>

Otherwise, this looks good to me.

-Kees

-- 
Kees Cook
Pixel Security

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

[Baoquan He]

On 04/27/17 at 03:57pm, YASUAKI ISHIMATSU wrote:
> Hi Baoquan,
> 
> 
> On 04/26/2017 06:16 AM, Baoquan He wrote:
> > 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.
> 
> I confirmed that mem= works correctly.
> 
> Tested-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

Thanks for your effort, Yasuaki Ishimatsu!

> 
> 
> > 
> > *) kernel-parameters.txt doesn't tell the updated behaviour of memmap=.
> > 
> > This patchset tries to solve above issues, and it sits on top of
> > tip:x86/boot branch.
> > 
> > Changelog
> > 
> > v2->v3:
> > No functionality change in this round.
> > a)Use local static variable insted of global variable
> >   mem_avoid_memmap_index in patch 1/3.
> > b)Fix a typo in patch 3/3.
> > 
> > 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.
> > 
> > 
> > 
> > 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                | 183 +++++++++++++++---------
> >  arch/x86/boot/string.c                          |   8 ++
> >  4 files changed, 136 insertions(+), 66 deletions(-)
> > 

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

[Baoquan He]

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

Thanks for your reviewing, Kees!

misc.h includes it.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Sure, will do.

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

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

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

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

[Baoquan He]

On 04/28/17 at 12:39pm, Kees Cook wrote:
> On Wed, Apr 26, 2017 at 3:16 AM, 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>
> > Acked-by: Kees Cook <keescook@chromium.org>
> > 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 53a06ec..e5eb0c3 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -78,6 +78,10 @@ static bool memmap_too_large;
> >  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,
> > @@ -128,16 +132,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;
> 
> Please explicitly add a comment like /* Fall through. */ or something
> here, to help future reviewers. :)

Sure, will add. Thanks!

> 
> > +       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;
> > @@ -163,9 +173,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;
> > @@ -189,6 +204,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 )
> > @@ -211,8 +227,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);
> > @@ -449,7 +477,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)
> > @@ -463,8 +492,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) {
> > @@ -478,7 +514,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
> >
> 
> Otherwise, this looks good to me.
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security