From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S642476AbdD1Thi (ORCPT ); Fri, 28 Apr 2017 15:37:38 -0400 Received: from mail-it0-f53.google.com ([209.85.214.53]:37343 "EHLO mail-it0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1425598AbdD1Thc (ORCPT ); Fri, 28 Apr 2017 15:37:32 -0400 MIME-Version: 1.0 In-Reply-To: <1493201772-19084-2-git-send-email-bhe@redhat.com> References: <1493201772-19084-1-git-send-email-bhe@redhat.com> <1493201772-19084-2-git-send-email-bhe@redhat.com> From: Kees Cook Date: Fri, 28 Apr 2017 12:37:30 -0700 X-Google-Sender-Auth: bXAMBbPab7Og5Vz7wLr1McD-yXY Message-ID: Subject: Re: [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline To: Baoquan He Cc: Ingo Molnar , LKML , Dave Young , douly.fnst@cn.fujitsu.com, Dan Williams , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , "x86@kernel.org" , Yinghai Lu , Borislav Petkov Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 26, 2017 at 3:16 AM, Baoquan He wrote: > In commit: > > f28442497b5c ("x86/boot: Fix KASLR and memmap= collision") > > ... the memmap= option is parsed so that KASLR can avoid those reserved > regions. It uses cmdline_find_option() to get the value if memmap= > is specified, however the problem is that cmdline_find_option() can only > find the last entry if multiple memmap entries are provided. This > is not correct. > > 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: x86@kernel.org > Cc: Kees Cook > Cc: Yinghai Lu > Cc: Borislav Petkov > --- > 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 > #include > #include > #include > +#include Why is misc.h's ctype insufficient? > #include > > /* 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 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