Re: [PATCH v2 1/3] KASLR: Parse all memmap entries in cmdline

[Dou Liyang]

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

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

Is it reasonable?  Is there any priority? The smaller the size, the more 
priority?

Thanks,
        Liyang

-       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, &param, &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