From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S376409AbdD2KM4 (ORCPT ); Sat, 29 Apr 2017 06:12:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34530 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756454AbdD2KMt (ORCPT ); Sat, 29 Apr 2017 06:12:49 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2D68A64080 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=bhe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 2D68A64080 Date: Sat, 29 Apr 2017 18:12:41 +0800 From: Baoquan He To: Kees Cook 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 Subject: Re: [PATCH v3 1/3] KASLR: Parse all memmap entries in cmdline Message-ID: <20170429101241.GB12873@x1> References: <1493201772-19084-1-git-send-email-bhe@redhat.com> <1493201772-19084-2-git-send-email-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.0 (2016-08-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Sat, 29 Apr 2017 10:12:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/28/17 at 12:37pm, Kees Cook wrote: > 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? Thanks for your reviewing, Kees! misc.h includes it. Function isdigit is implemented in boot/ctype.h. And with , 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 > > #include > > #include > > #include > > +#include > > 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 > > > > /* 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 > > 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