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
