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
> >
>
>
