[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20161122084754.GA25596@gmail.com>
Date: Tue, 22 Nov 2016 09:47:54 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Dave Jiang <dave.jiang@...el.com>
Cc: tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
dan.j.williams@...el.com, x86@...nel.org, david@...morbit.com,
linux-kernel@...r.kernel.org, linux-nvdimm@...ts.01.org,
Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] x86: fix kaslr and memmap collision
* 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?
> [...] Check has been added in the kaslr in order to avoid the region marked by
> memmap.
What does this mean?
> 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?
Thanks,
Ingo
Powered by blists - more mailing lists