All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Dave Jiang <dave.jiang@intel.com>
Cc: Kees Cook <keescook@chromium.org>,
	linux-nvdimm@lists.01.org, x86@kernel.org, david@fromorbit.com,
	linux-kernel@vger.kernel.org, mingo@redhat.com, hpa@zytor.com,
	tglx@linutronix.de
Subject: Re: [PATCH] x86: fix kaslr and memmap collision
Date: Tue, 22 Nov 2016 09:47:54 +0100	[thread overview]
Message-ID: <20161122084754.GA25596@gmail.com> (raw)
In-Reply-To: <147977413859.13657.2181994710415174471.stgit@djiang5-desk3.ch.intel.com>


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

> [...] 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@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?

Thanks,

	Ingo
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

WARNING: multiple messages have this Message-ID (diff)
From: Ingo Molnar <mingo@kernel.org>
To: Dave Jiang <dave.jiang@intel.com>
Cc: tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com,
	dan.j.williams@intel.com, x86@kernel.org, david@fromorbit.com,
	linux-kernel@vger.kernel.org, linux-nvdimm@ml01.01.org,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH] x86: fix kaslr and memmap collision
Date: Tue, 22 Nov 2016 09:47:54 +0100	[thread overview]
Message-ID: <20161122084754.GA25596@gmail.com> (raw)
In-Reply-To: <147977413859.13657.2181994710415174471.stgit@djiang5-desk3.ch.intel.com>


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

> [...] 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@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?

Thanks,

	Ingo

  reply	other threads:[~2016-11-22  8:47 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-22  0:22 [PATCH] x86: fix kaslr and memmap collision Dave Jiang
2016-11-22  0:22 ` Dave Jiang
2016-11-22  8:47 ` Ingo Molnar [this message]
2016-11-22  8:47   ` Ingo Molnar
2016-11-22 17:26   ` Dan Williams
2016-11-22 17:26     ` Dan Williams
2016-11-22 18:54     ` Kees Cook
2016-11-22 18:54       ` Kees Cook
2016-11-22 19:01       ` Dan Williams
2016-11-22 19:01         ` Dan Williams
2016-11-22 22:37         ` Kees Cook
2016-11-22 22:37           ` Kees Cook
2016-11-24  0:04         ` Dave Chinner
2016-11-24  0:04           ` Dave Chinner
2016-11-24 19:30           ` Dan Williams
2016-11-24 19:30             ` Dan Williams
2017-01-03  8:31     ` Baoquan He
2017-01-03  8:31       ` Baoquan He
2017-01-03 16:27       ` Ross Zwisler
2017-01-03 16:27         ` Ross Zwisler
2017-01-03 18:24       ` Dan Williams
2017-01-03 18:24         ` Dan Williams
2017-01-03 20:15         ` Dave Jiang
2017-01-03 20:15           ` Dave Jiang
2017-01-04  1:57           ` Baoquan He
2017-01-04  1:57             ` Baoquan He

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161122084754.GA25596@gmail.com \
    --to=mingo@kernel.org \
    --cc=dave.jiang@intel.com \
    --cc=david@fromorbit.com \
    --cc=hpa@zytor.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.