Message-ID: <CAGXu5jKwJaqXcmkB9Tt-sXOYhrixSRmFWA7r4QxKp10qCfCyuA@mail.gmail.com>
Date:   Tue, 22 Nov 2016 10:54:29 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Ingo Molnar <mingo@...nel.org>, Dave Jiang <dave.jiang@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
        david <david@...morbit.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>
Subject: Re: [PATCH] x86: fix kaslr and memmap collision

On Tue, Nov 22, 2016 at 9:26 AM, Dan Williams <dan.j.williams@...el.com> wrote:
> [ replying for Dave since he's offline today and tomorrow ]
>
> On Tue, Nov 22, 2016 at 12:47 AM, Ingo Molnar <mingo@...nel.org> wrote:
>>
>> * Dave Jiang <dave.jiang@...el.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@...el.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?

Yeah, that seems fine to me. I assume it's rare to have 4?

-Kees

-- 
Kees Cook
Nexus Security

Powered by blists - more mailing lists