Messages in this thread | | | Date | Wed, 19 Apr 2017 06:52:22 +0800 | From | Baoquan He <> | Subject | 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
| |