[PATCH 0/4] fix the compatibility between kaslr and kexe

[PATCH 0/4] fix the compatibility between kaslr and kexe

[Baoquan He]

Earlier it's reported kexec can't load kernel on system with over 4G
memory and kdump didn't work when set crashkenrel=xx, high when kaslr
is compiled. They didn't work though nokaslr is set in cmdline as long
as it's compiled in. This is caused by not checking if the kernel
decompression output region is in a legal region.

In this patchset, fixs the bug peopel reported that kexec/kdump didn't
work when kernel loading addr is above 1G. This is done in patch 2/4.

When kernel is put in a address which is not LOAD_PHYSICAL_ADDR, and
kaslr is compiled in, it will do the relocation handling though user
set nokaslr in cmdline. This is because no config checking in 2nd part
of kaslr process, namely handle_relocations(). This is fixed in patch
1/4.

Patch 3/4 is handling the setup data avoiding. Since setup data can
be put anywhere, if it's in below 1G region, need be avoided to be
the kaslr random relocation slot. I just tested it using a kexec-tools
user space trick, change kexec-tools to make the buffer allocating
from down to top, and set E820MAX to 10, then extra e820 regions have
to be added into setup data.

Patch 4/4 is to export KERNEL_IMAGE_SIZE to VMCOREINFO, makedumpfile
need this to calculate MODULES_VADDR. Since introduing kaslr, the
MODULES_VADDR is not fixed.

Baoquan He (3):
  kaslr: check user's config too when handle relocations
  kaslr: check if the random addr is available
  export the kernel image size KERNEL_IMAGE_SIZE

Dave Young (1):
  kaslr setup_data handling

 arch/x86/boot/compressed/aslr.c | 31 +++++++++++++++++++++++++++++--
 arch/x86/boot/compressed/misc.c | 17 +++++++++++++++++
 kernel/kexec.c                  |  3 +++
 3 files changed, 49 insertions(+), 2 deletions(-)

-- 
1.8.5.3

[PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

kaslr's action is splitted into 2 parts. The 1st is getting available memory
slots and randomly choose the kernel relocation address. After decompression
of kernel to the chosen place, the 2nd part begin to check if kaslr has got
a relocation address, and will do the relocations handling if yes.

However in current implementation, in the 2nd part, it doesn't check user's
config, just compare decompression output address and the LOAD_PHYSICAL_ADDR
where kernel was compiled to run. If they are equal, it means a kaslr is
taking action, and need do the relocation handling. This truly works when
bootloader always load kernel to LOAD_PHYSICAL_ADDR. But this doesn't always
happens. Kexec/kdump kernel loading is exceptional. Kdump/kexec can load
kernel anywhere, this is not fixed. So in this case, it will do the relocation
handling though user clearly set nokaslr in cmdline. This is not correct.

So in this patch, check user's config too in the 2nd part, namely in
handle_relocations().

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/aslr.c |  2 ++
 arch/x86/boot/compressed/misc.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
index fc6091a..975b07b 100644
--- a/arch/x86/boot/compressed/aslr.c
+++ b/arch/x86/boot/compressed/aslr.c
@@ -292,11 +292,13 @@ unsigned char *choose_kernel_location(unsigned char *input,
 #ifdef CONFIG_HIBERNATION
 	if (!cmdline_find_option_bool("kaslr")) {
 		debug_putstr("KASLR disabled by default...\n");
+		debug_putstr("No need to choose kernel relocation...\n");
 		goto out;
 	}
 #else
 	if (cmdline_find_option_bool("nokaslr")) {
 		debug_putstr("KASLR disabled by cmdline...\n");
+		debug_putstr("No need to choose kernel relocation...\n");
 		goto out;
 	}
 #endif
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 57ab74d..7780a5b 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -238,6 +238,18 @@ static void handle_relocations(void *output, unsigned long output_len)
 	unsigned long min_addr = (unsigned long)output;
 	unsigned long max_addr = min_addr + output_len;
 
+#ifdef CONFIG_HIBERNATION
+	if (!cmdline_find_option_bool("kaslr")) {
+		debug_putstr("No relocation needed... ");
+		return;
+	}
+#else
+	if (cmdline_find_option_bool("nokaslr")) {
+		debug_putstr("No relocation needed... ");
+		return;
+	}
+#endif
+
 	/*
 	 * Calculate the delta between where vmlinux was linked to load
 	 * and where it was actually loaded.
-- 
1.8.5.3

[PATCH 2/4] kaslr: check if the random addr is available

[Baoquan He]

Currently kaslr enabling can extend the kernel virtual address space
to 1G, next is for modules. So if kernel is loaded to above 1G, system
running will be exceptional, This happened when kexec/kdump load kernel.

So add a check to see if the decompression output region is contained
in 1G.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/misc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 7780a5b..d2a0eaa 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -250,6 +250,11 @@ static void handle_relocations(void *output, unsigned long output_len)
 	}
 #endif
 
+	if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
+                debug_putstr("Random addr is not allowed. No relocation needed... \n");
+                return;
+        }
+
 	/*
 	 * Calculate the delta between where vmlinux was linked to load
 	 * and where it was actually loaded.
-- 
1.8.5.3

[PATCH 3/4] kaslr setup_data handling

[Baoquan He]

From: Dave Young <dyoung@redhat.com>

X86 will pass setup_data region while necessary, these regions could be
overwitten by kernel due to kaslr.

Thus iterate and add setup regions to mem_avoid[] in this patch.
Up to now there isn't a official data to state the maximal entries
setup data could use. So just set max mem avoid entries 32, hopefully
it will be enough. This can be increased later when people report
they are using more setup data entries.

Signed-off-by: Dave Young <dyoung@redhat.com>
Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/aslr.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
index 975b07b..7e92fc8 100644
--- a/arch/x86/boot/compressed/aslr.c
+++ b/arch/x86/boot/compressed/aslr.c
@@ -110,8 +110,9 @@ struct mem_vector {
 	unsigned long size;
 };
 
-#define MEM_AVOID_MAX 5
+#define MEM_AVOID_MAX 32
 static struct mem_vector mem_avoid[MEM_AVOID_MAX];
+static int mem_avoid_nr;
 
 static bool mem_contains(struct mem_vector *region, struct mem_vector *item)
 {
@@ -135,6 +136,27 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
 	return true;
 }
 
+static void mem_avoid_setup_data(void)
+{
+	struct setup_data *data;
+	u64 pa_data;
+
+	pa_data = real_mode->hdr.setup_data;
+	while (pa_data) {
+		if (mem_avoid_nr >= MEM_AVOID_MAX) {
+			debug_putstr("KASLR: too many setup_data ranges.\n");
+			return;
+		}
+		data = (struct setup_data *)pa_data;
+		if (pa_data < CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
+			mem_avoid[mem_avoid_nr].start = pa_data;
+			mem_avoid[mem_avoid_nr].size = sizeof(*data) + data->len;
+			mem_avoid_nr++;
+		}
+		pa_data = data->next;
+	}
+}
+
 static void mem_avoid_init(unsigned long input, unsigned long input_size,
 			   unsigned long output, unsigned long output_size)
 {
@@ -177,6 +199,9 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* Avoid stack memory. */
 	mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
 	mem_avoid[4].size = BOOT_STACK_SIZE;
+	mem_avoid_nr = 5;
+
+	mem_avoid_setup_data();
 }
 
 /* Does this memory vector overlap a known avoided area? */
@@ -184,7 +209,7 @@ static bool mem_avoid_overlap(struct mem_vector *img)
 {
 	int i;
 
-	for (i = 0; i < MEM_AVOID_MAX; i++) {
+	for (i = 0; i < mem_avoid_nr; i++) {
 		if (mem_overlaps(img, &mem_avoid[i]))
 			return true;
 	}
-- 
1.8.5.3

[PATCH 4/4] export the kernel image size KERNEL_IMAGE_SIZE

[Baoquan He]

Now kaslr makes kernel image size changable, not the fixed size 512M.
So KERNEL_IMAGE_SIZE need be exported to VMCOREINFO, otherwise makedumfile
will crash.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 kernel/kexec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 2bee072..bd680d3 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -2003,6 +2003,9 @@ static int __init crash_save_vmcoreinfo_init(void)
 #endif
 	VMCOREINFO_NUMBER(PG_head_mask);
 	VMCOREINFO_NUMBER(PAGE_BUDDY_MAPCOUNT_VALUE);
+#ifdef CONFIG_X86
+       VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
+#endif
 #ifdef CONFIG_HUGETLBFS
 	VMCOREINFO_SYMBOL(free_huge_page);
 #endif
-- 
1.8.5.3

Re: [PATCH 4/4] export the kernel image size KERNEL_IMAGE_SIZE

[Kees Cook]

On Fri, Sep 5, 2014 at 7:08 AM, Baoquan He <bhe@redhat.com> wrote:
> Now kaslr makes kernel image size changable, not the fixed size 512M.
> So KERNEL_IMAGE_SIZE need be exported to VMCOREINFO, otherwise makedumfile
> will crash.

Seems like a good idea, yes.

>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  kernel/kexec.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 2bee072..bd680d3 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -2003,6 +2003,9 @@ static int __init crash_save_vmcoreinfo_init(void)
>  #endif
>         VMCOREINFO_NUMBER(PG_head_mask);
>         VMCOREINFO_NUMBER(PAGE_BUDDY_MAPCOUNT_VALUE);
> +#ifdef CONFIG_X86
> +       VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
> +#endif
>  #ifdef CONFIG_HUGETLBFS
>         VMCOREINFO_SYMBOL(free_huge_page);
>  #endif

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Kees Cook]

On Fri, Sep 5, 2014 at 7:08 AM, Baoquan He <bhe@redhat.com> wrote:
> kaslr's action is splitted into 2 parts. The 1st is getting available memory
> slots and randomly choose the kernel relocation address. After decompression
> of kernel to the chosen place, the 2nd part begin to check if kaslr has got
> a relocation address, and will do the relocations handling if yes.
>
> However in current implementation, in the 2nd part, it doesn't check user's
> config, just compare decompression output address and the LOAD_PHYSICAL_ADDR
> where kernel was compiled to run. If they are equal, it means a kaslr is
> taking action, and need do the relocation handling. This truly works when
> bootloader always load kernel to LOAD_PHYSICAL_ADDR. But this doesn't always
> happens. Kexec/kdump kernel loading is exceptional. Kdump/kexec can load
> kernel anywhere, this is not fixed. So in this case, it will do the relocation
> handling though user clearly set nokaslr in cmdline. This is not correct.
>
> So in this patch, check user's config too in the 2nd part, namely in
> handle_relocations().
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/aslr.c |  2 ++
>  arch/x86/boot/compressed/misc.c | 12 ++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> index fc6091a..975b07b 100644
> --- a/arch/x86/boot/compressed/aslr.c
> +++ b/arch/x86/boot/compressed/aslr.c
> @@ -292,11 +292,13 @@ unsigned char *choose_kernel_location(unsigned char *input,
>  #ifdef CONFIG_HIBERNATION
>         if (!cmdline_find_option_bool("kaslr")) {
>                 debug_putstr("KASLR disabled by default...\n");
> +               debug_putstr("No need to choose kernel relocation...\n");
>                 goto out;
>         }
>  #else
>         if (cmdline_find_option_bool("nokaslr")) {
>                 debug_putstr("KASLR disabled by cmdline...\n");
> +               debug_putstr("No need to choose kernel relocation...\n");
>                 goto out;
>         }
>  #endif

NAK on this part -- the whole point is to report the results of the
expected KASLR state based on build config, cmdline, etc.

> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 57ab74d..7780a5b 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -238,6 +238,18 @@ static void handle_relocations(void *output, unsigned long output_len)
>         unsigned long min_addr = (unsigned long)output;
>         unsigned long max_addr = min_addr + output_len;
>
> +#ifdef CONFIG_HIBERNATION
> +       if (!cmdline_find_option_bool("kaslr")) {
> +               debug_putstr("No relocation needed... ");
> +               return;
> +       }
> +#else
> +       if (cmdline_find_option_bool("nokaslr")) {
> +               debug_putstr("No relocation needed... ");
> +               return;
> +       }
> +#endif
> +

I don't think this is correct. If you look at a02150610776 ("x86,
relocs: Move ELF relocation handling to C"), we always did relocations
on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail
badly on 32-bit. 64-bit only needs relocation when
CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be
tested here instead. I think a better option would be, in
decompress_kernel(), to compare output before and after
choose_kernel_location(). If it's the same on 64-bit,
handle_relocations() can be skipped. (Perhaps pass the before/after to
handle_relocations() and it can perform the logic.)

-Kees

>         /*
>          * Calculate the delta between where vmlinux was linked to load
>          * and where it was actually loaded.
> --
> 1.8.5.3
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 2/4] kaslr: check if the random addr is available

[Kees Cook]

On Fri, Sep 5, 2014 at 7:08 AM, Baoquan He <bhe@redhat.com> wrote:
> Currently kaslr enabling can extend the kernel virtual address space
> to 1G, next is for modules. So if kernel is loaded to above 1G, system
> running will be exceptional, This happened when kexec/kdump load kernel.
>
> So add a check to see if the decompression output region is contained
> in 1G.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/misc.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 7780a5b..d2a0eaa 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -250,6 +250,11 @@ static void handle_relocations(void *output, unsigned long output_len)
>         }
>  #endif
>
> +       if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
> +                debug_putstr("Random addr is not allowed. No relocation needed... \n");
> +                return;
> +        }
> +

It's not clear to me what this is fixing. In aslr.c,
process_e820_entry() should already make it impossible to select
max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET. If you're trying to
detect a non-kaslr boot, I think this is better handled in 1/4 where I
suggest examining the "output" location before/after
choose_kernel_location.

-Kees

>         /*
>          * Calculate the delta between where vmlinux was linked to load
>          * and where it was actually loaded.
> --
> 1.8.5.3
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 3/4] kaslr setup_data handling

[Kees Cook]

On Fri, Sep 05, 2014 at 10:08:16PM +0800, Baoquan He wrote:
> From: Dave Young <dyoung@redhat.com>
> 
> X86 will pass setup_data region while necessary, these regions could be
> overwitten by kernel due to kaslr.
> 
> Thus iterate and add setup regions to mem_avoid[] in this patch.
> Up to now there isn't a official data to state the maximal entries
> setup data could use. So just set max mem avoid entries 32, hopefully
> it will be enough. This can be increased later when people report
> they are using more setup data entries.

Ew, yes, this is bad. I hadn't seen setup_data while designing the
mem_avoid stuff. I don't like the fixed 32 entry size here, so let me
consider some options. I think the mem_avoid logic can just walk the
setup_data list itself, since that's what it's for. :)

Does only kexec use this? I assume other boot loaders must be using this
too. Is there an easy test case for validating this is fixed?

> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/aslr.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> index 975b07b..7e92fc8 100644
> --- a/arch/x86/boot/compressed/aslr.c
> +++ b/arch/x86/boot/compressed/aslr.c
> @@ -110,8 +110,9 @@ struct mem_vector {
>  	unsigned long size;
>  };
>  
> -#define MEM_AVOID_MAX 5
> +#define MEM_AVOID_MAX 32
>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
> +static int mem_avoid_nr;
>  
>  static bool mem_contains(struct mem_vector *region, struct mem_vector *item)
>  {
> @@ -135,6 +136,27 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>  	return true;
>  }
>  
> +static void mem_avoid_setup_data(void)
> +{
> +	struct setup_data *data;
> +	u64 pa_data;
> +
> +	pa_data = real_mode->hdr.setup_data;
> +	while (pa_data) {
> +		if (mem_avoid_nr >= MEM_AVOID_MAX) {
> +			debug_putstr("KASLR: too many setup_data ranges.\n");
> +			return;
> +		}
> +		data = (struct setup_data *)pa_data;
> +		if (pa_data < CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
> +			mem_avoid[mem_avoid_nr].start = pa_data;
> +			mem_avoid[mem_avoid_nr].size = sizeof(*data) + data->len;
> +			mem_avoid_nr++;
> +		}
> +		pa_data = data->next;
> +	}
> +}
> +
>  static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  			   unsigned long output, unsigned long output_size)
>  {
> @@ -177,6 +199,9 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  	/* Avoid stack memory. */
>  	mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
>  	mem_avoid[4].size = BOOT_STACK_SIZE;
> +	mem_avoid_nr = 5;
> +
> +	mem_avoid_setup_data();
>  }
>  
>  /* Does this memory vector overlap a known avoided area? */
> @@ -184,7 +209,7 @@ static bool mem_avoid_overlap(struct mem_vector *img)
>  {
>  	int i;
>  
> -	for (i = 0; i < MEM_AVOID_MAX; i++) {
> +	for (i = 0; i < mem_avoid_nr; i++) {
>  		if (mem_overlaps(img, &mem_avoid[i]))
>  			return true;
>  	}
> -- 
> 1.8.5.3

Here's an alternative... can you test it?

---
Subject: x86, kaslr: avoid setup_data when choosing kernel location

The KASLR location-choosing logic needs to avoid the setup_data list
areas as well.

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@vger.kernel.org

diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
index fc6091abedb7..7c75c22d9bc3 100644
--- a/arch/x86/boot/compressed/aslr.c
+++ b/arch/x86/boot/compressed/aslr.c
@@ -112,6 +112,7 @@ struct mem_vector {
 
 #define MEM_AVOID_MAX 5
 static struct mem_vector mem_avoid[MEM_AVOID_MAX];
+static struct setup_data *setup_data_avoid;
 
 static bool mem_contains(struct mem_vector *region, struct mem_vector *item)
 {
@@ -177,17 +178,30 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* Avoid stack memory. */
 	mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
 	mem_avoid[4].size = BOOT_STACK_SIZE;
+
+	/* Locate the setup_data list, if it exists. */
+	setup_data_avoid = (struct setup_data *)real_mode->hdr.setup_data;
 }
 
 /* Does this memory vector overlap a known avoided area? */
 static bool mem_avoid_overlap(struct mem_vector *img)
 {
 	int i;
+	struct setup_data *ptr;
 
 	for (i = 0; i < MEM_AVOID_MAX; i++) {
 		if (mem_overlaps(img, &mem_avoid[i]))
 			return true;
 	}
+	for (ptr = setup_data_avoid; ptr; ptr = ptr->next) {
+		struct mem_vector avoid;
+
+		avoid.start = (u64)ptr;
+		avoid.size = sizeof(*ptr) + ptr->len;
+
+		if (mem_overlaps(img, &avoid))
+			return true;
+	}
 
 	return false;
 }

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 2/4] kaslr: check if the random addr is available

[Baoquan He]

On 09/05/14 at 10:16am, Kees Cook wrote:
> On Fri, Sep 5, 2014 at 7:08 AM, Baoquan He <bhe@redhat.com> wrote:
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index 7780a5b..d2a0eaa 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -250,6 +250,11 @@ static void handle_relocations(void *output, unsigned long output_len)
> >         }
> >  #endif
> >
> > +       if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
> > +                debug_putstr("Random addr is not allowed. No relocation needed... \n");
> > +                return;
> > +        }
> > +
> 
> It's not clear to me what this is fixing. In aslr.c,
> process_e820_entry() should already make it impossible to select
> max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET. If you're trying to
> detect a non-kaslr boot, I think this is better handled in 1/4 where I
> suggest examining the "output" location before/after
> choose_kernel_location.
> 
> -Kees

Hi Kees,

Yes, process_e820_entry() can make sure the choice+output_len <
CONFIG_RANDOMIZE_BASE_MAX_OFFSET, but that can't stop other bootloaders
to put kernel in region above CONFIG_RANDOMIZE_BASE_MAX_OFFSET.

E.g in kdump, we can set crashkernel=256M@1024M in cmdline. Then the 1st
kernel will reserve 256M memory just at 1024M place. So if load kdump
kernel now, the output will be 1024M before choose_kernel_location().
With this value, output won't be changed in choose_kernel_location(),
then it will do decompress(), then call handle_relocations(). Then since
1024 is not equal to LOAD_PHYSICAL_ADDR, it will start relocatoins
handling. And this cause _text stamping into MODULES vaddr range. System
will be exceptional.

Thanks
Baoquan
> 
> >         /*
> >          * Calculate the delta between where vmlinux was linked to load
> >          * and where it was actually loaded.
> > --
> > 1.8.5.3
> >
> 
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH 3/4] kaslr setup_data handling

[Baoquan He]

On 09/05/14 at 10:32am, Kees Cook wrote:
> On Fri, Sep 05, 2014 at 10:08:16PM +0800, Baoquan He wrote:
> > From: Dave Young <dyoung@redhat.com>
> > 
> > X86 will pass setup_data region while necessary, these regions could be
> > overwitten by kernel due to kaslr.
> > 
> > Thus iterate and add setup regions to mem_avoid[] in this patch.
> > Up to now there isn't a official data to state the maximal entries
> > setup data could use. So just set max mem avoid entries 32, hopefully
> > it will be enough. This can be increased later when people report
> > they are using more setup data entries.
> 
> Ew, yes, this is bad. I hadn't seen setup_data while designing the
> mem_avoid stuff. I don't like the fixed 32 entry size here, so let me
> consider some options. I think the mem_avoid logic can just walk the
> setup_data list itself, since that's what it's for. :)

Yes, maybe you are right. But it's hard to say what's the max. I guess
1024 could be enough, after all e820MAX is 128. If people don't think
that eat too much stack.

> 
> Does only kexec use this? I assume other boot loaders must be using this
> too. Is there an easy test case for validating this is fixed?

I don't think only kexec use this. If e820 is not enough, extending the
memory region to setup data is designed for all bootloaders. Kernel will
check memory regions from setup data. 

I can't find a machine with so many memory regions. Dave told me one
person has this kind of machine, will ask him to help test if possible.

About test case, if that machine with many memory regions could put
setup data in 1G, that's very easy to test. Not sure of that.

> 

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/05/14 at 10:11am, Kees Cook wrote:
> On Fri, Sep 5, 2014 at 7:08 AM, Baoquan He <bhe@redhat.com> wrote:
> > kaslr's action is splitted into 2 parts. The 1st is getting available memory
> > slots and randomly choose the kernel relocation address. After decompression
> > of kernel to the chosen place, the 2nd part begin to check if kaslr has got
> > a relocation address, and will do the relocations handling if yes.
> >
> > However in current implementation, in the 2nd part, it doesn't check user's
> > config, just compare decompression output address and the LOAD_PHYSICAL_ADDR
> > where kernel was compiled to run. If they are equal, it means a kaslr is
> > taking action, and need do the relocation handling. This truly works when
> > bootloader always load kernel to LOAD_PHYSICAL_ADDR. But this doesn't always
> > happens. Kexec/kdump kernel loading is exceptional. Kdump/kexec can load
> > kernel anywhere, this is not fixed. So in this case, it will do the relocation
> > handling though user clearly set nokaslr in cmdline. This is not correct.
> >
> > So in this patch, check user's config too in the 2nd part, namely in
> > handle_relocations().
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/boot/compressed/aslr.c |  2 ++
> >  arch/x86/boot/compressed/misc.c | 12 ++++++++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> > index fc6091a..975b07b 100644
> > --- a/arch/x86/boot/compressed/aslr.c
> > +++ b/arch/x86/boot/compressed/aslr.c
> > @@ -292,11 +292,13 @@ unsigned char *choose_kernel_location(unsigned char *input,
> >  #ifdef CONFIG_HIBERNATION
> >         if (!cmdline_find_option_bool("kaslr")) {
> >                 debug_putstr("KASLR disabled by default...\n");
> > +               debug_putstr("No need to choose kernel relocation...\n");
> >                 goto out;
> >         }
> >  #else
> >         if (cmdline_find_option_bool("nokaslr")) {
> >                 debug_putstr("KASLR disabled by cmdline...\n");
> > +               debug_putstr("No need to choose kernel relocation...\n");
> >                 goto out;
> >         }
> >  #endif
> 
> NAK on this part -- the whole point is to report the results of the
> expected KASLR state based on build config, cmdline, etc.
> 
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index 57ab74d..7780a5b 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -238,6 +238,18 @@ static void handle_relocations(void *output, unsigned long output_len)
> >         unsigned long min_addr = (unsigned long)output;
> >         unsigned long max_addr = min_addr + output_len;
> >
> > +#ifdef CONFIG_HIBERNATION
> > +       if (!cmdline_find_option_bool("kaslr")) {
> > +               debug_putstr("No relocation needed... ");
> > +               return;
> > +       }
> > +#else
> > +       if (cmdline_find_option_bool("nokaslr")) {
> > +               debug_putstr("No relocation needed... ");
> > +               return;
> > +       }
> > +#endif
> > +
> 
> I don't think this is correct. If you look at a02150610776 ("x86,
> relocs: Move ELF relocation handling to C"), we always did relocations
> on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail
> badly on 32-bit. 64-bit only needs relocation when
> CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be
> tested here instead. I think a better option would be, in
> decompress_kernel(), to compare output before and after
> choose_kernel_location(). If it's the same on 64-bit,
> handle_relocations() can be skipped. (Perhaps pass the before/after to
> handle_relocations() and it can perform the logic.)


This doesn't work for kdump and kexec since they can be put anywhere.
Assume kdump kernel is put in 512M, and I compiled in the kaslr and set
nokaslr in cmdline. It will skip the choose_kernel_location(), then
decompress kernel to 512M. Then it will do relocations handling in
handle_relocations().

This is not correct. If kaslr is not compiled in, the kernel text
mapping will start from 0xffffffff81000000. But now if kaslr is compiled
in, and nokaslr is added into cmdline, the text mapping will start from
0xffffffffa0000000. This result is confusing.

> 
> -Kees
> 
> >         /*
> >          * Calculate the delta between where vmlinux was linked to load
> >          * and where it was actually loaded.
> > --
> > 1.8.5.3
> >
> 
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/05/14 at 10:11am, Kees Cook wrote:
> I don't think this is correct. If you look at a02150610776 ("x86,
> relocs: Move ELF relocation handling to C"), we always did relocations
> on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail
> badly on 32-bit. 64-bit only needs relocation when
> CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be
> tested here instead. I think a better option would be, in
> decompress_kernel(), to compare output before and after
> choose_kernel_location(). If it's the same on 64-bit,
> handle_relocations() can be skipped. (Perhaps pass the before/after to
> handle_relocations() and it can perform the logic.)
> 
> -Kees

Hi Kees,

Checking handle_relocations() again, I just didn't notice it's mandatory
to do the relocations handling in i386. So in this function delta is
checked to see if it's a kaslr relocation handling. This might be a
little confusing. But I am fine with it.

Per your comment, you prefer to compare the output before and after
choose_kernel_location(). That's also good, Lu Yinghai posted a draft
patch in this way before, however the checking and the delta calculation
are not correct. I changed that and test all cases, it works well. So
do you like this it? If yes  I will repost it.

>From 13471bd838c52a0e143c2aee81e3863cfff585bd Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Mon, 25 Aug 2014 14:57:43 +0800
Subject: [PATCH] kaslr: check if kernel location is changed

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/boot/compressed/misc.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 57ab74d..887f404 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -230,8 +230,9 @@ static void error(char *x)
 		asm("hlt");
 }
 
-#if CONFIG_X86_NEED_RELOCS
-static void handle_relocations(void *output, unsigned long output_len)
+#ifdef CONFIG_X86_NEED_RELOCS
+static void handle_relocations(void *output_orig, void *output,
+			       unsigned long output_len)
 {
 	int *reloc;
 	unsigned long delta, map, ptr;
@@ -242,6 +243,9 @@ static void handle_relocations(void *output, unsigned long output_len)
 	 * Calculate the delta between where vmlinux was linked to load
 	 * and where it was actually loaded.
 	 */
+	if (output_orig == output)
+		return;
+
 	delta = min_addr - LOAD_PHYSICAL_ADDR;
 	if (!delta) {
 		debug_putstr("No relocation needed... ");
@@ -299,7 +303,8 @@ static void handle_relocations(void *output, unsigned long output_len)
 #endif
 }
 #else
-static inline void handle_relocations(void *output, unsigned long output_len)
+static inline void handle_relocations(void *output_orig, void *output,
+				      unsigned long output_len)
 { }
 #endif
 
@@ -360,6 +365,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 				  unsigned char *output,
 				  unsigned long output_len)
 {
+	unsigned char *output_orig = output;
+
 	real_mode = rmode;
 
 	sanitize_boot_params(real_mode);
@@ -402,7 +409,7 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	debug_putstr("\nDecompressing Linux... ");
 	decompress(input_data, input_len, NULL, NULL, output, NULL, error);
 	parse_elf(output);
-	handle_relocations(output, output_len);
+	handle_relocations(output_orig, output, output_len);
 	debug_putstr("done.\nBooting the kernel.\n");
 	return output;
 }
-- 
1.8.5.3

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Kees Cook]

On Mon, Sep 8, 2014 at 11:24 PM, Baoquan He <bhe@redhat.com> wrote:
> On 09/05/14 at 10:11am, Kees Cook wrote:
>> I don't think this is correct. If you look at a02150610776 ("x86,
>> relocs: Move ELF relocation handling to C"), we always did relocations
>> on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail
>> badly on 32-bit. 64-bit only needs relocation when
>> CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be
>> tested here instead. I think a better option would be, in
>> decompress_kernel(), to compare output before and after
>> choose_kernel_location(). If it's the same on 64-bit,
>> handle_relocations() can be skipped. (Perhaps pass the before/after to
>> handle_relocations() and it can perform the logic.)
>>
>> -Kees
>
> Hi Kees,
>
> Checking handle_relocations() again, I just didn't notice it's mandatory
> to do the relocations handling in i386. So in this function delta is
> checked to see if it's a kaslr relocation handling. This might be a
> little confusing. But I am fine with it.
>
> Per your comment, you prefer to compare the output before and after
> choose_kernel_location(). That's also good, Lu Yinghai posted a draft
> patch in this way before, however the checking and the delta calculation
> are not correct. I changed that and test all cases, it works well. So
> do you like this it? If yes  I will repost it.
>
> From 13471bd838c52a0e143c2aee81e3863cfff585bd Mon Sep 17 00:00:00 2001
> From: Baoquan He <bhe@redhat.com>
> Date: Mon, 25 Aug 2014 14:57:43 +0800
> Subject: [PATCH] kaslr: check if kernel location is changed
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/boot/compressed/misc.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 57ab74d..887f404 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -230,8 +230,9 @@ static void error(char *x)
>                 asm("hlt");
>  }
>
> -#if CONFIG_X86_NEED_RELOCS
> -static void handle_relocations(void *output, unsigned long output_len)
> +#ifdef CONFIG_X86_NEED_RELOCS
> +static void handle_relocations(void *output_orig, void *output,
> +                              unsigned long output_len)
>  {
>         int *reloc;
>         unsigned long delta, map, ptr;
> @@ -242,6 +243,9 @@ static void handle_relocations(void *output, unsigned long output_len)
>          * Calculate the delta between where vmlinux was linked to load
>          * and where it was actually loaded.
>          */
> +       if (output_orig == output)
> +               return;
> +

I still think this needs a test for the 32-bit case, since IUIC, it
requires relocations unconditionally.

-Kees

>         delta = min_addr - LOAD_PHYSICAL_ADDR;
>         if (!delta) {
>                 debug_putstr("No relocation needed... ");
> @@ -299,7 +303,8 @@ static void handle_relocations(void *output, unsigned long output_len)
>  #endif
>  }
>  #else
> -static inline void handle_relocations(void *output, unsigned long output_len)
> +static inline void handle_relocations(void *output_orig, void *output,
> +                                     unsigned long output_len)
>  { }
>  #endif
>
> @@ -360,6 +365,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
>                                   unsigned char *output,
>                                   unsigned long output_len)
>  {
> +       unsigned char *output_orig = output;
> +
>         real_mode = rmode;
>
>         sanitize_boot_params(real_mode);
> @@ -402,7 +409,7 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
>         debug_putstr("\nDecompressing Linux... ");
>         decompress(input_data, input_len, NULL, NULL, output, NULL, error);
>         parse_elf(output);
> -       handle_relocations(output, output_len);
> +       handle_relocations(output_orig, output, output_len);
>         debug_putstr("done.\nBooting the kernel.\n");
>         return output;
>  }
> --
> 1.8.5.3
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Vivek Goyal]

On Tue, Sep 09, 2014 at 08:53:29AM -0700, Kees Cook wrote:
> On Mon, Sep 8, 2014 at 11:24 PM, Baoquan He <bhe@redhat.com> wrote:
> > On 09/05/14 at 10:11am, Kees Cook wrote:
> >> I don't think this is correct. If you look at a02150610776 ("x86,
> >> relocs: Move ELF relocation handling to C"), we always did relocations
> >> on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail
> >> badly on 32-bit. 64-bit only needs relocation when
> >> CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be
> >> tested here instead. I think a better option would be, in
> >> decompress_kernel(), to compare output before and after
> >> choose_kernel_location(). If it's the same on 64-bit,
> >> handle_relocations() can be skipped. (Perhaps pass the before/after to
> >> handle_relocations() and it can perform the logic.)
> >>
> >> -Kees
> >
> > Hi Kees,
> >
> > Checking handle_relocations() again, I just didn't notice it's mandatory
> > to do the relocations handling in i386. So in this function delta is
> > checked to see if it's a kaslr relocation handling. This might be a
> > little confusing. But I am fine with it.
> >
> > Per your comment, you prefer to compare the output before and after
> > choose_kernel_location(). That's also good, Lu Yinghai posted a draft
> > patch in this way before, however the checking and the delta calculation
> > are not correct. I changed that and test all cases, it works well. So
> > do you like this it? If yes  I will repost it.
> >
> > From 13471bd838c52a0e143c2aee81e3863cfff585bd Mon Sep 17 00:00:00 2001
> > From: Baoquan He <bhe@redhat.com>
> > Date: Mon, 25 Aug 2014 14:57:43 +0800
> > Subject: [PATCH] kaslr: check if kernel location is changed
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/boot/compressed/misc.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index 57ab74d..887f404 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -230,8 +230,9 @@ static void error(char *x)
> >                 asm("hlt");
> >  }
> >
> > -#if CONFIG_X86_NEED_RELOCS
> > -static void handle_relocations(void *output, unsigned long output_len)
> > +#ifdef CONFIG_X86_NEED_RELOCS
> > +static void handle_relocations(void *output_orig, void *output,
> > +                              unsigned long output_len)
> >  {
> >         int *reloc;
> >         unsigned long delta, map, ptr;
> > @@ -242,6 +243,9 @@ static void handle_relocations(void *output, unsigned long output_len)
> >          * Calculate the delta between where vmlinux was linked to load
> >          * and where it was actually loaded.
> >          */
> > +       if (output_orig == output)
> > +               return;
> > +
> 
> I still think this needs a test for the 32-bit case, since IUIC, it
> requires relocations unconditionally.

[ CC hpa ]

Bao, for modifications in this area, I would recommend CC hpa too.

I also think that i386 will always require relocation handling. That's
how Eric had designed it. There was no separate kernel text mapping
region so PAGE_OFFSET was constant. Hence if you shift kernel in physical
address space, you had to shift mappings in virtual address space by
equal amount.

But in x86_64, we have kernel text mapped in a separate virtual region, and 
that allowed us wiggling room and we could load kernel anywhere
in physical address space and just change mappings of kernel text
virtual address region accordingly.

So I agree that on i386, we will most likely require relocations all
the time. Having said that, it is interesting that one can disable
X86_NEED_RELOCS on i386 while RELOCATBALE=y.

# Relocation on x86 needs some additional build support
config X86_NEED_RELOCS
        def_bool y
        depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)

I am not sure how i386 RELOCATABLE kernel will work if X86_NEED_RELOCS=n.


Secondly, IIUC, kernel has 32bit signed relocations. That means
relocations can be processed successfully only if kernel is loaded
in first 2G or -2G. If that's the case, then aslr mechanism should
see that where kernel is loaded physically and if it is loaded outside
the range where relocations can be processed successfully, it should
disable itself and output a message.

That way, one will not have to pass explicit "nokaslr" parameter to kernel.
If kernel can't handle relocations successfully, it will automatically
disable kaslr.

Thanks
Vivek

> 
> -Kees
> 
> >         delta = min_addr - LOAD_PHYSICAL_ADDR;
> >         if (!delta) {
> >                 debug_putstr("No relocation needed... ");
> > @@ -299,7 +303,8 @@ static void handle_relocations(void *output, unsigned long output_len)
> >  #endif
> >  }
> >  #else
> > -static inline void handle_relocations(void *output, unsigned long output_len)
> > +static inline void handle_relocations(void *output_orig, void *output,
> > +                                     unsigned long output_len)
> >  { }
> >  #endif
> >
> > @@ -360,6 +365,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
> >                                   unsigned char *output,
> >                                   unsigned long output_len)
> >  {
> > +       unsigned char *output_orig = output;
> > +
> >         real_mode = rmode;
> >
> >         sanitize_boot_params(real_mode);
> > @@ -402,7 +409,7 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
> >         debug_putstr("\nDecompressing Linux... ");
> >         decompress(input_data, input_len, NULL, NULL, output, NULL, error);
> >         parse_elf(output);
> > -       handle_relocations(output, output_len);
> > +       handle_relocations(output_orig, output, output_len);
> >         debug_putstr("done.\nBooting the kernel.\n");
> >         return output;
> >  }
> > --
> > 1.8.5.3
> >
> 
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH 2/4] kaslr: check if the random addr is available

[Vivek Goyal]

On Sat, Sep 06, 2014 at 06:16:57AM +0800, Baoquan He wrote:
> On 09/05/14 at 10:16am, Kees Cook wrote:
> > On Fri, Sep 5, 2014 at 7:08 AM, Baoquan He <bhe@redhat.com> wrote:
> > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > > index 7780a5b..d2a0eaa 100644
> > > --- a/arch/x86/boot/compressed/misc.c
> > > +++ b/arch/x86/boot/compressed/misc.c
> > > @@ -250,6 +250,11 @@ static void handle_relocations(void *output, unsigned long output_len)
> > >         }
> > >  #endif
> > >
> > > +       if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
> > > +                debug_putstr("Random addr is not allowed. No relocation needed... \n");
> > > +                return;
> > > +        }
> > > +
> > 
> > It's not clear to me what this is fixing. In aslr.c,
> > process_e820_entry() should already make it impossible to select
> > max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET. If you're trying to
> > detect a non-kaslr boot, I think this is better handled in 1/4 where I
> > suggest examining the "output" location before/after
> > choose_kernel_location.
> > 
> > -Kees
> 

[CC hpa ]

> Hi Kees,
> 
> Yes, process_e820_entry() can make sure the choice+output_len <
> CONFIG_RANDOMIZE_BASE_MAX_OFFSET, but that can't stop other bootloaders
> to put kernel in region above CONFIG_RANDOMIZE_BASE_MAX_OFFSET.
> 
> E.g in kdump, we can set crashkernel=256M@1024M in cmdline. Then the 1st
> kernel will reserve 256M memory just at 1024M place. So if load kdump
> kernel now, the output will be 1024M before choose_kernel_location().
> With this value, output won't be changed in choose_kernel_location(),
> then it will do decompress(), then call handle_relocations(). Then since
> 1024 is not equal to LOAD_PHYSICAL_ADDR, it will start relocatoins
> handling. And this cause _text stamping into MODULES vaddr range. System
> will be exceptional.

Bao,

If you apply your first patch where output_orig == output, then
handle_relocations() will not do anything for x86_64 case and bail
out. That should take care of this issue. Isn't it? And we should
not require this patch.

Thanks
Vivek

Re: [PATCH 3/4] kaslr setup_data handling

[Vivek Goyal]

On Fri, Sep 05, 2014 at 10:32:56AM -0700, Kees Cook wrote:
> On Fri, Sep 05, 2014 at 10:08:16PM +0800, Baoquan He wrote:
> > From: Dave Young <dyoung@redhat.com>
> > 
> > X86 will pass setup_data region while necessary, these regions could be
> > overwitten by kernel due to kaslr.
> > 
> > Thus iterate and add setup regions to mem_avoid[] in this patch.
> > Up to now there isn't a official data to state the maximal entries
> > setup data could use. So just set max mem avoid entries 32, hopefully
> > it will be enough. This can be increased later when people report
> > they are using more setup data entries.
> 
> Ew, yes, this is bad. I hadn't seen setup_data while designing the
> mem_avoid stuff. I don't like the fixed 32 entry size here, so let me
> consider some options. I think the mem_avoid logic can just walk the
> setup_data list itself, since that's what it's for. :)
> 
> Does only kexec use this? I assume other boot loaders must be using this
> too. Is there an easy test case for validating this is fixed?

[CC hpa]

I think this is generic mechanism and any bootloader can make use of it.
May be testing it using kexec on an EFI machine might work as kexec
prepares setup_data entry to pass some information to second kernel.

Thanks
Vivek

> 
> > 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/boot/compressed/aslr.c | 29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> > index 975b07b..7e92fc8 100644
> > --- a/arch/x86/boot/compressed/aslr.c
> > +++ b/arch/x86/boot/compressed/aslr.c
> > @@ -110,8 +110,9 @@ struct mem_vector {
> >  	unsigned long size;
> >  };
> >  
> > -#define MEM_AVOID_MAX 5
> > +#define MEM_AVOID_MAX 32
> >  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
> > +static int mem_avoid_nr;
> >  
> >  static bool mem_contains(struct mem_vector *region, struct mem_vector *item)
> >  {
> > @@ -135,6 +136,27 @@ static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
> >  	return true;
> >  }
> >  
> > +static void mem_avoid_setup_data(void)
> > +{
> > +	struct setup_data *data;
> > +	u64 pa_data;
> > +
> > +	pa_data = real_mode->hdr.setup_data;
> > +	while (pa_data) {
> > +		if (mem_avoid_nr >= MEM_AVOID_MAX) {
> > +			debug_putstr("KASLR: too many setup_data ranges.\n");
> > +			return;
> > +		}
> > +		data = (struct setup_data *)pa_data;
> > +		if (pa_data < CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
> > +			mem_avoid[mem_avoid_nr].start = pa_data;
> > +			mem_avoid[mem_avoid_nr].size = sizeof(*data) + data->len;
> > +			mem_avoid_nr++;
> > +		}
> > +		pa_data = data->next;
> > +	}
> > +}
> > +
> >  static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >  			   unsigned long output, unsigned long output_size)
> >  {
> > @@ -177,6 +199,9 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
> >  	/* Avoid stack memory. */
> >  	mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
> >  	mem_avoid[4].size = BOOT_STACK_SIZE;
> > +	mem_avoid_nr = 5;
> > +
> > +	mem_avoid_setup_data();
> >  }
> >  
> >  /* Does this memory vector overlap a known avoided area? */
> > @@ -184,7 +209,7 @@ static bool mem_avoid_overlap(struct mem_vector *img)
> >  {
> >  	int i;
> >  
> > -	for (i = 0; i < MEM_AVOID_MAX; i++) {
> > +	for (i = 0; i < mem_avoid_nr; i++) {
> >  		if (mem_overlaps(img, &mem_avoid[i]))
> >  			return true;
> >  	}
> > -- 
> > 1.8.5.3
> 
> Here's an alternative... can you test it?
> 
> ---
> Subject: x86, kaslr: avoid setup_data when choosing kernel location
> 
> The KASLR location-choosing logic needs to avoid the setup_data list
> areas as well.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@vger.kernel.org
> 
> diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> index fc6091abedb7..7c75c22d9bc3 100644
> --- a/arch/x86/boot/compressed/aslr.c
> +++ b/arch/x86/boot/compressed/aslr.c
> @@ -112,6 +112,7 @@ struct mem_vector {
>  
>  #define MEM_AVOID_MAX 5
>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
> +static struct setup_data *setup_data_avoid;
>  
>  static bool mem_contains(struct mem_vector *region, struct mem_vector *item)
>  {
> @@ -177,17 +178,30 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  	/* Avoid stack memory. */
>  	mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
>  	mem_avoid[4].size = BOOT_STACK_SIZE;
> +
> +	/* Locate the setup_data list, if it exists. */
> +	setup_data_avoid = (struct setup_data *)real_mode->hdr.setup_data;
>  }
>  
>  /* Does this memory vector overlap a known avoided area? */
>  static bool mem_avoid_overlap(struct mem_vector *img)
>  {
>  	int i;
> +	struct setup_data *ptr;
>  
>  	for (i = 0; i < MEM_AVOID_MAX; i++) {
>  		if (mem_overlaps(img, &mem_avoid[i]))
>  			return true;
>  	}
> +	for (ptr = setup_data_avoid; ptr; ptr = ptr->next) {
> +		struct mem_vector avoid;
> +
> +		avoid.start = (u64)ptr;
> +		avoid.size = sizeof(*ptr) + ptr->len;
> +
> +		if (mem_overlaps(img, &avoid))
> +			return true;
> +	}
>  
>  	return false;
>  }
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH 4/4] export the kernel image size KERNEL_IMAGE_SIZE

[Vivek Goyal]

On Fri, Sep 05, 2014 at 10:08:17PM +0800, Baoquan He wrote:
> Now kaslr makes kernel image size changable, not the fixed size 512M.
> So KERNEL_IMAGE_SIZE need be exported to VMCOREINFO, otherwise makedumfile
> will crash.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

This one sounds reasonable. CCing Atshushi. 

Thanks
Vivek

> ---
>  kernel/kexec.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 2bee072..bd680d3 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -2003,6 +2003,9 @@ static int __init crash_save_vmcoreinfo_init(void)
>  #endif
>  	VMCOREINFO_NUMBER(PG_head_mask);
>  	VMCOREINFO_NUMBER(PAGE_BUDDY_MAPCOUNT_VALUE);
> +#ifdef CONFIG_X86
> +       VMCOREINFO_NUMBER(KERNEL_IMAGE_SIZE);
> +#endif
>  #ifdef CONFIG_HUGETLBFS
>  	VMCOREINFO_SYMBOL(free_huge_page);
>  #endif
> -- 
> 1.8.5.3

Re: [PATCH 3/4] kaslr setup_data handling

[H. Peter Anvin]

setup_data is used by a variety of bootloaders.  The first user was large memory machines with more than 128 memory areas.

On September 9, 2014 12:45:00 PM PDT, Vivek Goyal <vgoyal@redhat.com> wrote:
>On Fri, Sep 05, 2014 at 10:32:56AM -0700, Kees Cook wrote:
>> On Fri, Sep 05, 2014 at 10:08:16PM +0800, Baoquan He wrote:
>> > From: Dave Young <dyoung@redhat.com>
>> > 
>> > X86 will pass setup_data region while necessary, these regions
>could be
>> > overwitten by kernel due to kaslr.
>> > 
>> > Thus iterate and add setup regions to mem_avoid[] in this patch.
>> > Up to now there isn't a official data to state the maximal entries
>> > setup data could use. So just set max mem avoid entries 32,
>hopefully
>> > it will be enough. This can be increased later when people report
>> > they are using more setup data entries.
>> 
>> Ew, yes, this is bad. I hadn't seen setup_data while designing the
>> mem_avoid stuff. I don't like the fixed 32 entry size here, so let me
>> consider some options. I think the mem_avoid logic can just walk the
>> setup_data list itself, since that's what it's for. :)
>> 
>> Does only kexec use this? I assume other boot loaders must be using
>this
>> too. Is there an easy test case for validating this is fixed?
>
>[CC hpa]
>
>I think this is generic mechanism and any bootloader can make use of
>it.
>May be testing it using kexec on an EFI machine might work as kexec
>prepares setup_data entry to pass some information to second kernel.
>
>Thanks
>Vivek
>
>> 
>> > 
>> > Signed-off-by: Dave Young <dyoung@redhat.com>
>> > Signed-off-by: Baoquan He <bhe@redhat.com>
>> > ---
>> >  arch/x86/boot/compressed/aslr.c | 29 +++++++++++++++++++++++++++--
>> >  1 file changed, 27 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/arch/x86/boot/compressed/aslr.c
>b/arch/x86/boot/compressed/aslr.c
>> > index 975b07b..7e92fc8 100644
>> > --- a/arch/x86/boot/compressed/aslr.c
>> > +++ b/arch/x86/boot/compressed/aslr.c
>> > @@ -110,8 +110,9 @@ struct mem_vector {
>> >  	unsigned long size;
>> >  };
>> >  
>> > -#define MEM_AVOID_MAX 5
>> > +#define MEM_AVOID_MAX 32
>> >  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>> > +static int mem_avoid_nr;
>> >  
>> >  static bool mem_contains(struct mem_vector *region, struct
>mem_vector *item)
>> >  {
>> > @@ -135,6 +136,27 @@ static bool mem_overlaps(struct mem_vector
>*one, struct mem_vector *two)
>> >  	return true;
>> >  }
>> >  
>> > +static void mem_avoid_setup_data(void)
>> > +{
>> > +	struct setup_data *data;
>> > +	u64 pa_data;
>> > +
>> > +	pa_data = real_mode->hdr.setup_data;
>> > +	while (pa_data) {
>> > +		if (mem_avoid_nr >= MEM_AVOID_MAX) {
>> > +			debug_putstr("KASLR: too many setup_data ranges.\n");
>> > +			return;
>> > +		}
>> > +		data = (struct setup_data *)pa_data;
>> > +		if (pa_data < CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
>> > +			mem_avoid[mem_avoid_nr].start = pa_data;
>> > +			mem_avoid[mem_avoid_nr].size = sizeof(*data) + data->len;
>> > +			mem_avoid_nr++;
>> > +		}
>> > +		pa_data = data->next;
>> > +	}
>> > +}
>> > +
>> >  static void mem_avoid_init(unsigned long input, unsigned long
>input_size,
>> >  			   unsigned long output, unsigned long output_size)
>> >  {
>> > @@ -177,6 +199,9 @@ static void mem_avoid_init(unsigned long input,
>unsigned long input_size,
>> >  	/* Avoid stack memory. */
>> >  	mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
>> >  	mem_avoid[4].size = BOOT_STACK_SIZE;
>> > +	mem_avoid_nr = 5;
>> > +
>> > +	mem_avoid_setup_data();
>> >  }
>> >  
>> >  /* Does this memory vector overlap a known avoided area? */
>> > @@ -184,7 +209,7 @@ static bool mem_avoid_overlap(struct mem_vector
>*img)
>> >  {
>> >  	int i;
>> >  
>> > -	for (i = 0; i < MEM_AVOID_MAX; i++) {
>> > +	for (i = 0; i < mem_avoid_nr; i++) {
>> >  		if (mem_overlaps(img, &mem_avoid[i]))
>> >  			return true;
>> >  	}
>> > -- 
>> > 1.8.5.3
>> 
>> Here's an alternative... can you test it?
>> 
>> ---
>> Subject: x86, kaslr: avoid setup_data when choosing kernel location
>> 
>> The KASLR location-choosing logic needs to avoid the setup_data list
>> areas as well.
>> 
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: stable@vger.kernel.org
>> 
>> diff --git a/arch/x86/boot/compressed/aslr.c
>b/arch/x86/boot/compressed/aslr.c
>> index fc6091abedb7..7c75c22d9bc3 100644
>> --- a/arch/x86/boot/compressed/aslr.c
>> +++ b/arch/x86/boot/compressed/aslr.c
>> @@ -112,6 +112,7 @@ struct mem_vector {
>>  
>>  #define MEM_AVOID_MAX 5
>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>> +static struct setup_data *setup_data_avoid;
>>  
>>  static bool mem_contains(struct mem_vector *region, struct
>mem_vector *item)
>>  {
>> @@ -177,17 +178,30 @@ static void mem_avoid_init(unsigned long input,
>unsigned long input_size,
>>  	/* Avoid stack memory. */
>>  	mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
>>  	mem_avoid[4].size = BOOT_STACK_SIZE;
>> +
>> +	/* Locate the setup_data list, if it exists. */
>> +	setup_data_avoid = (struct setup_data *)real_mode->hdr.setup_data;
>>  }
>>  
>>  /* Does this memory vector overlap a known avoided area? */
>>  static bool mem_avoid_overlap(struct mem_vector *img)
>>  {
>>  	int i;
>> +	struct setup_data *ptr;
>>  
>>  	for (i = 0; i < MEM_AVOID_MAX; i++) {
>>  		if (mem_overlaps(img, &mem_avoid[i]))
>>  			return true;
>>  	}
>> +	for (ptr = setup_data_avoid; ptr; ptr = ptr->next) {
>> +		struct mem_vector avoid;
>> +
>> +		avoid.start = (u64)ptr;
>> +		avoid.size = sizeof(*ptr) + ptr->len;
>> +
>> +		if (mem_overlaps(img, &avoid))
>> +			return true;
>> +	}
>>  
>>  	return false;
>>  }
>> 
>> -- 
>> Kees Cook
>> Chrome OS Security

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH 3/4] kaslr setup_data handling

[Kees Cook]

On Tue, Sep 9, 2014 at 12:49 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> setup_data is used by a variety of bootloaders.  The first user was large memory machines with more than 128 memory areas.
>
> On September 9, 2014 12:45:00 PM PDT, Vivek Goyal <vgoyal@redhat.com> wrote:
>>On Fri, Sep 05, 2014 at 10:32:56AM -0700, Kees Cook wrote:
>>> On Fri, Sep 05, 2014 at 10:08:16PM +0800, Baoquan He wrote:
>>> > From: Dave Young <dyoung@redhat.com>
>>> >
>>> > X86 will pass setup_data region while necessary, these regions
>>could be
>>> > overwitten by kernel due to kaslr.
>>> >
>>> > Thus iterate and add setup regions to mem_avoid[] in this patch.
>>> > Up to now there isn't a official data to state the maximal entries
>>> > setup data could use. So just set max mem avoid entries 32,
>>hopefully
>>> > it will be enough. This can be increased later when people report
>>> > they are using more setup data entries.
>>>
>>> Ew, yes, this is bad. I hadn't seen setup_data while designing the
>>> mem_avoid stuff. I don't like the fixed 32 entry size here, so let me
>>> consider some options. I think the mem_avoid logic can just walk the
>>> setup_data list itself, since that's what it's for. :)
>>>
>>> Does only kexec use this? I assume other boot loaders must be using
>>this
>>> too. Is there an easy test case for validating this is fixed?
>>
>>[CC hpa]
>>
>>I think this is generic mechanism and any bootloader can make use of
>>it.
>>May be testing it using kexec on an EFI machine might work as kexec
>>prepares setup_data entry to pass some information to second kernel.

Has anyone been able to test my solution? I could try to cook
something up with QEMU, but that might take a while. Note that I've
fixed a few build warnings in my originally proposed patch. The latest
is here:
https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=kaslr&id=be892817cf8424c22739c56df472f160fc710021

-Kees

>>> Subject: x86, kaslr: avoid setup_data when choosing kernel location
>>>
>>> The KASLR location-choosing logic needs to avoid the setup_data list
>>> areas as well.
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Cc: stable@vger.kernel.org
>>>
>>> diff --git a/arch/x86/boot/compressed/aslr.c
>>b/arch/x86/boot/compressed/aslr.c
>>> index fc6091abedb7..7c75c22d9bc3 100644
>>> --- a/arch/x86/boot/compressed/aslr.c
>>> +++ b/arch/x86/boot/compressed/aslr.c
>>> @@ -112,6 +112,7 @@ struct mem_vector {
>>>
>>>  #define MEM_AVOID_MAX 5
>>>  static struct mem_vector mem_avoid[MEM_AVOID_MAX];
>>> +static struct setup_data *setup_data_avoid;
>>>
>>>  static bool mem_contains(struct mem_vector *region, struct
>>mem_vector *item)
>>>  {
>>> @@ -177,17 +178,30 @@ static void mem_avoid_init(unsigned long input,
>>unsigned long input_size,
>>>      /* Avoid stack memory. */
>>>      mem_avoid[4].start = (unsigned long)free_mem_end_ptr;
>>>      mem_avoid[4].size = BOOT_STACK_SIZE;
>>> +
>>> +    /* Locate the setup_data list, if it exists. */
>>> +    setup_data_avoid = (struct setup_data *)real_mode->hdr.setup_data;
>>>  }
>>>
>>>  /* Does this memory vector overlap a known avoided area? */
>>>  static bool mem_avoid_overlap(struct mem_vector *img)
>>>  {
>>>      int i;
>>> +    struct setup_data *ptr;
>>>
>>>      for (i = 0; i < MEM_AVOID_MAX; i++) {
>>>              if (mem_overlaps(img, &mem_avoid[i]))
>>>                      return true;
>>>      }
>>> +    for (ptr = setup_data_avoid; ptr; ptr = ptr->next) {
>>> +            struct mem_vector avoid;
>>> +
>>> +            avoid.start = (u64)ptr;
>>> +            avoid.size = sizeof(*ptr) + ptr->len;
>>> +
>>> +            if (mem_overlaps(img, &avoid))
>>> +                    return true;
>>> +    }
>>>
>>>      return false;
>>>  }
>>>
>>> --
>>> Kees Cook
>>> Chrome OS Security
>
> --
> Sent from my mobile phone.  Please pardon brevity and lack of formatting.



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Kees Cook]

On Tue, Sep 9, 2014 at 12:28 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Sep 09, 2014 at 08:53:29AM -0700, Kees Cook wrote:
>> On Mon, Sep 8, 2014 at 11:24 PM, Baoquan He <bhe@redhat.com> wrote:
>> > On 09/05/14 at 10:11am, Kees Cook wrote:
>> >> I don't think this is correct. If you look at a02150610776 ("x86,
>> >> relocs: Move ELF relocation handling to C"), we always did relocations
>> >> on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail
>> >> badly on 32-bit. 64-bit only needs relocation when
>> >> CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be
>> >> tested here instead. I think a better option would be, in
>> >> decompress_kernel(), to compare output before and after
>> >> choose_kernel_location(). If it's the same on 64-bit,
>> >> handle_relocations() can be skipped. (Perhaps pass the before/after to
>> >> handle_relocations() and it can perform the logic.)
>> >>
>> >> -Kees
>> >
>> > Hi Kees,
>> >
>> > Checking handle_relocations() again, I just didn't notice it's mandatory
>> > to do the relocations handling in i386. So in this function delta is
>> > checked to see if it's a kaslr relocation handling. This might be a
>> > little confusing. But I am fine with it.
>> >
>> > Per your comment, you prefer to compare the output before and after
>> > choose_kernel_location(). That's also good, Lu Yinghai posted a draft
>> > patch in this way before, however the checking and the delta calculation
>> > are not correct. I changed that and test all cases, it works well. So
>> > do you like this it? If yes  I will repost it.
>> >
>> > From 13471bd838c52a0e143c2aee81e3863cfff585bd Mon Sep 17 00:00:00 2001
>> > From: Baoquan He <bhe@redhat.com>
>> > Date: Mon, 25 Aug 2014 14:57:43 +0800
>> > Subject: [PATCH] kaslr: check if kernel location is changed
>> >
>> > Signed-off-by: Baoquan He <bhe@redhat.com>
>> > ---
>> >  arch/x86/boot/compressed/misc.c | 15 +++++++++++----
>> >  1 file changed, 11 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>> > index 57ab74d..887f404 100644
>> > --- a/arch/x86/boot/compressed/misc.c
>> > +++ b/arch/x86/boot/compressed/misc.c
>> > @@ -230,8 +230,9 @@ static void error(char *x)
>> >                 asm("hlt");
>> >  }
>> >
>> > -#if CONFIG_X86_NEED_RELOCS
>> > -static void handle_relocations(void *output, unsigned long output_len)
>> > +#ifdef CONFIG_X86_NEED_RELOCS
>> > +static void handle_relocations(void *output_orig, void *output,
>> > +                              unsigned long output_len)
>> >  {
>> >         int *reloc;
>> >         unsigned long delta, map, ptr;
>> > @@ -242,6 +243,9 @@ static void handle_relocations(void *output, unsigned long output_len)
>> >          * Calculate the delta between where vmlinux was linked to load
>> >          * and where it was actually loaded.
>> >          */
>> > +       if (output_orig == output)
>> > +               return;
>> > +
>>
>> I still think this needs a test for the 32-bit case, since IUIC, it
>> requires relocations unconditionally.
>
> [ CC hpa ]
>
> Bao, for modifications in this area, I would recommend CC hpa too.
>
> I also think that i386 will always require relocation handling. That's
> how Eric had designed it. There was no separate kernel text mapping
> region so PAGE_OFFSET was constant. Hence if you shift kernel in physical
> address space, you had to shift mappings in virtual address space by
> equal amount.
>
> But in x86_64, we have kernel text mapped in a separate virtual region, and
> that allowed us wiggling room and we could load kernel anywhere
> in physical address space and just change mappings of kernel text
> virtual address region accordingly.
>
> So I agree that on i386, we will most likely require relocations all
> the time. Having said that, it is interesting that one can disable
> X86_NEED_RELOCS on i386 while RELOCATBALE=y.
>
> # Relocation on x86 needs some additional build support
> config X86_NEED_RELOCS
>         def_bool y
>         depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>
> I am not sure how i386 RELOCATABLE kernel will work if X86_NEED_RELOCS=n.
>
>
> Secondly, IIUC, kernel has 32bit signed relocations. That means
> relocations can be processed successfully only if kernel is loaded
> in first 2G or -2G. If that's the case, then aslr mechanism should
> see that where kernel is loaded physically and if it is loaded outside
> the range where relocations can be processed successfully, it should
> disable itself and output a message.

Yeah, I think this is the best idea. I'm not sure if it can currently
handle -2G, though. Right now it gives up doing ASLR if it's above 2G,
but doesn't give up on doing relocation, so that's the core of this
bug, I think.

-Kees

>
> That way, one will not have to pass explicit "nokaslr" parameter to kernel.
> If kernel can't handle relocations successfully, it will automatically
> disable kaslr.
>
> Thanks
> Vivek
>
>>
>> -Kees
>>
>> >         delta = min_addr - LOAD_PHYSICAL_ADDR;
>> >         if (!delta) {
>> >                 debug_putstr("No relocation needed... ");
>> > @@ -299,7 +303,8 @@ static void handle_relocations(void *output, unsigned long output_len)
>> >  #endif
>> >  }
>> >  #else
>> > -static inline void handle_relocations(void *output, unsigned long output_len)
>> > +static inline void handle_relocations(void *output_orig, void *output,
>> > +                                     unsigned long output_len)
>> >  { }
>> >  #endif
>> >
>> > @@ -360,6 +365,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
>> >                                   unsigned char *output,
>> >                                   unsigned long output_len)
>> >  {
>> > +       unsigned char *output_orig = output;
>> > +
>> >         real_mode = rmode;
>> >
>> >         sanitize_boot_params(real_mode);
>> > @@ -402,7 +409,7 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
>> >         debug_putstr("\nDecompressing Linux... ");
>> >         decompress(input_data, input_len, NULL, NULL, output, NULL, error);
>> >         parse_elf(output);
>> > -       handle_relocations(output, output_len);
>> > +       handle_relocations(output_orig, output, output_len);
>> >         debug_putstr("done.\nBooting the kernel.\n");
>> >         return output;
>> >  }
>> > --
>> > 1.8.5.3
>> >
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/09/14 at 08:53am, Kees Cook wrote:
 >
> > Hi Kees,
> >
> > Checking handle_relocations() again, I just didn't notice it's mandatory
> > to do the relocations handling in i386. So in this function delta is
> > checked to see if it's a kaslr relocation handling. This might be a
> > little confusing. But I am fine with it.
> >
> > Per your comment, you prefer to compare the output before and after
> > choose_kernel_location(). That's also good, Lu Yinghai posted a draft
> > patch in this way before, however the checking and the delta calculation
> > are not correct. I changed that and test all cases, it works well. So
> > do you like this it? If yes  I will repost it.
> >
> > From 13471bd838c52a0e143c2aee81e3863cfff585bd Mon Sep 17 00:00:00 2001
> > From: Baoquan He <bhe@redhat.com>
> > Date: Mon, 25 Aug 2014 14:57:43 +0800
> > Subject: [PATCH] kaslr: check if kernel location is changed
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/x86/boot/compressed/misc.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index 57ab74d..887f404 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -230,8 +230,9 @@ static void error(char *x)
> >                 asm("hlt");
> >  }
> >
> > -#if CONFIG_X86_NEED_RELOCS
> > -static void handle_relocations(void *output, unsigned long output_len)
> > +#ifdef CONFIG_X86_NEED_RELOCS
> > +static void handle_relocations(void *output_orig, void *output,
> > +                              unsigned long output_len)
> >  {
> >         int *reloc;
> >         unsigned long delta, map, ptr;
> > @@ -242,6 +243,9 @@ static void handle_relocations(void *output, unsigned long output_len)
> >          * Calculate the delta between where vmlinux was linked to load
> >          * and where it was actually loaded.
> >          */
> > +       if (output_orig == output)
> > +               return;
> > +
> 
> I still think this needs a test for the 32-bit case, since IUIC, it
> requires relocations unconditionally.

Oops, just understood that 32 bit kernel alwasy need relocations, but
only focus on x86_64 and kaslr again when I was doing it. You are right,
this is not correct for 32 bit kernel.

I am thinking if I can add a compiling condition check like below. This
only works only when x86_64 or when kaslr is compiled in. Otherwise it
wokrs as before.

#if CONFIG_X86_64
	if (output_orig == output)
		return;
#endif

or
#if CONFIG_RANDOMIZE_BASE
	if (output_orig == output)
		return;
#endif



> 
> -Kees
> 

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/09/14 at 03:28pm, Vivek Goyal wrote:
> On Tue, Sep 09, 2014 at 08:53:29AM -0700, Kees Cook wrote:
 
> > I still think this needs a test for the 32-bit case, since IUIC, it
> > requires relocations unconditionally.
> 
> [ CC hpa ]
> 
> Bao, for modifications in this area, I would recommend CC hpa too.
> 
> I also think that i386 will always require relocation handling. That's
> how Eric had designed it. There was no separate kernel text mapping
> region so PAGE_OFFSET was constant. Hence if you shift kernel in physical
> address space, you had to shift mappings in virtual address space by
> equal amount.
> 
> But in x86_64, we have kernel text mapped in a separate virtual region, and 
> that allowed us wiggling room and we could load kernel anywhere
> in physical address space and just change mappings of kernel text
> virtual address region accordingly.
> 
> So I agree that on i386, we will most likely require relocations all
> the time. Having said that, it is interesting that one can disable
> X86_NEED_RELOCS on i386 while RELOCATBALE=y.
> 
> # Relocation on x86 needs some additional build support
> config X86_NEED_RELOCS
>         def_bool y
>         depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
> 
> I am not sure how i386 RELOCATABLE kernel will work if X86_NEED_RELOCS=n.
> 
> 
> Secondly, IIUC, kernel has 32bit signed relocations. That means
> relocations can be processed successfully only if kernel is loaded
> in first 2G or -2G. If that's the case, then aslr mechanism should
> see that where kernel is loaded physically and if it is loaded outside
> the range where relocations can be processed successfully, it should
> disable itself and output a message.

Yes, kernel only handle 2G or -2G relocation since 32 bit signed
relocations. But for aslr, since the kernel text mapping shares 2G
virtual address space with modules, only 1G relocation can be done. I
took a test, when load kernel at 1G, if not checking if output_orig
and output are equal, it will trigger a BIOS reboot. And with the
restriction checking in process_e820_entry(), the kaslr relocations only
happens inside 1G.

If max_addr is outside 1G, namely CONFIG_RANDOMIZE_BASE_MAX_OFFSET, the
kaslr random kernel location choosing won't happen, then checking if
output_orig is equal to outout in handle_relocations(), if equal nothing
happened. This truly don't need to specify "nokaslr". 

In fact, I think below checking will be clearer and works too.


static void handle_relocations(void *output, unsigned long output_len)
{

...

#if CONFIG_X86_64

or

#if CONFIG_RANDOMIZE_BASE
#ifdef CONFIG_HIBERNATION
	if (!cmdline_find_option_bool("kaslr")) {
               debug_putstr("No relocation needed... ");
               return;
	}
#else
	if (cmdline_find_option_bool("nokaslr")) {
               debug_putstr("No relocation needed... ");
               return;
	}
#endif

	if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
                debug_putstr("Random addr is not allowed. No relocation
needed... \n");
                return;
        }

#endif

...
}

> 
> That way, one will not have to pass explicit "nokaslr" parameter to kernel.
> If kernel can't handle relocations successfully, it will automatically
> disable kaslr.
> 
> Thanks
> Vivek
> 
> > 
> > -Kees
> > 
> > >         delta = min_addr - LOAD_PHYSICAL_ADDR;
> > >         if (!delta) {
> > >                 debug_putstr("No relocation needed... ");
> > > @@ -299,7 +303,8 @@ static void handle_relocations(void *output, unsigned long output_len)
> > >  #endif
> > >  }
> > >  #else
> > > -static inline void handle_relocations(void *output, unsigned long output_len)
> > > +static inline void handle_relocations(void *output_orig, void *output,
> > > +                                     unsigned long output_len)
> > >  { }
> > >  #endif
> > >
> > > @@ -360,6 +365,8 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
> > >                                   unsigned char *output,
> > >                                   unsigned long output_len)
> > >  {
> > > +       unsigned char *output_orig = output;
> > > +
> > >         real_mode = rmode;
> > >
> > >         sanitize_boot_params(real_mode);
> > > @@ -402,7 +409,7 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
> > >         debug_putstr("\nDecompressing Linux... ");
> > >         decompress(input_data, input_len, NULL, NULL, output, NULL, error);
> > >         parse_elf(output);
> > > -       handle_relocations(output, output_len);
> > > +       handle_relocations(output_orig, output, output_len);
> > >         debug_putstr("done.\nBooting the kernel.\n");
> > >         return output;
> > >  }
> > > --
> > > 1.8.5.3
> > >
> > 
> > 
> > 
> > -- 
> > Kees Cook
> > Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Vivek Goyal]

On Wed, Sep 10, 2014 at 02:10:35PM +0800, Baoquan He wrote:

[..]
> > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > > index 57ab74d..887f404 100644
> > > --- a/arch/x86/boot/compressed/misc.c
> > > +++ b/arch/x86/boot/compressed/misc.c
> > > @@ -230,8 +230,9 @@ static void error(char *x)
> > >                 asm("hlt");
> > >  }
> > >
> > > -#if CONFIG_X86_NEED_RELOCS
> > > -static void handle_relocations(void *output, unsigned long output_len)
> > > +#ifdef CONFIG_X86_NEED_RELOCS
> > > +static void handle_relocations(void *output_orig, void *output,
> > > +                              unsigned long output_len)
> > >  {
> > >         int *reloc;
> > >         unsigned long delta, map, ptr;
> > > @@ -242,6 +243,9 @@ static void handle_relocations(void *output, unsigned long output_len)
> > >          * Calculate the delta between where vmlinux was linked to load
> > >          * and where it was actually loaded.
> > >          */
> > > +       if (output_orig == output)
> > > +               return;
> > > +
> > 
> > I still think this needs a test for the 32-bit case, since IUIC, it
> > requires relocations unconditionally.
> 
> Oops, just understood that 32 bit kernel alwasy need relocations, but
> only focus on x86_64 and kaslr again when I was doing it. You are right,
> this is not correct for 32 bit kernel.
> 
> I am thinking if I can add a compiling condition check like below. This
> only works only when x86_64 or when kaslr is compiled in. Otherwise it
> wokrs as before.
> 
> #if CONFIG_X86_64
> 	if (output_orig == output)
> 		return;
> #endif

Hi Bao,

I think above should work reasonably well. Also put a comment above it.
Something like as follows.

/*
 * 32bit always requires relocations to be performed. For x86_64,
 * relocations need to be performed only if kaslr has chosen a
 * different load address then kernel was originally loaded at.
 *
 * If we are here, either kaslr is not configured in or kaslr is disabled
 * or kaslr has chosen not to change the load location of kernel. Don't
 * perform any relocations.
 */

Thanks
Vivek

Re: [PATCH 2/4] kaslr: check if the random addr is available

[Baoquan He]

On 09/09/14 at 03:41pm, Vivek Goyal wrote:
> On Sat, Sep 06, 2014 at 06:16:57AM +0800, Baoquan He wrote:
> 
> [CC hpa ]
> 
> > Hi Kees,
> > 
> > Yes, process_e820_entry() can make sure the choice+output_len <
> > CONFIG_RANDOMIZE_BASE_MAX_OFFSET, but that can't stop other bootloaders
> > to put kernel in region above CONFIG_RANDOMIZE_BASE_MAX_OFFSET.
> > 
> > E.g in kdump, we can set crashkernel=256M@1024M in cmdline. Then the 1st
> > kernel will reserve 256M memory just at 1024M place. So if load kdump
> > kernel now, the output will be 1024M before choose_kernel_location().
> > With this value, output won't be changed in choose_kernel_location(),
> > then it will do decompress(), then call handle_relocations(). Then since
> > 1024 is not equal to LOAD_PHYSICAL_ADDR, it will start relocatoins
> > handling. And this cause _text stamping into MODULES vaddr range. System
> > will be exceptional.
> 
> Bao,
> 
> If you apply your first patch where output_orig == output, then
> handle_relocations() will not do anything for x86_64 case and bail
> out. That should take care of this issue. Isn't it? And we should
> not require this patch.

Yes, exactly.

> 
> Thanks
> Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Vivek Goyal]

On Wed, Sep 10, 2014 at 03:21:15PM +0800, Baoquan He wrote:
> On 09/09/14 at 03:28pm, Vivek Goyal wrote:
> > On Tue, Sep 09, 2014 at 08:53:29AM -0700, Kees Cook wrote:
>  
> > > I still think this needs a test for the 32-bit case, since IUIC, it
> > > requires relocations unconditionally.
> > 
> > [ CC hpa ]
> > 
> > Bao, for modifications in this area, I would recommend CC hpa too.
> > 
> > I also think that i386 will always require relocation handling. That's
> > how Eric had designed it. There was no separate kernel text mapping
> > region so PAGE_OFFSET was constant. Hence if you shift kernel in physical
> > address space, you had to shift mappings in virtual address space by
> > equal amount.
> > 
> > But in x86_64, we have kernel text mapped in a separate virtual region, and 
> > that allowed us wiggling room and we could load kernel anywhere
> > in physical address space and just change mappings of kernel text
> > virtual address region accordingly.
> > 
> > So I agree that on i386, we will most likely require relocations all
> > the time. Having said that, it is interesting that one can disable
> > X86_NEED_RELOCS on i386 while RELOCATBALE=y.
> > 
> > # Relocation on x86 needs some additional build support
> > config X86_NEED_RELOCS
> >         def_bool y
> >         depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
> > 
> > I am not sure how i386 RELOCATABLE kernel will work if X86_NEED_RELOCS=n.
> > 
> > 
> > Secondly, IIUC, kernel has 32bit signed relocations. That means
> > relocations can be processed successfully only if kernel is loaded
> > in first 2G or -2G. If that's the case, then aslr mechanism should
> > see that where kernel is loaded physically and if it is loaded outside
> > the range where relocations can be processed successfully, it should
> > disable itself and output a message.
> 
> Yes, kernel only handle 2G or -2G relocation since 32 bit signed
> relocations. But for aslr, since the kernel text mapping shares 2G
> virtual address space with modules, only 1G relocation can be done. I
> took a test, when load kernel at 1G, if not checking if output_orig
> and output are equal, it will trigger a BIOS reboot. And with the
> restriction checking in process_e820_entry(), the kaslr relocations only
> happens inside 1G.
> 
> If max_addr is outside 1G, namely CONFIG_RANDOMIZE_BASE_MAX_OFFSET, the
> kaslr random kernel location choosing won't happen, then checking if
> output_orig is equal to outout in handle_relocations(), if equal nothing
> happened. This truly don't need to specify "nokaslr". 
> 
> In fact, I think below checking will be clearer and works too.
> 
> 
> static void handle_relocations(void *output, unsigned long output_len)
> {
> 
> ...
> 
> #if CONFIG_X86_64
> 
> or
> 
> #if CONFIG_RANDOMIZE_BASE
> #ifdef CONFIG_HIBERNATION
> 	if (!cmdline_find_option_bool("kaslr")) {
>                debug_putstr("No relocation needed... ");
>                return;
> 	}
> #else
> 	if (cmdline_find_option_bool("nokaslr")) {
>                debug_putstr("No relocation needed... ");
>                return;
> 	}
> #endif

> 
> 	if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
>                 debug_putstr("Random addr is not allowed. No relocation
> needed... \n");
>                 return;

Hi Bao,

I dont think that this is required or this is correct. kaslr will not
choose a physical location which is beyond CONFIG_RANDOMIZE_BASE_MAX_OFFSET,
So not sure what will this do.

Just the other check of output_orig == output should fix issue for us.

If one is not passing nokaslr in case of kexec, and kernel is loaded
high (say 64G), I think kaslr will automatically move kernel below 1G
and handle_relocations() should succeed.

In case of kdump we will have to pass nokaslr, as we don't want kernel
to move as it could stomp over other things we have loaded.

So I would suggest that test and repost the other patch with proper changelog
and that might be sufficient for now. Only other thing we will need is
Kees's patch for avoiding setup data regions in kaslr.

Thanks
Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Kees Cook]

On Wed, Sep 10, 2014 at 7:30 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> So I would suggest that test and repost the other patch with proper changelog
> and that might be sufficient for now. Only other thing we will need is
> Kees's patch for avoiding setup data regions in kaslr.

If someone can confirm that my patch works, I can request x86 pull it.

Currently living here:
https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/10/14 at 10:30am, Vivek Goyal wrote:
> On Wed, Sep 10, 2014 at 03:21:15PM +0800, Baoquan He wrote:
 
 
> > In fact, I think below checking will be clearer and works too.
> > 
> > 
> > static void handle_relocations(void *output, unsigned long output_len)
> > {
> > 
> > ...
> > 
> > #if CONFIG_X86_64
> > 
> > or
> > 
> > #if CONFIG_RANDOMIZE_BASE
> > #ifdef CONFIG_HIBERNATION
> > 	if (!cmdline_find_option_bool("kaslr")) {
> >                debug_putstr("No relocation needed... ");
> >                return;
> > 	}
> > #else
> > 	if (cmdline_find_option_bool("nokaslr")) {
> >                debug_putstr("No relocation needed... ");
> >                return;
> > 	}
> > #endif
> 
> > 
> > 	if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
> >                 debug_putstr("Random addr is not allowed. No relocation
> > needed... \n");
> >                 return;
> 
> Hi Bao,
> 
> I dont think that this is required or this is correct. kaslr will not
> choose a physical location which is beyond CONFIG_RANDOMIZE_BASE_MAX_OFFSET,
> So not sure what will this do.
> 
> Just the other check of output_orig == output should fix issue for us.
> 
> If one is not passing nokaslr in case of kexec, and kernel is loaded
> high (say 64G), I think kaslr will automatically move kernel below 1G
> and handle_relocations() should succeed.

No, if kernel is loaded high (say 64G), the random kernel location
choosing won't happen. Since in process_e820_entry() it only accepts the
memory region which is above output, plus the
CONFIG_RANDOMIZE_BASE_MAX_OFFSET checking, any kenrel loaded high above
1G will not get a new random kernel location. So it's safe in this case.

> 
> In case of kdump we will have to pass nokaslr, as we don't want kernel
> to move as it could stomp over other things we have loaded.

For kdump and kexec nokaslr is unnecessary. As you know we always
call add_buffer with buf_end as 1, this will cause kernel loaded at the
top of available memory. E.g on my pc with 16G memory, kexec kernel will
be put nearby 16G, so no random location choosing happen as I said in
above. For kdump, if reserved memory is at 500M~700M, then kernel will
be put nearby 700M, the random location choosing also never happen.

In fact, for some cases I need change kexec-tools user app code, to make
kernel be put from down to top.

> 
> So I would suggest that test and repost the other patch with proper changelog
> and that might be sufficient for now. Only other thing we will need is
> Kees's patch for avoiding setup data regions in kaslr.

Anyway, it's great people think the "output_orig=output" is good and can
solve our problems, and it does change less code. I can repost that one
with your suggestion that put some comments above that.

Thanks for your comments and suggstion.

> 
> Thanks
> Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Vivek Goyal]

On Wed, Sep 10, 2014 at 10:53:34PM +0800, Baoquan He wrote:
> On 09/10/14 at 10:30am, Vivek Goyal wrote:
> > On Wed, Sep 10, 2014 at 03:21:15PM +0800, Baoquan He wrote:
>  
>  
> > > In fact, I think below checking will be clearer and works too.
> > > 
> > > 
> > > static void handle_relocations(void *output, unsigned long output_len)
> > > {
> > > 
> > > ...
> > > 
> > > #if CONFIG_X86_64
> > > 
> > > or
> > > 
> > > #if CONFIG_RANDOMIZE_BASE
> > > #ifdef CONFIG_HIBERNATION
> > > 	if (!cmdline_find_option_bool("kaslr")) {
> > >                debug_putstr("No relocation needed... ");
> > >                return;
> > > 	}
> > > #else
> > > 	if (cmdline_find_option_bool("nokaslr")) {
> > >                debug_putstr("No relocation needed... ");
> > >                return;
> > > 	}
> > > #endif
> > 
> > > 
> > > 	if (max_addr > CONFIG_RANDOMIZE_BASE_MAX_OFFSET) {
> > >                 debug_putstr("Random addr is not allowed. No relocation
> > > needed... \n");
> > >                 return;
> > 
> > Hi Bao,
> > 
> > I dont think that this is required or this is correct. kaslr will not
> > choose a physical location which is beyond CONFIG_RANDOMIZE_BASE_MAX_OFFSET,
> > So not sure what will this do.
> > 
> > Just the other check of output_orig == output should fix issue for us.
> > 
> > If one is not passing nokaslr in case of kexec, and kernel is loaded
> > high (say 64G), I think kaslr will automatically move kernel below 1G
> > and handle_relocations() should succeed.
> 
> No, if kernel is loaded high (say 64G), the random kernel location
> choosing won't happen. Since in process_e820_entry() it only accepts the
> memory region which is above output, plus the
> CONFIG_RANDOMIZE_BASE_MAX_OFFSET checking, any kenrel loaded high above
> 1G will not get a new random kernel location. So it's safe in this case.

Ok, if kaslr does not move kernel in this case, that is also fine. Your
other patch will detect this condition and not perform any relocations.
That means kexec should work as it is without passing nokaslr.

> 
> > 
> > In case of kdump we will have to pass nokaslr, as we don't want kernel
> > to move as it could stomp over other things we have loaded.
> 
> For kdump and kexec nokaslr is unnecessary. As you know we always
> call add_buffer with buf_end as 1, this will cause kernel loaded at the
> top of available memory. E.g on my pc with 16G memory, kexec kernel will
> be put nearby 16G, so no random location choosing happen as I said in
> above. For kdump, if reserved memory is at 500M~700M, then kernel will
> be put nearby 700M, the random location choosing also never happen.
> 
> In fact, for some cases I need change kexec-tools user app code, to make
> kernel be put from down to top.

I think we can't rely on where exactly in memory kexec-tools places the
kernel. For kdump case we will have to pass nokaslr to make sure that
kaslr does not move kernel.

Thanks
Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Vivek Goyal]

On Wed, Sep 10, 2014 at 07:41:38AM -0700, Kees Cook wrote:
> On Wed, Sep 10, 2014 at 7:30 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > So I would suggest that test and repost the other patch with proper changelog
> > and that might be sufficient for now. Only other thing we will need is
> > Kees's patch for avoiding setup data regions in kaslr.
> 
> If someone can confirm that my patch works, I can request x86 pull it.
> 
> Currently living here:
> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr
> 

Bao,

Can you please test Kees's patch and provide feedback.

Thanks
Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/10/14 at 11:04am, Vivek Goyal wrote:
> On Wed, Sep 10, 2014 at 10:53:34PM +0800, Baoquan He wrote:
> > On 09/10/14 at 10:30am, Vivek Goyal wrote:
> > > In case of kdump we will have to pass nokaslr, as we don't want kernel
> > > to move as it could stomp over other things we have loaded.
> > 
> > For kdump and kexec nokaslr is unnecessary. As you know we always
> > call add_buffer with buf_end as 1, this will cause kernel loaded at the
> > top of available memory. E.g on my pc with 16G memory, kexec kernel will
> > be put nearby 16G, so no random location choosing happen as I said in
> > above. For kdump, if reserved memory is at 500M~700M, then kernel will
> > be put nearby 700M, the random location choosing also never happen.
> > 
> > In fact, for some cases I need change kexec-tools user app code, to make
> > kernel be put from down to top.
> 
> I think we can't rely on where exactly in memory kexec-tools places the
> kernel. For kdump case we will have to pass nokaslr to make sure that
> kaslr does not move kernel.

In fact with this fix, it still works though kdump kernel is relocated
if kdump kernel is put in a low addr of reserved memory. But I am fine
with it that adding nokaslr to make it safer.

> 
> Thanks
> Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/10/14 at 11:05am, Vivek Goyal wrote:
> On Wed, Sep 10, 2014 at 07:41:38AM -0700, Kees Cook wrote:
> > On Wed, Sep 10, 2014 at 7:30 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > So I would suggest that test and repost the other patch with proper changelog
> > > and that might be sufficient for now. Only other thing we will need is
> > > Kees's patch for avoiding setup data regions in kaslr.
> > 
> > If someone can confirm that my patch works, I can request x86 pull it.
> > 
> > Currently living here:
> > https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr
> > 
> 
> Bao,
> 
> Can you please test Kees's patch and provide feedback.

I can only use a trick by setting the E820_MAX to a very small number,
say 10 in kexec-tools. Then extra memory regions will be added into
setup data. Sufficient real test can't be taken by me since lack of
machine.

Chao said someone has this kind of machine and help him test his
kexec-tools patches, I will ask him for help to test Kees's patch.

> 
> Thanks
> Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Vivek Goyal]

On Wed, Sep 10, 2014 at 11:27:16PM +0800, Baoquan He wrote:
> On 09/10/14 at 11:05am, Vivek Goyal wrote:
> > On Wed, Sep 10, 2014 at 07:41:38AM -0700, Kees Cook wrote:
> > > On Wed, Sep 10, 2014 at 7:30 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > So I would suggest that test and repost the other patch with proper changelog
> > > > and that might be sufficient for now. Only other thing we will need is
> > > > Kees's patch for avoiding setup data regions in kaslr.
> > > 
> > > If someone can confirm that my patch works, I can request x86 pull it.
> > > 
> > > Currently living here:
> > > https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr
> > > 
> > 
> > Bao,
> > 
> > Can you please test Kees's patch and provide feedback.
> 
> I can only use a trick by setting the E820_MAX to a very small number,
> say 10 in kexec-tools. Then extra memory regions will be added into
> setup data. Sufficient real test can't be taken by me since lack of
> machine.

I think modifying kexec-tools test should be good. Also we prepare
an kexec specific data blob for EFI machines and pass to second
kernel through setup_data. We can put some printk and make sure
this patch avoids that region. 

Thanks
Vivek

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Baoquan He]

On 09/10/14 at 07:41am, Kees Cook wrote:
> On Wed, Sep 10, 2014 at 7:30 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > So I would suggest that test and repost the other patch with proper changelog
> > and that might be sufficient for now. Only other thing we will need is
> > Kees's patch for avoiding setup data regions in kaslr.
> 
> If someone can confirm that my patch works, I can request x86 pull it.
> 
> Currently living here:
> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr

Hi Kees,

I tested your patch, it works. The way is like below:

I changed kexec-tools to make all add_buffer doing from down to top.
Then all stuff including kernel, initrd, setupda, cmdline, realmode_data
all are put inside 0~1G. And set E820_MAX to 10, since on my pc there are 17
memoery regions, then 7 memory regions are added into setup_data as a
blob. Then copy arch/x86/boot/printf.c to arch/x86/boot/compressed/ and
make it work. From printing message, the setup data is checking each
time when a memory slot is added.

> 
> Thanks!
> 
> -Kees
> 
> -- 
> Kees Cook
> Chrome OS Security

Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

[Kees Cook]

On Thu, Sep 11, 2014 at 2:31 AM, Baoquan He <bhe@redhat.com> wrote:
> On 09/10/14 at 07:41am, Kees Cook wrote:
>> On Wed, Sep 10, 2014 at 7:30 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > So I would suggest that test and repost the other patch with proper changelog
>> > and that might be sufficient for now. Only other thing we will need is
>> > Kees's patch for avoiding setup data regions in kaslr.
>>
>> If someone can confirm that my patch works, I can request x86 pull it.
>>
>> Currently living here:
>> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=kaslr
>
> Hi Kees,
>
> I tested your patch, it works. The way is like below:
>
> I changed kexec-tools to make all add_buffer doing from down to top.
> Then all stuff including kernel, initrd, setupda, cmdline, realmode_data
> all are put inside 0~1G. And set E820_MAX to 10, since on my pc there are 17
> memoery regions, then 7 memory regions are added into setup_data as a
> blob. Then copy arch/x86/boot/printf.c to arch/x86/boot/compressed/ and
> make it work. From printing message, the setup data is checking each
> time when a memory slot is added.

Great! Thanks very much! I'll send the patch to Peter now.

-Kees

-- 
Kees Cook
Chrome OS Security