Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline
On 04/18/17 at 04:32pm, Kees Cook wrote: > On Tue, Apr 18, 2017 at 3:52 PM, Baoquan He wrote: > > On 04/18/17 at 01:22pm, Kees Cook wrote: > >> > +#define COMMAND_LINE_SIZE 256 > >> > +static int handle_mem_memmap(void) > >> > +{ > >> > + char *args = (char *)get_cmd_line_ptr(); > >> > + char tmp_cmdline[COMMAND_LINE_SIZE]; > >> > >> Can't this use a dynamic allocation instead of the 256 limit? > > > > This is in boot/compressed code, no mm allocator built yet? Am I right? > > misc.c uses malloc for phdrs, and the boot_heap is create to build an > area for those calls, see include/linux/decompress/mm.h. I *think* it > should be safe to use malloc here. It should be a pretty small > allocation normally. Yes, didn't notice this. Will use it to do dynamic malloc. Thanks for telling!
Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline
On Tue, Apr 18, 2017 at 3:52 PM, Baoquan He wrote: > On 04/18/17 at 01:22pm, Kees Cook wrote: >> > +#define COMMAND_LINE_SIZE 256 >> > +static int handle_mem_memmap(void) >> > +{ >> > + char *args = (char *)get_cmd_line_ptr(); >> > + char tmp_cmdline[COMMAND_LINE_SIZE]; >> >> Can't this use a dynamic allocation instead of the 256 limit? > > This is in boot/compressed code, no mm allocator built yet? Am I right? misc.c uses malloc for phdrs, and the boot_heap is create to build an area for those calls, see include/linux/decompress/mm.h. I *think* it should be safe to use malloc here. It should be a pretty small allocation normally. -Kees -- Kees Cook Pixel Security
Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline
Hi Kees, Thanks for your reviewing! On 04/18/17 at 01:22pm, Kees Cook wrote: > > static int > > parse_memmap(char *p, unsigned long long *start, unsigned long long *size) > > @@ -142,40 +112,33 @@ 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; > > > > switch (*p) { > > case '@': > > /* Skip this region, usable */ > > - *start = 0; > > *size = 0; > > - return 0; > > + *start = 0; > > Is this intentionally falling through? If so, why assign *start at all? OOPS, this is a mistake when I split patch. Here it should not be changed in this patch though code change is OK with patch 3/4 together. Will change that. > > > case '#': > > case '$': > > case '!': > > - *start = _memparse(p + 1, &p); > > + *start = memparse(p + 1, &p); > > return 0; > > } > > > > return -EINVAL; > > } > > > > -static void mem_avoid_memmap(void) > > +static void mem_avoid_memmap(char *str) > > { > > - char arg[128]; > > int rc; > > - int i; > > - char *str; > > + int i = mem_avoid_memmap_index; > > > > - /* See if we have any memmap areas */ > > - rc = cmdline_find_option("memmap", arg, sizeof(arg)); > > - if (rc <= 0) > > + if (i >= MAX_MEMMAP_REGIONS) > > return; > > > > - i = 0; > > - str = arg; > > while (str && (i < MAX_MEMMAP_REGIONS)) { > > int rc; > > unsigned long long start, size; > > @@ -196,12 +159,49 @@ static void mem_avoid_memmap(void) > > mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size; > > i++; > > } > > + mem_avoid_memmap_index = i; > > > > /* More than 4 memmaps, fail kaslr */ > > if ((i >= MAX_MEMMAP_REGIONS) && str) > > memmap_too_large = true; > > } > > > > +#define COMMAND_LINE_SIZE 256 > > +static int handle_mem_memmap(void) > > +{ > > + char *args = (char *)get_cmd_line_ptr(); > > + char tmp_cmdline[COMMAND_LINE_SIZE]; > > Can't this use a dynamic allocation instead of the 256 limit? This is in boot/compressed code, no mm allocator built yet? Am I right? > > > + size_t len = strlen((char *)args); > > + char *param, *val; > > + > > + 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) { > > + int ret; > > + > > + debug_putstr(args); > > + debug_putstr("\n"); > > Are these accidentally left over? Yes, it's for debugging. Will remove. Thanks Baoquan > > > + > > + args = next_arg(args, ¶m, &val); > > + /* Stop at -- */ > > + if (!val && strcmp(param, "--") == 0) { > > + warn("Only '--' specified in cmdline"); > > + return -1; > > + } > > + > > + if (!strcmp(param, "memmap")) > > + mem_avoid_memmap(val); > > + } > > + > > + 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 +323,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 > > > > Otherwise, yeah, this looks sensible. > > -Kees > > -- > Kees Cook > Pixel Security
Re: [PATCH 2/4] KASLR: Parse all memmap entries in cmdline
On Mon, Apr 17, 2017 at 6:34 AM, Baoquan He wrote: > In commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision"), > 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 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 > Cc: "H. Peter Anvin" > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: x...@kernel.org > Cc: Kees Cook > Cc: Baoquan He > Cc: Yinghai Lu > Cc: Borislav Petkov > --- > arch/x86/boot/compressed/cmdline.c | 2 +- > arch/x86/boot/compressed/kaslr.c | 112 > ++--- > arch/x86/boot/string.c | 8 +++ > 3 files changed, 65 insertions(+), 57 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 8b7c9e7..36ab429 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -9,14 +9,16 @@ > * contain the entire properly aligned running kernel image. > * > */ > + > +#define BOOT_CTYPE_H > #include "misc.h" > #include "error.h" > -#include "../boot.h" > > #include > #include > #include > #include > +#include > #include > > /* Simplified build-specific string for starting entropy. */ > @@ -61,6 +63,9 @@ struct mem_vector { > #define MAX_MEMMAP_REGIONS 4 > > static bool memmap_too_large; > +int mem_avoid_memmap_index; > +extern unsigned long get_cmd_line_ptr(void); > + > > enum mem_avoid_index { > MEM_AVOID_ZO_RANGE = 0, > @@ -85,49 +90,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,40 +112,33 @@ 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; > > switch (*p) { > case '@': > /* Skip this region, usable */ > - *start = 0; > *size = 0; > - return 0; > + *start = 0; Is this intentionally falling through? If so, why assign *start at all? > case '#': > case '$': > case '!': > - *start = _memparse(p + 1, &p); > + *start = memparse(p + 1, &p); > return 0; > } > > return -EINVAL; > } > > -static void mem_avoid_memmap(void) > +static void mem_avoid_memmap(char *str) > { > - char arg[128]; > int rc; > - int i; > - char *str; > + int i = mem_avoid_memmap_index; > > - /* See if we have any memmap areas */ > - rc = cmdline_find_option("memmap", arg, sizeof(arg)); > -