Re: [PATCH] x86: fix kaslr and memmap collision

[Dan Williams <dan.j.williams@intel.com>]

[ replying for Dave since he's offline today and tomorrow ]

On Tue, Nov 22, 2016 at 12:47 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dave Jiang <dave.jiang@intel.com> wrote:
>
>> CONFIG_RANDOMIZE_BASE relocates the kernel to a random base address.
>> However it does not take into account the memmap= parameter passed in from
>> the kernel commandline.
>
> memmap= parameters are often used as a list.
>
>> [...] This results in the kernel sometimes being put in the middle of the user
>> memmap. [...]
>
> What does this mean? If memmap= is used to re-define the memory map then the
> kernel getting in the middle of a RAM area is what we want, isn't it? What we
> don't want is for the kernel to get into reserved areas, right?

Right, this is about teaching kaslr to not land the kernel in newly
defined reserved regions that were not marked reserved in the initial
e820 map from platform firmware.

>> [...] Check has been added in the kaslr in order to avoid the region marked by
>> memmap.
>
> What does this mean?

Is this clearer? "Update the set of 'mem_avoid' entries to exclude
'memmap=' defined reserved regions from the set of valid address range
to land the kernel image."

>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  arch/x86/boot/boot.h             |    2 ++
>>  arch/x86/boot/compressed/kaslr.c |   45 ++++++++++++++++++++++++++++++++++++++
>>  arch/x86/boot/string.c           |   25 +++++++++++++++++++++
>>  3 files changed, 72 insertions(+)
>>
>> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
>> index e5612f3..0d5fe5b 100644
>> --- a/arch/x86/boot/boot.h
>> +++ b/arch/x86/boot/boot.h
>> @@ -332,6 +332,8 @@ int strncmp(const char *cs, const char *ct, size_t count);
>>  size_t strnlen(const char *s, size_t maxlen);
>>  unsigned int atou(const char *s);
>>  unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base);
>> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
>> +long simple_strtol(const char *cp, char **endp, unsigned int base);
>>  size_t strlen(const char *s);
>>
>>  /* tty.c */
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index a66854d..6fb8f1ec 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -11,6 +11,7 @@
>>   */
>>  #include "misc.h"
>>  #include "error.h"
>> +#include "../boot.h"
>>
>>  #include <generated/compile.h>
>>  #include <linux/module.h>
>> @@ -61,6 +62,7 @@ enum mem_avoid_index {
>>       MEM_AVOID_INITRD,
>>       MEM_AVOID_CMDLINE,
>>       MEM_AVOID_BOOTPARAMS,
>> +     MEM_AVOID_MEMMAP,
>>       MEM_AVOID_MAX,
>>  };
>>
>> @@ -77,6 +79,37 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>>       return true;
>>  }
>>
>> +#include "../../../../lib/cmdline.c"
>> +
>> +static int
>> +parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>> +{
>> +     char *oldp;
>> +
>> +     if (!p)
>> +             return -EINVAL;
>> +
>> +     /* we don't care about this option here */
>> +     if (!strncmp(p, "exactmap", 8))
>> +             return -EINVAL;
>> +
>> +     oldp = p;
>> +     *size = memparse(p, &p);
>> +     if (p == oldp)
>> +             return -EINVAL;
>> +
>> +     switch (*p) {
>> +     case '@':
>> +     case '#':
>> +     case '$':
>> +     case '!':
>> +             *start = memparse(p+1, &p);
>> +             return 0;
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>>  /*
>>   * 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
>> @@ -158,6 +191,8 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>       u64 initrd_start, initrd_size;
>>       u64 cmd_line, cmd_line_size;
>>       char *ptr;
>> +     char arg[38];
>
> Where does the magic '38' come from?
>
>> +     unsigned long long memmap_start, memmap_size;
>>
>>       /*
>>        * Avoid the region that is unsafe to overlap during
>> @@ -195,6 +230,16 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>       add_identity_map(mem_avoid[MEM_AVOID_BOOTPARAMS].start,
>>                        mem_avoid[MEM_AVOID_BOOTPARAMS].size);
>>
>> +     /* see if we have any memmap areas */
>> +     if (cmdline_find_option("memmap", arg, sizeof(arg)) > 0) {
>> +             int rc = parse_memmap(arg, &memmap_start, &memmap_size);
>> +
>> +             if (!rc) {
>> +                     mem_avoid[MEM_AVOID_MEMMAP].start = memmap_start;
>> +                     mem_avoid[MEM_AVOID_MEMMAP].size = memmap_size;
>> +             }
>> +     }
>> +
>
> This only handles a single (first) memmap argument, is that sufficient?

No, you're right, we need to handle multiple ranges.  Since the
mem_avoid array is statically allocated perhaps we can handle up to 4
memmap= entries, but past that point disable kaslr for that boot?