* [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr @ 2017-04-26 10:16 Baoquan He 2017-04-26 10:16 ` [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline Baoquan He ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Baoquan He @ 2017-04-26 10:16 UTC (permalink / raw) To: mingo, linux-kernel Cc: keescook, dyoung, douly.fnst, dan.j.williams, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline 2017-04-26 10:16 [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr Baoquan He @ 2017-04-26 10:16 ` Baoquan He 2017-04-28 19:37 ` Kees Cook 2017-04-26 10:16 ` [PATCH v3 2/3] KASLR: Handle memory limit specified by memmap and mem option Baoquan He ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Baoquan He @ 2017-04-26 10:16 UTC (permalink / raw) To: mingo, linux-kernel Cc: keescook, dyoung, douly.fnst, dan.j.williams, Baoquan He, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, Yinghai Lu, Borislav Petkov 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, ¶m, &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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline 2017-04-26 10:16 ` [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline Baoquan He @ 2017-04-28 19:37 ` Kees Cook 2017-04-29 10:12 ` Baoquan He 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2017-04-28 19:37 UTC (permalink / raw) To: Baoquan He Cc: Ingo Molnar, LKML, Dave Young, douly.fnst, Dan Williams, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, Yinghai Lu, Borislav Petkov 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, ¶m, &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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline 2017-04-28 19:37 ` Kees Cook @ 2017-04-29 10:12 ` Baoquan He 0 siblings, 0 replies; 10+ messages in thread From: Baoquan He @ 2017-04-29 10:12 UTC (permalink / raw) To: Kees Cook Cc: Ingo Molnar, LKML, Dave Young, douly.fnst, Dan Williams, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, Yinghai Lu, Borislav Petkov 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, ¶m, &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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] KASLR: Handle memory limit specified by memmap and mem option 2017-04-26 10:16 [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr Baoquan He 2017-04-26 10:16 ` [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline Baoquan He @ 2017-04-26 10:16 ` Baoquan He 2017-04-28 19:39 ` Kees Cook 2017-04-26 10:16 ` [PATCH v3 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description Baoquan He 2017-04-27 19:57 ` [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr YASUAKI ISHIMATSU 3 siblings, 1 reply; 10+ messages in thread From: Baoquan He @ 2017-04-26 10:16 UTC (permalink / raw) To: mingo, linux-kernel Cc: keescook, dyoung, douly.fnst, dan.j.williams, Baoquan He, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, Yinghai Lu, Borislav Petkov 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/3] KASLR: Handle memory limit specified by memmap and mem option 2017-04-26 10:16 ` [PATCH v3 2/3] KASLR: Handle memory limit specified by memmap and mem option Baoquan He @ 2017-04-28 19:39 ` Kees Cook 2017-04-29 10:14 ` Baoquan He 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2017-04-28 19:39 UTC (permalink / raw) To: Baoquan He Cc: Ingo Molnar, LKML, Dave Young, douly.fnst, Dan Williams, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, Yinghai Lu, Borislav Petkov 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/3] KASLR: Handle memory limit specified by memmap and mem option 2017-04-28 19:39 ` Kees Cook @ 2017-04-29 10:14 ` Baoquan He 0 siblings, 0 replies; 10+ messages in thread From: Baoquan He @ 2017-04-29 10:14 UTC (permalink / raw) To: Kees Cook Cc: Ingo Molnar, LKML, Dave Young, douly.fnst, Dan Williams, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, Yinghai Lu, Borislav Petkov 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description 2017-04-26 10:16 [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr Baoquan He 2017-04-26 10:16 ` [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline Baoquan He 2017-04-26 10:16 ` [PATCH v3 2/3] KASLR: Handle memory limit specified by memmap and mem option Baoquan He @ 2017-04-26 10:16 ` Baoquan He 2017-04-27 19:57 ` [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr YASUAKI ISHIMATSU 3 siblings, 0 replies; 10+ messages in thread From: Baoquan He @ 2017-04-26 10:16 UTC (permalink / raw) To: mingo, linux-kernel Cc: keescook, dyoung, douly.fnst, dan.j.williams, Baoquan He, Jonathan Corbet, Rafael J. Wysocki, Andrew Morton, Greg Kroah-Hartman, Bjorn Helgaas, Mauro Carvalho Chehab, linux-doc 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr 2017-04-26 10:16 [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr Baoquan He ` (2 preceding siblings ...) 2017-04-26 10:16 ` [PATCH v3 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description Baoquan He @ 2017-04-27 19:57 ` YASUAKI ISHIMATSU 2017-04-28 23:02 ` Baoquan He 3 siblings, 1 reply; 10+ messages in thread From: YASUAKI ISHIMATSU @ 2017-04-27 19:57 UTC (permalink / raw) To: Baoquan He, mingo, linux-kernel Cc: keescook, dyoung, douly.fnst, dan.j.williams 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(-) > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr 2017-04-27 19:57 ` [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr YASUAKI ISHIMATSU @ 2017-04-28 23:02 ` Baoquan He 0 siblings, 0 replies; 10+ messages in thread From: Baoquan He @ 2017-04-28 23:02 UTC (permalink / raw) To: YASUAKI ISHIMATSU Cc: mingo, linux-kernel, keescook, dyoung, douly.fnst, dan.j.williams 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(-) > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-04-29 10:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-26 10:16 [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr Baoquan He 2017-04-26 10:16 ` [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline Baoquan He 2017-04-28 19:37 ` Kees Cook 2017-04-29 10:12 ` Baoquan He 2017-04-26 10:16 ` [PATCH v3 2/3] KASLR: Handle memory limit specified by memmap and mem option Baoquan He 2017-04-28 19:39 ` Kees Cook 2017-04-29 10:14 ` Baoquan He 2017-04-26 10:16 ` [PATCH v3 3/3] Documentation/kernel-parameters.txt: Update 'memmap=' option description Baoquan He 2017-04-27 19:57 ` [PATCH v3 0/3] Handle memmap and mem kernel options in boot stage kaslr YASUAKI ISHIMATSU 2017-04-28 23:02 ` Baoquan He
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).