Messages in this thread | | | Date | Mon, 24 Apr 2017 17:09:42 +0800 | From | Baoquan He <> | Subject | Re: [PATCH v2 1/3] KASLR: Parse all memmap entries in cmdline |
| |
On 04/24/17 at 04:00pm, Dou Liyang wrote: > Hi Baoquan, > > At 04/24/2017 10:40 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. > > > [...] > > > > -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; > > Is it better that we make that variable *static* to remove the global > variable(mem_avoid_memmap_index)?
Yeah, I am fine with it. Can change lik this:
> > - int i = mem_avoid_memmap_index; > + static int i; > > if (i >= MAX_MEMMAP_REGIONS) > return; > @@ -172,7 +172,6 @@ static void mem_avoid_memmap(char *str) > 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) > > > > > > - /* 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 guess we just parsed and handled 4 MEMMAP_REGIONS and might ignore > the following in the whole cmdline.
Right > > Is it reasonable? Is there any priority? The smaller the size, the more > priority?
It's on purpose. Please see the discussion during Dave Jiang's patch posting.
commit f28442497b5c ("x86/boot: Fix KASLR and memmap= collision") > > > > - i = 0; > > - str = arg; > > while (str && (i < MAX_MEMMAP_REGIONS)) { > > int rc; > > unsigned long long start, size; > > @@ -196,12 +172,55 @@ 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; > > } > > > > + > > +/* 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 +342,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 > > > >
| |