[resend Patch v3 1/2] kaslr: check if kernel location is changed

[resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

Function handle_relocations() is used to do the relocations handling
for i686 and kaslr of x86_64. For 32 bit the relocation handling is
mandotary to perform. For x86_64 only when kaslr is enabled and a
random kernel location is chosen successfully the relocation handling
shound be done. However previous implementation only compared the
kernel loading address and LOAD_PHYSICAL_ADDR where kernel were
compiled to run at. This would casue system to be exceptional in
few conditions like when delta between load address and compiled
address is bigger than what 32bit signed relocations can handle.
Also there will be limitations that delta can't be too big otherwise
kernel text virtual addresses will overflow in module address space.

So in this patch check if kernel location is changed after
choose_kernel_location() when x86_64. If and only if in x86_64
and kernel location is changed, we say a kaslr random kernel
location is chosen, then the relocation handling is needed.

Signed-off-by: Baoquan He <bhe@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Acked-by: Kees Cook <keescook@chromium.org>
Tested-by: Thomas D. <whissi@whissi.de>
Cc: stable@vger.kernel.org
---
 arch/x86/boot/compressed/misc.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 57ab74d..3bb2a17 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;
@@ -239,6 +240,20 @@ static void handle_relocations(void *output, unsigned long output_len)
 	unsigned long max_addr = min_addr + output_len;
 
 	/*
+	* 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.
+	*/
+#if CONFIG_X86_64
+	if (output_orig == output)
+		return;
+#endif
+
+	/*
 	 * Calculate the delta between where vmlinux was linked to load
 	 * and where it was actually loaded.
 	 */
@@ -299,7 +314,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 +376,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 +420,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

[resend Patch v3 2/2] 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>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Vivek Goyal <vgoyal@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: [resend Patch v3 2/2] export the kernel image size KERNEL_IMAGE_SIZE

[Baoquan He]

Hi Thomas,

Could you please also merge this patch? Since you have merged this patch
"x86, boot: Skip relocs when load address unchanged [commit f285f4a21]",
and this issue was raised because it broke kexec/kdump, then I posted
these 2 patches. Without this patch makedumpfile will be broken when
enable kaslr and do the kdump.

Thanks
Baouqan

On 09/30/2014 03:08 PM, 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>
> Acked-by: Kees Cook <keescook@chromium.org>
> Acked-by: Vivek Goyal <vgoyal@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
> 
	

Re: [resend Patch v3 2/2] export the kernel image size KERNEL_IMAGE_SIZE

[Kees Cook]

Ping?

Can someone pull this into x86 tip?

Thanks!

-Kees


On Sun, Feb 1, 2015 at 11:32 PM, Baoquan He <bhe@redhat.com> wrote:
>
> Hi Thomas,
>
> Could you please also merge this patch? Since you have merged this patch
> "x86, boot: Skip relocs when load address unchanged [commit f285f4a21]",
> and this issue was raised because it broke kexec/kdump, then I posted
> these 2 patches. Without this patch makedumpfile will be broken when
> enable kaslr and do the kdump.
>
> Thanks
> Baouqan
>
> On 09/30/2014 03:08 PM, 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>
>> Acked-by: Kees Cook <keescook@chromium.org>
>> Acked-by: Vivek Goyal <vgoyal@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
>>
>



-- 
Kees Cook
Chrome OS Security

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[H. Peter Anvin]

On 09/30/2014 12:08 AM, Baoquan He wrote:
> Function handle_relocations() is used to do the relocations handling
> for i686 and kaslr of x86_64. For 32 bit the relocation handling is
> mandotary to perform. For x86_64 only when kaslr is enabled and a
> random kernel location is chosen successfully the relocation handling
> shound be done. However previous implementation only compared the
> kernel loading address and LOAD_PHYSICAL_ADDR where kernel were
> compiled to run at. This would casue system to be exceptional in
> few conditions like when delta between load address and compiled
> address is bigger than what 32bit signed relocations can handle.
> Also there will be limitations that delta can't be too big otherwise
> kernel text virtual addresses will overflow in module address space.
> 
> So in this patch check if kernel location is changed after
> choose_kernel_location() when x86_64. If and only if in x86_64
> and kernel location is changed, we say a kaslr random kernel
> location is chosen, then the relocation handling is needed.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Acked-by: Kees Cook <keescook@chromium.org>
> Tested-by: Thomas D. <whissi@whissi.de>
> Cc: stable@vger.kernel.org

Could you clarify under what conditions we may end up with 32-bit signed
overflow, and yet have a functional kernel?

	-hpa

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Vivek Goyal]

On Tue, Sep 30, 2014 at 02:21:05PM -0700, H. Peter Anvin wrote:
> On 09/30/2014 12:08 AM, Baoquan He wrote:
> > Function handle_relocations() is used to do the relocations handling
> > for i686 and kaslr of x86_64. For 32 bit the relocation handling is
> > mandotary to perform. For x86_64 only when kaslr is enabled and a
> > random kernel location is chosen successfully the relocation handling
> > shound be done. However previous implementation only compared the
> > kernel loading address and LOAD_PHYSICAL_ADDR where kernel were
> > compiled to run at. This would casue system to be exceptional in
> > few conditions like when delta between load address and compiled
> > address is bigger than what 32bit signed relocations can handle.
> > Also there will be limitations that delta can't be too big otherwise
> > kernel text virtual addresses will overflow in module address space.
> > 
> > So in this patch check if kernel location is changed after
> > choose_kernel_location() when x86_64. If and only if in x86_64
> > and kernel location is changed, we say a kaslr random kernel
> > location is chosen, then the relocation handling is needed.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > Acked-by: Kees Cook <keescook@chromium.org>
> > Tested-by: Thomas D. <whissi@whissi.de>
> > Cc: stable@vger.kernel.org
> 
> Could you clarify under what conditions we may end up with 32-bit signed
> overflow, and yet have a functional kernel?


Hi Peter,

I think there is some confusion. I will try to clarify.

If we have 32bit signed overflow, we will not have a functional kernel.
And that's the message we get when we try to kexec with
CONFIG_RANDOMIZE_BASE=y.

**********************************************************************
[  340.709078] kexec: Starting new kernel
early console in decompress_kernel
KASLR disabled by default...

Decompressing Linux... Parsing ELF...

Performing relocations...
32-bit relocation outside of kernel!


 -- System halted
*****************************************************************

We realized that kexec tries to load kernel at higher physical addresses
and that can lead to problmes.

Currently for x86_64, handle_relocations() will perform relocations if
kernel is not loaded at LOAD_PHYSICAL_ADDR. I think this does not work for
all the cases and kerenl can not be loaded anywhere in the physical address
space. And that's why we run into issues with kexec.

My understanding is that we introduce handle_relcoations() i386 style
because of RANDOMIZE_BASE. If that's the case, one possible solution
is that perform relocations only if ranodmize base logic has chosen
a different load location for kernel than where boot loader loaded
it. Otherwise don't do anything.

In case of kexec/kdump, we will pass "nokaslr" to second kernel forcing
it to do nothing and let the kernel run where it was loaded by bootloader.
And in that case handle_relocations() should not do any relocations and
that should allow kernel to be loaded anywhere in physical memory on
x86_64.

Thanks
Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[H. Peter Anvin]

On 10/01/2014 06:52 AM, Vivek Goyal wrote:
> 
> Hi Peter,
> 
> I think there is some confusion. I will try to clarify.
> 
> If we have 32bit signed overflow, we will not have a functional kernel.
> And that's the message we get when we try to kexec with
> CONFIG_RANDOMIZE_BASE=y.
> 

And how does that happen?

> **********************************************************************
> [  340.709078] kexec: Starting new kernel
> early console in decompress_kernel
> KASLR disabled by default...
> 
> Decompressing Linux... Parsing ELF...
> 
> Performing relocations...
> 32-bit relocation outside of kernel!
> 
> 
>  -- System halted
> *****************************************************************
> 
> We realized that kexec tries to load kernel at higher physical addresses
> and that can lead to problmes.
> 
> Currently for x86_64, handle_relocations() will perform relocations if
> kernel is not loaded at LOAD_PHYSICAL_ADDR. I think this does not work for
> all the cases and kerenl can not be loaded anywhere in the physical address
> space. And that's why we run into issues with kexec.
> 
> My understanding is that we introduce handle_relcoations() i386 style
> because of RANDOMIZE_BASE. If that's the case, one possible solution
> is that perform relocations only if ranodmize base logic has chosen
> a different load location for kernel than where boot loader loaded
> it. Otherwise don't do anything.
> 
> In case of kexec/kdump, we will pass "nokaslr" to second kernel forcing
> it to do nothing and let the kernel run where it was loaded by bootloader.
> And in that case handle_relocations() should not do any relocations and
> that should allow kernel to be loaded anywhere in physical memory on
> x86_64.
> 

Sorry... this makes no sense.

For x86-64, there is no direct connection between the physical and
virtual address spaces that the kernel runs in...

	-hpa

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Vivek Goyal]

On Wed, Oct 08, 2014 at 08:09:59AM -0700, H. Peter Anvin wrote:
> On 10/01/2014 06:52 AM, Vivek Goyal wrote:
> > 
> > Hi Peter,
> > 
> > I think there is some confusion. I will try to clarify.
> > 
> > If we have 32bit signed overflow, we will not have a functional kernel.
> > And that's the message we get when we try to kexec with
> > CONFIG_RANDOMIZE_BASE=y.
> > 
> 
> And how does that happen?

I compile a kernel for physical address 16MB (CONFIG_PHYSICAL_START=0x1000000).
And kexec loads this kernel at physical address above 16GB (0x000000042e000000).

When we boot into second kernel it tries to perform relocations and fails
as follows. I have printed bunch of variables in handle_relocations(), so
referring to code will help.

min_addr=0x000000042e000000 (Physical address where decompressed kernel is
			     loaded).

delta=0x000000042d000000  (Difference between load and compile addr).

map=0x00000004ad000000	 (map = delta - __START_KERNEL_map)

Now we start processing 32bit relocations and read first reloc.

extended=0x81e819c2	(extended = *reloc)

We add map to it and new value of extended is.

extended=0x2ee819c2	(extended += map)

Now we convert this to actual 64bit address which relocation needs to be
applied and ptr value is.

ptr = 0x000000002ee819c2 (ptr = (unsigned long)extended;)

And this address is outside the range of where kernel is actually loaded.
(0x000000042e000000). So we ended up with a wrong address to patch hence
following check fails.

if (ptr < min_addr || ptr > max_addr)
	error("32-bit relocation outside of kernel!\n");


> 
> > **********************************************************************
> > [  340.709078] kexec: Starting new kernel
> > early console in decompress_kernel
> > KASLR disabled by default...
> > 
> > Decompressing Linux... Parsing ELF...
> > 
> > Performing relocations...
> > 32-bit relocation outside of kernel!
> > 
> > 
> >  -- System halted
> > *****************************************************************
> > 
> > We realized that kexec tries to load kernel at higher physical addresses
> > and that can lead to problmes.
> > 
> > Currently for x86_64, handle_relocations() will perform relocations if
> > kernel is not loaded at LOAD_PHYSICAL_ADDR. I think this does not work for
> > all the cases and kerenl can not be loaded anywhere in the physical address
> > space. And that's why we run into issues with kexec.
> > 
> > My understanding is that we introduce handle_relcoations() i386 style
> > because of RANDOMIZE_BASE. If that's the case, one possible solution
> > is that perform relocations only if ranodmize base logic has chosen
> > a different load location for kernel than where boot loader loaded
> > it. Otherwise don't do anything.
> > 
> > In case of kexec/kdump, we will pass "nokaslr" to second kernel forcing
> > it to do nothing and let the kernel run where it was loaded by bootloader.
> > And in that case handle_relocations() should not do any relocations and
> > that should allow kernel to be loaded anywhere in physical memory on
> > x86_64.
> > 
> 
> Sorry... this makes no sense.
> 
> For x86-64, there is no direct connection between the physical and
> virtual address spaces that the kernel runs in...

I am sorry I did not understand this one. I thought that initial
relocatable kernel implementaion did not have any direct connection
between virtual and physical address. One could load kernel anywhere
and kernel virtual address will not change and we will just adjust
page tables to map virtual address to right physical address.

Now handle_relocation() stuff seems to introduce a close coupling
between physical and virtual address. So if kernel shifts by 16MB
in physical address space, then it will shift by equal amount
in virtual address space. So there seems to be a direct connection
between virtual and physical address space in this case.

What am I missing?

Thanks
Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 10/08/14 at 03:27pm, Vivek Goyal wrote:
> On Wed, Oct 08, 2014 at 08:09:59AM -0700, H. Peter Anvin wrote:
 
> > Sorry... this makes no sense.
> > 
> > For x86-64, there is no direct connection between the physical and
> > virtual address spaces that the kernel runs in...
> 
> I am sorry I did not understand this one. I thought that initial
> relocatable kernel implementaion did not have any direct connection
> between virtual and physical address. One could load kernel anywhere
> and kernel virtual address will not change and we will just adjust
> page tables to map virtual address to right physical address.
> 
> Now handle_relocation() stuff seems to introduce a close coupling
> between physical and virtual address. So if kernel shifts by 16MB
> in physical address space, then it will shift by equal amount
> in virtual address space. So there seems to be a direct connection
> between virtual and physical address space in this case.

Yeah, it's exactly as Vivek said.

Before kaslr was introduced, x86_64 kernel can be put anywhere, and
always _text is 0xffffffff81000000. Meanwhile phys_base contains the
offset between the compiled addr (namely 0x1000000) and kernel loaded
addr. After kaslr implementation was added, as long as kernel loaded
addr is different 0x1000000, it will call handle_relocations(). The
offset now is added onto each symbols including _text and phys_base
becomes 0.

It's clearly showing that by checking /proc/kallsyms and value of
phys_base.


Thanks
Baoquan

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[H. Peter Anvin]

On 10/10/2014 08:14 PM, Baoquan He wrote:
> On 10/08/14 at 03:27pm, Vivek Goyal wrote:
>> On Wed, Oct 08, 2014 at 08:09:59AM -0700, H. Peter Anvin wrote:
>
>>> Sorry... this makes no sense.
>>>
>>> For x86-64, there is no direct connection between the physical and
>>> virtual address spaces that the kernel runs in...
>>
>> I am sorry I did not understand this one. I thought that initial
>> relocatable kernel implementaion did not have any direct connection
>> between virtual and physical address. One could load kernel anywhere
>> and kernel virtual address will not change and we will just adjust
>> page tables to map virtual address to right physical address.
>>
>> Now handle_relocation() stuff seems to introduce a close coupling
>> between physical and virtual address. So if kernel shifts by 16MB
>> in physical address space, then it will shift by equal amount
>> in virtual address space. So there seems to be a direct connection
>> between virtual and physical address space in this case.
>
> Yeah, it's exactly as Vivek said.
>
> Before kaslr was introduced, x86_64 kernel can be put anywhere, and
> always _text is 0xffffffff81000000. Meanwhile phys_base contains the
> offset between the compiled addr (namely 0x1000000) and kernel loaded
> addr. After kaslr implementation was added, as long as kernel loaded
> addr is different 0x1000000, it will call handle_relocations(). The
> offset now is added onto each symbols including _text and phys_base
> becomes 0.
>
> It's clearly showing that by checking /proc/kallsyms and value of
> phys_base.
>

This really shouldn't have happened this way on x86-64.  It has to 
happen this way on i386, but I worry that this may be a serious 
misdesign in kaslr on x86-64.  I'm also wondering if there is any other 
fallout of this?

	-hpa

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 10/11/14 at 03:34am, H. Peter Anvin wrote:
> On 10/10/2014 08:14 PM, Baoquan He wrote:
> >On 10/08/14 at 03:27pm, Vivek Goyal wrote:
> >>On Wed, Oct 08, 2014 at 08:09:59AM -0700, H. Peter Anvin wrote:
> >
> >>>Sorry... this makes no sense.
> >>>
> >>>For x86-64, there is no direct connection between the physical and
> >>>virtual address spaces that the kernel runs in...
> >>
> >>I am sorry I did not understand this one. I thought that initial
> >>relocatable kernel implementaion did not have any direct connection
> >>between virtual and physical address. One could load kernel anywhere
> >>and kernel virtual address will not change and we will just adjust
> >>page tables to map virtual address to right physical address.
> >>
> >>Now handle_relocation() stuff seems to introduce a close coupling
> >>between physical and virtual address. So if kernel shifts by 16MB
> >>in physical address space, then it will shift by equal amount
> >>in virtual address space. So there seems to be a direct connection
> >>between virtual and physical address space in this case.
> >
> >Yeah, it's exactly as Vivek said.
> >
> >Before kaslr was introduced, x86_64 kernel can be put anywhere, and
> >always _text is 0xffffffff81000000. Meanwhile phys_base contains the
> >offset between the compiled addr (namely 0x1000000) and kernel loaded
> >addr. After kaslr implementation was added, as long as kernel loaded
> >addr is different 0x1000000, it will call handle_relocations(). The
> >offset now is added onto each symbols including _text and phys_base
> >becomes 0.
> >
> >It's clearly showing that by checking /proc/kallsyms and value of
> >phys_base.
> >
> 
> This really shouldn't have happened this way on x86-64.  It has to
> happen this way on i386, but I worry that this may be a serious
> misdesign in kaslr on x86-64.  I'm also wondering if there is any
> other fallout of this?

Yes, this shouldn't happen this way on x86_64. With this patch, those
are fixed as expected. If kernel location is not chosen randomly, we
should not do the relocations handling. If and only if kaslr is enabled
and it relocated the kernel randomly as expected, we do the relocations
handling.

I think this patch really makes sense and it's simple and won't impact
i386 and other implementations.

Thanks
Baoquan

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 10/11/14 at 08:38pm, Baoquan He wrote:
> On 10/11/14 at 03:34am, H. Peter Anvin wrote:
> > On 10/10/2014 08:14 PM, Baoquan He wrote:
> > >On 10/08/14 at 03:27pm, Vivek Goyal wrote:
> > >>On Wed, Oct 08, 2014 at 08:09:59AM -0700, H. Peter Anvin wrote:
> > >
> > >>>Sorry... this makes no sense.
> > >>>
> > >>>For x86-64, there is no direct connection between the physical and
> > >>>virtual address spaces that the kernel runs in...
> > >>
> > >>I am sorry I did not understand this one. I thought that initial
> > >>relocatable kernel implementaion did not have any direct connection
> > >>between virtual and physical address. One could load kernel anywhere
> > >>and kernel virtual address will not change and we will just adjust
> > >>page tables to map virtual address to right physical address.
> > >>
> > >>Now handle_relocation() stuff seems to introduce a close coupling
> > >>between physical and virtual address. So if kernel shifts by 16MB
> > >>in physical address space, then it will shift by equal amount
> > >>in virtual address space. So there seems to be a direct connection
> > >>between virtual and physical address space in this case.
> > >
> > >Yeah, it's exactly as Vivek said.
> > >
> > >Before kaslr was introduced, x86_64 kernel can be put anywhere, and
> > >always _text is 0xffffffff81000000. Meanwhile phys_base contains the
> > >offset between the compiled addr (namely 0x1000000) and kernel loaded
> > >addr. After kaslr implementation was added, as long as kernel loaded
> > >addr is different 0x1000000, it will call handle_relocations(). The
> > >offset now is added onto each symbols including _text and phys_base
> > >becomes 0.
> > >
> > >It's clearly showing that by checking /proc/kallsyms and value of
> > >phys_base.
> > >
> > 
> > This really shouldn't have happened this way on x86-64.  It has to
> > happen this way on i386, but I worry that this may be a serious
> > misdesign in kaslr on x86-64.  I'm also wondering if there is any
> > other fallout of this?

Btw, except of this bug, I didn't find other risk of kaslr currently.
The code flow is straightforward and clear.

> 
> Yes, this shouldn't happen this way on x86_64. With this patch, those
> are fixed as expected. If kernel location is not chosen randomly, we
> should not do the relocations handling. If and only if kaslr is enabled
> and it relocated the kernel randomly as expected, we do the relocations
> handling.
> 
> I think this patch really makes sense and it's simple and won't impact
> i386 and other implementations.
> 

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Vivek Goyal]

On Sat, Oct 11, 2014 at 03:34:29AM -0700, H. Peter Anvin wrote:
> On 10/10/2014 08:14 PM, Baoquan He wrote:
> >On 10/08/14 at 03:27pm, Vivek Goyal wrote:
> >>On Wed, Oct 08, 2014 at 08:09:59AM -0700, H. Peter Anvin wrote:
> >
> >>>Sorry... this makes no sense.
> >>>
> >>>For x86-64, there is no direct connection between the physical and
> >>>virtual address spaces that the kernel runs in...
> >>
> >>I am sorry I did not understand this one. I thought that initial
> >>relocatable kernel implementaion did not have any direct connection
> >>between virtual and physical address. One could load kernel anywhere
> >>and kernel virtual address will not change and we will just adjust
> >>page tables to map virtual address to right physical address.
> >>
> >>Now handle_relocation() stuff seems to introduce a close coupling
> >>between physical and virtual address. So if kernel shifts by 16MB
> >>in physical address space, then it will shift by equal amount
> >>in virtual address space. So there seems to be a direct connection
> >>between virtual and physical address space in this case.
> >
> >Yeah, it's exactly as Vivek said.
> >
> >Before kaslr was introduced, x86_64 kernel can be put anywhere, and
> >always _text is 0xffffffff81000000. Meanwhile phys_base contains the
> >offset between the compiled addr (namely 0x1000000) and kernel loaded
> >addr. After kaslr implementation was added, as long as kernel loaded
> >addr is different 0x1000000, it will call handle_relocations(). The
> >offset now is added onto each symbols including _text and phys_base
> >becomes 0.
> >
> >It's clearly showing that by checking /proc/kallsyms and value of
> >phys_base.
> >
> 
> This really shouldn't have happened this way on x86-64.  It has to happen
> this way on i386, but I worry that this may be a serious misdesign in kaslr
> on x86-64.  I'm also wondering if there is any other fallout of this?

I agree. On x86_64, we should stick to previous design and this new
logic of performing relocations does not sound very clean and makes
things very confusing.

I am wondering that why couldn't we simply adjust page tables in case of
kaslr on x86_64, instead of performing relocations.

Thanks
Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Vivek Goyal]

On Mon, Oct 13, 2014 at 08:52:57AM -0400, Vivek Goyal wrote:
> On Sat, Oct 11, 2014 at 03:34:29AM -0700, H. Peter Anvin wrote:
> > On 10/10/2014 08:14 PM, Baoquan He wrote:
> > >On 10/08/14 at 03:27pm, Vivek Goyal wrote:
> > >>On Wed, Oct 08, 2014 at 08:09:59AM -0700, H. Peter Anvin wrote:
> > >
> > >>>Sorry... this makes no sense.
> > >>>
> > >>>For x86-64, there is no direct connection between the physical and
> > >>>virtual address spaces that the kernel runs in...
> > >>
> > >>I am sorry I did not understand this one. I thought that initial
> > >>relocatable kernel implementaion did not have any direct connection
> > >>between virtual and physical address. One could load kernel anywhere
> > >>and kernel virtual address will not change and we will just adjust
> > >>page tables to map virtual address to right physical address.
> > >>
> > >>Now handle_relocation() stuff seems to introduce a close coupling
> > >>between physical and virtual address. So if kernel shifts by 16MB
> > >>in physical address space, then it will shift by equal amount
> > >>in virtual address space. So there seems to be a direct connection
> > >>between virtual and physical address space in this case.
> > >
> > >Yeah, it's exactly as Vivek said.
> > >
> > >Before kaslr was introduced, x86_64 kernel can be put anywhere, and
> > >always _text is 0xffffffff81000000. Meanwhile phys_base contains the
> > >offset between the compiled addr (namely 0x1000000) and kernel loaded
> > >addr. After kaslr implementation was added, as long as kernel loaded
> > >addr is different 0x1000000, it will call handle_relocations(). The
> > >offset now is added onto each symbols including _text and phys_base
> > >becomes 0.
> > >
> > >It's clearly showing that by checking /proc/kallsyms and value of
> > >phys_base.
> > >
> > 
> > This really shouldn't have happened this way on x86-64.  It has to happen
> > this way on i386, but I worry that this may be a serious misdesign in kaslr
> > on x86-64.  I'm also wondering if there is any other fallout of this?
> 
> I agree. On x86_64, we should stick to previous design and this new
> logic of performing relocations does not sound very clean and makes
> things very confusing.
> 
> I am wondering that why couldn't we simply adjust page tables in case of
> kaslr on x86_64, instead of performing relocations.

Well, IIUC, if virtual addresses are shifted w.r.t what virtual address
kernel was compiled for, then relocation will have to be done.

So question will be if physical address shift is enough for kaslr or
virtual address shift is necessary.

Thanks
Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[H. Peter Anvin]

On 10/13/2014 08:19 AM, Vivek Goyal wrote:
>>>
>>> This really shouldn't have happened this way on x86-64.  It has to happen
>>> this way on i386, but I worry that this may be a serious misdesign in kaslr
>>> on x86-64.  I'm also wondering if there is any other fallout of this?
>>
>> I agree. On x86_64, we should stick to previous design and this new
>> logic of performing relocations does not sound very clean and makes
>> things very confusing.
>>
>> I am wondering that why couldn't we simply adjust page tables in case of
>> kaslr on x86_64, instead of performing relocations.
> 
> Well, IIUC, if virtual addresses are shifted w.r.t what virtual address
> kernel was compiled for, then relocation will have to be done.
> 
> So question will be if physical address shift is enough for kaslr or
> virtual address shift is necessary.
> 

I would assume that without a virtual address shift kaslr is pretty darn
pointless.  Without the physical address shift the 1:1 map can be used,
and again, kaslr becomes pointless.  However, there is absolutely no
reason why they should be coupled.  They can, in fact, be independently
randomized.

	-hpa

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Vivek Goyal]

On Mon, Oct 13, 2014 at 08:43:00AM -0700, H. Peter Anvin wrote:
> On 10/13/2014 08:19 AM, Vivek Goyal wrote:
> >>>
> >>> This really shouldn't have happened this way on x86-64.  It has to happen
> >>> this way on i386, but I worry that this may be a serious misdesign in kaslr
> >>> on x86-64.  I'm also wondering if there is any other fallout of this?
> >>
> >> I agree. On x86_64, we should stick to previous design and this new
> >> logic of performing relocations does not sound very clean and makes
> >> things very confusing.
> >>
> >> I am wondering that why couldn't we simply adjust page tables in case of
> >> kaslr on x86_64, instead of performing relocations.
> > 
> > Well, IIUC, if virtual addresses are shifted w.r.t what virtual address
> > kernel was compiled for, then relocation will have to be done.
> > 
> > So question will be if physical address shift is enough for kaslr or
> > virtual address shift is necessary.
> > 
> 
> I would assume that without a virtual address shift kaslr is pretty darn
> pointless.  Without the physical address shift the 1:1 map can be used,
> and again, kaslr becomes pointless.  However, there is absolutely no
> reason why they should be coupled.  They can, in fact, be independently
> randomized.

Agreed. On x86_64, we should be able to randomize virtual address space
and physical address space independently. And in that case whole of
the physical memory should be available for a possible location for
kernel. (As opposed to a small limit (I guess 1GB) now)

Thanks
Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Vivek Goyal]

On Mon, Oct 13, 2014 at 01:22:42PM -0400, Vivek Goyal wrote:
> On Mon, Oct 13, 2014 at 08:43:00AM -0700, H. Peter Anvin wrote:
> > On 10/13/2014 08:19 AM, Vivek Goyal wrote:
> > >>>
> > >>> This really shouldn't have happened this way on x86-64.  It has to happen
> > >>> this way on i386, but I worry that this may be a serious misdesign in kaslr
> > >>> on x86-64.  I'm also wondering if there is any other fallout of this?
> > >>
> > >> I agree. On x86_64, we should stick to previous design and this new
> > >> logic of performing relocations does not sound very clean and makes
> > >> things very confusing.
> > >>
> > >> I am wondering that why couldn't we simply adjust page tables in case of
> > >> kaslr on x86_64, instead of performing relocations.
> > > 
> > > Well, IIUC, if virtual addresses are shifted w.r.t what virtual address
> > > kernel was compiled for, then relocation will have to be done.
> > > 
> > > So question will be if physical address shift is enough for kaslr or
> > > virtual address shift is necessary.
> > > 
> > 
> > I would assume that without a virtual address shift kaslr is pretty darn
> > pointless.  Without the physical address shift the 1:1 map can be used,
> > and again, kaslr becomes pointless.  However, there is absolutely no
> > reason why they should be coupled.  They can, in fact, be independently
> > randomized.
> 
> Agreed. On x86_64, we should be able to randomize virtual address space
> and physical address space independently. And in that case whole of
> the physical memory should be available for a possible location for
> kernel. (As opposed to a small limit (I guess 1GB) now)

Hi Peter,

So what do we do about this issue in short term to make kexec work. Even
if we go for above solution, to make kexec work we will have to pass
"nokaslr" as we don't want kernel to move around in physical address space
as it might stomp over ELF headers we have stored.

If you don't like current patch, should we just disable relocations in
x86_64 if "nokaslr" command line is passed. That way kernel will not
be moved in physical as well as virtual address space.

Thanks
Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 10/14/14 at 08:49am, Vivek Goyal wrote:
> On Mon, Oct 13, 2014 at 01:22:42PM -0400, Vivek Goyal wrote:
> > On Mon, Oct 13, 2014 at 08:43:00AM -0700, H. Peter Anvin wrote:
> > > On 10/13/2014 08:19 AM, Vivek Goyal wrote:
> > > >>>
> > > >>> This really shouldn't have happened this way on x86-64.  It has to happen
> > > >>> this way on i386, but I worry that this may be a serious misdesign in kaslr
> > > >>> on x86-64.  I'm also wondering if there is any other fallout of this?
> > > >>
> > > >> I agree. On x86_64, we should stick to previous design and this new
> > > >> logic of performing relocations does not sound very clean and makes
> > > >> things very confusing.
> > > >>
> > > >> I am wondering that why couldn't we simply adjust page tables in case of
> > > >> kaslr on x86_64, instead of performing relocations.
> > > > 
> > > > Well, IIUC, if virtual addresses are shifted w.r.t what virtual address
> > > > kernel was compiled for, then relocation will have to be done.
> > > > 
> > > > So question will be if physical address shift is enough for kaslr or
> > > > virtual address shift is necessary.
> > > > 
> > > 
> > > I would assume that without a virtual address shift kaslr is pretty darn
> > > pointless.  Without the physical address shift the 1:1 map can be used,
> > > and again, kaslr becomes pointless.  However, there is absolutely no
> > > reason why they should be coupled.  They can, in fact, be independently
> > > randomized.
> > 
> > Agreed. On x86_64, we should be able to randomize virtual address space
> > and physical address space independently. And in that case whole of
> > the physical memory should be available for a possible location for
> > kernel. (As opposed to a small limit (I guess 1GB) now)

It can be done to randomize virtual address space and physical address
space independently. But limited by the 2G of kernel text mapping and
module mapping virtual address space, virtual address can be randomized
in (0x1000000, 1G) range. While physical address can be randomized in
(0x1000000, 4G) according to the identity mapping of normal kernel. Then
phys_base still stores an relative value, a different offset than before.

This can be easily implement. One thing is still there's a limit for
physical addr randomization, only below 4G. So I am wondering if we can
extend the identify mapping to complete mapping of 48 bit, using 1G page
frame. This can make physical addr be randomized to anywhere.

So now there may be 3 options:

1) Fix this bug in current kaslr. Since when randomize the new kernel
location in choose_kernel_location(), cmdline options has been checked
strictly, e.g if nokaslr is specified, it's safe to do the kernel
location randomization. Then in handle_relocations(), we only need to
check if the kernel location is changed, comparing with kernel loaded
addr. If changed, kaslr is done, let's do the relocation handling. If
not changed, no kaslr id done, just skip the relocation handling like
before.

2) randomize the virtual addr space and physical addr space
independently. But physical addr space must be below 4G.

3) extend the identity mapping to 48bit of addr space. Then we can
randomized the virtual addr space in (0x1000000, 1G) and physical addr
space in (0x1000000, real physical memory end).

If option 3 is doable, it's the best. If not, I think bug fix should be
better.

> 
> Hi Peter,
> 
> So what do we do about this issue in short term to make kexec work. Even
> if we go for above solution, to make kexec work we will have to pass
> "nokaslr" as we don't want kernel to move around in physical address space
> as it might stomp over ELF headers we have stored.

kexec doesn't need ELF headers. Kdump may need it. But in current
kexec-tools implementation, kernel/initrd and other stuffs are placed
from top to down, current implementation won't do kaslr since it only
happened between kernel loaded addr and 1G. So we don't need to worry
about the stomping.

> 
> If you don't like current patch, should we just disable relocations in
> x86_64 if "nokaslr" command line is passed. That way kernel will not
> be moved in physical as well as virtual address space.
> 
> Thanks
> Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Vivek Goyal]

On Wed, Oct 15, 2014 at 11:37:01AM +0800, Baoquan He wrote:
> On 10/14/14 at 08:49am, Vivek Goyal wrote:
> > On Mon, Oct 13, 2014 at 01:22:42PM -0400, Vivek Goyal wrote:
> > > On Mon, Oct 13, 2014 at 08:43:00AM -0700, H. Peter Anvin wrote:
> > > > On 10/13/2014 08:19 AM, Vivek Goyal wrote:
> > > > >>>
> > > > >>> This really shouldn't have happened this way on x86-64.  It has to happen
> > > > >>> this way on i386, but I worry that this may be a serious misdesign in kaslr
> > > > >>> on x86-64.  I'm also wondering if there is any other fallout of this?
> > > > >>
> > > > >> I agree. On x86_64, we should stick to previous design and this new
> > > > >> logic of performing relocations does not sound very clean and makes
> > > > >> things very confusing.
> > > > >>
> > > > >> I am wondering that why couldn't we simply adjust page tables in case of
> > > > >> kaslr on x86_64, instead of performing relocations.
> > > > > 
> > > > > Well, IIUC, if virtual addresses are shifted w.r.t what virtual address
> > > > > kernel was compiled for, then relocation will have to be done.
> > > > > 
> > > > > So question will be if physical address shift is enough for kaslr or
> > > > > virtual address shift is necessary.
> > > > > 
> > > > 
> > > > I would assume that without a virtual address shift kaslr is pretty darn
> > > > pointless.  Without the physical address shift the 1:1 map can be used,
> > > > and again, kaslr becomes pointless.  However, there is absolutely no
> > > > reason why they should be coupled.  They can, in fact, be independently
> > > > randomized.
> > > 
> > > Agreed. On x86_64, we should be able to randomize virtual address space
> > > and physical address space independently. And in that case whole of
> > > the physical memory should be available for a possible location for
> > > kernel. (As opposed to a small limit (I guess 1GB) now)
> 
> It can be done to randomize virtual address space and physical address
> space independently. But limited by the 2G of kernel text mapping and
> module mapping virtual address space, virtual address can be randomized
> in (0x1000000, 1G) range. While physical address can be randomized in
> (0x1000000, 4G) according to the identity mapping of normal kernel. Then
> phys_base still stores an relative value, a different offset than before.
> 
> This can be easily implement. One thing is still there's a limit for
> physical addr randomization, only below 4G. So I am wondering if we can
> extend the identify mapping to complete mapping of 48 bit, using 1G page
> frame. This can make physical addr be randomized to anywhere.

I am wondering where does this 4G limit come from? Without kaslr now
we are able to load kernels much higher than 4G. That would suggest
that we should be able to pick randomly any address above 4G too?

Thanks
Vivek

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[H. Peter Anvin]

I don't see why we can't randomize anywhere in physical space.  We already handle the kernel above 4 GB and it wouldn't be hard to do the equivalent for the decompress/relocation code, using a #PF handler.  Not all CPUs support 1 GB pages.

On October 14, 2014 8:37:01 PM PDT, Baoquan He <bhe@redhat.com> wrote:
>On 10/14/14 at 08:49am, Vivek Goyal wrote:
>> On Mon, Oct 13, 2014 at 01:22:42PM -0400, Vivek Goyal wrote:
>> > On Mon, Oct 13, 2014 at 08:43:00AM -0700, H. Peter Anvin wrote:
>> > > On 10/13/2014 08:19 AM, Vivek Goyal wrote:
>> > > >>>
>> > > >>> This really shouldn't have happened this way on x86-64.  It
>has to happen
>> > > >>> this way on i386, but I worry that this may be a serious
>misdesign in kaslr
>> > > >>> on x86-64.  I'm also wondering if there is any other fallout
>of this?
>> > > >>
>> > > >> I agree. On x86_64, we should stick to previous design and
>this new
>> > > >> logic of performing relocations does not sound very clean and
>makes
>> > > >> things very confusing.
>> > > >>
>> > > >> I am wondering that why couldn't we simply adjust page tables
>in case of
>> > > >> kaslr on x86_64, instead of performing relocations.
>> > > > 
>> > > > Well, IIUC, if virtual addresses are shifted w.r.t what virtual
>address
>> > > > kernel was compiled for, then relocation will have to be done.
>> > > > 
>> > > > So question will be if physical address shift is enough for
>kaslr or
>> > > > virtual address shift is necessary.
>> > > > 
>> > > 
>> > > I would assume that without a virtual address shift kaslr is
>pretty darn
>> > > pointless.  Without the physical address shift the 1:1 map can be
>used,
>> > > and again, kaslr becomes pointless.  However, there is absolutely
>no
>> > > reason why they should be coupled.  They can, in fact, be
>independently
>> > > randomized.
>> > 
>> > Agreed. On x86_64, we should be able to randomize virtual address
>space
>> > and physical address space independently. And in that case whole of
>> > the physical memory should be available for a possible location for
>> > kernel. (As opposed to a small limit (I guess 1GB) now)
>
>It can be done to randomize virtual address space and physical address
>space independently. But limited by the 2G of kernel text mapping and
>module mapping virtual address space, virtual address can be randomized
>in (0x1000000, 1G) range. While physical address can be randomized in
>(0x1000000, 4G) according to the identity mapping of normal kernel.
>Then
>phys_base still stores an relative value, a different offset than
>before.
>
>This can be easily implement. One thing is still there's a limit for
>physical addr randomization, only below 4G. So I am wondering if we can
>extend the identify mapping to complete mapping of 48 bit, using 1G
>page
>frame. This can make physical addr be randomized to anywhere.
>
>So now there may be 3 options:
>
>1) Fix this bug in current kaslr. Since when randomize the new kernel
>location in choose_kernel_location(), cmdline options has been checked
>strictly, e.g if nokaslr is specified, it's safe to do the kernel
>location randomization. Then in handle_relocations(), we only need to
>check if the kernel location is changed, comparing with kernel loaded
>addr. If changed, kaslr is done, let's do the relocation handling. If
>not changed, no kaslr id done, just skip the relocation handling like
>before.
>
>2) randomize the virtual addr space and physical addr space
>independently. But physical addr space must be below 4G.
>
>3) extend the identity mapping to 48bit of addr space. Then we can
>randomized the virtual addr space in (0x1000000, 1G) and physical addr
>space in (0x1000000, real physical memory end).
>
>If option 3 is doable, it's the best. If not, I think bug fix should be
>better.
>
>> 
>> Hi Peter,
>> 
>> So what do we do about this issue in short term to make kexec work.
>Even
>> if we go for above solution, to make kexec work we will have to pass
>> "nokaslr" as we don't want kernel to move around in physical address
>space
>> as it might stomp over ELF headers we have stored.
>
>kexec doesn't need ELF headers. Kdump may need it. But in current
>kexec-tools implementation, kernel/initrd and other stuffs are placed
>from top to down, current implementation won't do kaslr since it only
>happened between kernel loaded addr and 1G. So we don't need to worry
>about the stomping.
>
>> 
>> If you don't like current patch, should we just disable relocations
>in
>> x86_64 if "nokaslr" command line is passed. That way kernel will not
>> be moved in physical as well as virtual address space.
>> 
>> Thanks
>> Vivek

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

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 10/15/14 at 01:32pm, H. Peter Anvin wrote:
> I don't see why we can't randomize anywhere in physical space.  We already handle the kernel above 4 GB and it wouldn't be hard to do the equivalent for the decompress/relocation code, using a #PF handler.  Not all CPUs support 1 GB pages.
> 

Well, IIUC, in arch/x86/boot/compressed/head_64.S pgtable contains 6
page frames and is used to build 4G identity mapping. Then this pgtable
is loaded into cr3. Later jump into 64bit mode. decompress/relocation
code should work in this region. Isn't it right?

Currently kernel surely can be put anywhere, even above 4G. This have
been done very often in kexec/kdump. However in kexec/kdump the identity
mapping is built for all physical memory after jumping to startup_64
directly from purgatory, that's why kexec/kdump kernel can be put
anywhere.

So for normal kernel, don't we need to extend the physical memory mappng
to a larger region if we want to randomize anywhere in physical space?

Thanks
Baoquan


> On October 14, 2014 8:37:01 PM PDT, Baoquan He <bhe@redhat.com> wrote:
> >On 10/14/14 at 08:49am, Vivek Goyal wrote:
> >> On Mon, Oct 13, 2014 at 01:22:42PM -0400, Vivek Goyal wrote:
> >> > On Mon, Oct 13, 2014 at 08:43:00AM -0700, H. Peter Anvin wrote:
> >> > > On 10/13/2014 08:19 AM, Vivek Goyal wrote:
> >> > > >>>
> >> > > >>> This really shouldn't have happened this way on x86-64.  It
> >has to happen
> >> > > >>> this way on i386, but I worry that this may be a serious
> >misdesign in kaslr
> >> > > >>> on x86-64.  I'm also wondering if there is any other fallout
> >of this?
> >> > > >>
> >> > > >> I agree. On x86_64, we should stick to previous design and
> >this new
> >> > > >> logic of performing relocations does not sound very clean and
> >makes
> >> > > >> things very confusing.
> >> > > >>
> >> > > >> I am wondering that why couldn't we simply adjust page tables
> >in case of
> >> > > >> kaslr on x86_64, instead of performing relocations.
> >> > > > 
> >> > > > Well, IIUC, if virtual addresses are shifted w.r.t what virtual
> >address
> >> > > > kernel was compiled for, then relocation will have to be done.
> >> > > > 
> >> > > > So question will be if physical address shift is enough for
> >kaslr or
> >> > > > virtual address shift is necessary.
> >> > > > 
> >> > > 
> >> > > I would assume that without a virtual address shift kaslr is
> >pretty darn
> >> > > pointless.  Without the physical address shift the 1:1 map can be
> >used,
> >> > > and again, kaslr becomes pointless.  However, there is absolutely
> >no
> >> > > reason why they should be coupled.  They can, in fact, be
> >independently
> >> > > randomized.
> >> > 
> >> > Agreed. On x86_64, we should be able to randomize virtual address
> >space
> >> > and physical address space independently. And in that case whole of
> >> > the physical memory should be available for a possible location for
> >> > kernel. (As opposed to a small limit (I guess 1GB) now)
> >
> >It can be done to randomize virtual address space and physical address
> >space independently. But limited by the 2G of kernel text mapping and
> >module mapping virtual address space, virtual address can be randomized
> >in (0x1000000, 1G) range. While physical address can be randomized in
> >(0x1000000, 4G) according to the identity mapping of normal kernel.
> >Then
> >phys_base still stores an relative value, a different offset than
> >before.
> >
> >This can be easily implement. One thing is still there's a limit for
> >physical addr randomization, only below 4G. So I am wondering if we can
> >extend the identify mapping to complete mapping of 48 bit, using 1G
> >page
> >frame. This can make physical addr be randomized to anywhere.
> >
> >So now there may be 3 options:
> >
> >1) Fix this bug in current kaslr. Since when randomize the new kernel
> >location in choose_kernel_location(), cmdline options has been checked
> >strictly, e.g if nokaslr is specified, it's safe to do the kernel
> >location randomization. Then in handle_relocations(), we only need to
> >check if the kernel location is changed, comparing with kernel loaded
> >addr. If changed, kaslr is done, let's do the relocation handling. If
> >not changed, no kaslr id done, just skip the relocation handling like
> >before.
> >
> >2) randomize the virtual addr space and physical addr space
> >independently. But physical addr space must be below 4G.
> >
> >3) extend the identity mapping to 48bit of addr space. Then we can
> >randomized the virtual addr space in (0x1000000, 1G) and physical addr
> >space in (0x1000000, real physical memory end).
> >
> >If option 3 is doable, it's the best. If not, I think bug fix should be
> >better.
> >
> >> 
> >> Hi Peter,
> >> 
> >> So what do we do about this issue in short term to make kexec work.
> >Even
> >> if we go for above solution, to make kexec work we will have to pass
> >> "nokaslr" as we don't want kernel to move around in physical address
> >space
> >> as it might stomp over ELF headers we have stored.
> >
> >kexec doesn't need ELF headers. Kdump may need it. But in current
> >kexec-tools implementation, kernel/initrd and other stuffs are placed
> >from top to down, current implementation won't do kaslr since it only
> >happened between kernel loaded addr and 1G. So we don't need to worry
> >about the stomping.
> >
> >> 
> >> If you don't like current patch, should we just disable relocations
> >in
> >> x86_64 if "nokaslr" command line is passed. That way kernel will not
> >> be moved in physical as well as virtual address space.
> >> 
> >> Thanks
> >> Vivek
> 
> -- 
> Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 10/16/14 at 07:55am, Baoquan He wrote:
> On 10/15/14 at 01:32pm, H. Peter Anvin wrote:
> > I don't see why we can't randomize anywhere in physical space.  We already handle the kernel above 4 GB and it wouldn't be hard to do the equivalent for the decompress/relocation code, using a #PF handler.  Not all CPUs support 1 GB pages.
> > 
> 
> Well, IIUC, in arch/x86/boot/compressed/head_64.S pgtable contains 6
> page frames and is used to build 4G identity mapping. Then this pgtable
> is loaded into cr3. Later jump into 64bit mode. decompress/relocation
> code should work in this region. Isn't it right?
> 
> Currently kernel surely can be put anywhere, even above 4G. This have
> been done very often in kexec/kdump. However in kexec/kdump the identity
> mapping is built for all physical memory after jumping to startup_64
                                           ~~~~~ should be before

> directly from purgatory, that's why kexec/kdump kernel can be put
> anywhere.
> 
> So for normal kernel, don't we need to extend the physical memory mappng
> to a larger region if we want to randomize anywhere in physical space?
> 
> Thanks
> Baoquan
> 
> 
> > On October 14, 2014 8:37:01 PM PDT, Baoquan He <bhe@redhat.com> wrote:
> > >On 10/14/14 at 08:49am, Vivek Goyal wrote:
> > >> On Mon, Oct 13, 2014 at 01:22:42PM -0400, Vivek Goyal wrote:
> > >> > On Mon, Oct 13, 2014 at 08:43:00AM -0700, H. Peter Anvin wrote:
> > >> > > On 10/13/2014 08:19 AM, Vivek Goyal wrote:
> > >> > > >>>
> > >> > > >>> This really shouldn't have happened this way on x86-64.  It
> > >has to happen
> > >> > > >>> this way on i386, but I worry that this may be a serious
> > >misdesign in kaslr
> > >> > > >>> on x86-64.  I'm also wondering if there is any other fallout
> > >of this?
> > >> > > >>
> > >> > > >> I agree. On x86_64, we should stick to previous design and
> > >this new
> > >> > > >> logic of performing relocations does not sound very clean and
> > >makes
> > >> > > >> things very confusing.
> > >> > > >>
> > >> > > >> I am wondering that why couldn't we simply adjust page tables
> > >in case of
> > >> > > >> kaslr on x86_64, instead of performing relocations.
> > >> > > > 
> > >> > > > Well, IIUC, if virtual addresses are shifted w.r.t what virtual
> > >address
> > >> > > > kernel was compiled for, then relocation will have to be done.
> > >> > > > 
> > >> > > > So question will be if physical address shift is enough for
> > >kaslr or
> > >> > > > virtual address shift is necessary.
> > >> > > > 
> > >> > > 
> > >> > > I would assume that without a virtual address shift kaslr is
> > >pretty darn
> > >> > > pointless.  Without the physical address shift the 1:1 map can be
> > >used,
> > >> > > and again, kaslr becomes pointless.  However, there is absolutely
> > >no
> > >> > > reason why they should be coupled.  They can, in fact, be
> > >independently
> > >> > > randomized.
> > >> > 
> > >> > Agreed. On x86_64, we should be able to randomize virtual address
> > >space
> > >> > and physical address space independently. And in that case whole of
> > >> > the physical memory should be available for a possible location for
> > >> > kernel. (As opposed to a small limit (I guess 1GB) now)
> > >
> > >It can be done to randomize virtual address space and physical address
> > >space independently. But limited by the 2G of kernel text mapping and
> > >module mapping virtual address space, virtual address can be randomized
> > >in (0x1000000, 1G) range. While physical address can be randomized in
> > >(0x1000000, 4G) according to the identity mapping of normal kernel.
> > >Then
> > >phys_base still stores an relative value, a different offset than
> > >before.
> > >
> > >This can be easily implement. One thing is still there's a limit for
> > >physical addr randomization, only below 4G. So I am wondering if we can
> > >extend the identify mapping to complete mapping of 48 bit, using 1G
> > >page
> > >frame. This can make physical addr be randomized to anywhere.
> > >
> > >So now there may be 3 options:
> > >
> > >1) Fix this bug in current kaslr. Since when randomize the new kernel
> > >location in choose_kernel_location(), cmdline options has been checked
> > >strictly, e.g if nokaslr is specified, it's safe to do the kernel
> > >location randomization. Then in handle_relocations(), we only need to
> > >check if the kernel location is changed, comparing with kernel loaded
> > >addr. If changed, kaslr is done, let's do the relocation handling. If
> > >not changed, no kaslr id done, just skip the relocation handling like
> > >before.
> > >
> > >2) randomize the virtual addr space and physical addr space
> > >independently. But physical addr space must be below 4G.
> > >
> > >3) extend the identity mapping to 48bit of addr space. Then we can
> > >randomized the virtual addr space in (0x1000000, 1G) and physical addr
> > >space in (0x1000000, real physical memory end).
> > >
> > >If option 3 is doable, it's the best. If not, I think bug fix should be
> > >better.
> > >
> > >> 
> > >> Hi Peter,
> > >> 
> > >> So what do we do about this issue in short term to make kexec work.
> > >Even
> > >> if we go for above solution, to make kexec work we will have to pass
> > >> "nokaslr" as we don't want kernel to move around in physical address
> > >space
> > >> as it might stomp over ELF headers we have stored.
> > >
> > >kexec doesn't need ELF headers. Kdump may need it. But in current
> > >kexec-tools implementation, kernel/initrd and other stuffs are placed
> > >from top to down, current implementation won't do kaslr since it only
> > >happened between kernel loaded addr and 1G. So we don't need to worry
> > >about the stomping.
> > >
> > >> 
> > >> If you don't like current patch, should we just disable relocations
> > >in
> > >> x86_64 if "nokaslr" command line is passed. That way kernel will not
> > >> be moved in physical as well as virtual address space.
> > >> 
> > >> Thanks
> > >> Vivek
> > 
> > -- 
> > Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 10/15/14 at 11:37am, Baoquan He wrote:
> On 10/14/14 at 08:49am, Vivek Goyal wrote:
> > On Mon, Oct 13, 2014 at 01:22:42PM -0400, Vivek Goyal wrote:
 
> > > Agreed. On x86_64, we should be able to randomize virtual address space
> > > and physical address space independently. And in that case whole of
> > > the physical memory should be available for a possible location for
> > > kernel. (As opposed to a small limit (I guess 1GB) now)
> 
> It can be done to randomize virtual address space and physical address
> space independently. But limited by the 2G of kernel text mapping and
> module mapping virtual address space, virtual address can be randomized
> in (0x1000000, 1G) range. While physical address can be randomized in
> (0x1000000, 4G) according to the identity mapping of normal kernel. Then
> phys_base still stores an relative value, a different offset than before.
> 
> This can be easily implement. One thing is still there's a limit for
> physical addr randomization, only below 4G. So I am wondering if we can
> extend the identify mapping to complete mapping of 48 bit, using 1G page
> frame. This can make physical addr be randomized to anywhere.
> 
> So now there may be 3 options:

Hi hpa,

About kaslr, do you have a suggestion or plan? Should we fix bug based
on current code, or any other solutions?

Thanks
Baoquan


> 
> 1) Fix this bug in current kaslr. Since when randomize the new kernel
> location in choose_kernel_location(), cmdline options has been checked
> strictly, e.g if nokaslr is specified, it's safe to do the kernel
> location randomization. Then in handle_relocations(), we only need to
> check if the kernel location is changed, comparing with kernel loaded
> addr. If changed, kaslr is done, let's do the relocation handling. If
> not changed, no kaslr id done, just skip the relocation handling like
> before.
> 
> 2) randomize the virtual addr space and physical addr space
> independently. But physical addr space must be below 4G.
> 
> 3) extend the identity mapping to 48bit of addr space. Then we can
> randomized the virtual addr space in (0x1000000, 1G) and physical addr
> space in (0x1000000, real physical memory end).
> 
> If option 3 is doable, it's the best. If not, I think bug fix should be
> better.
> 

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

On 09/30/14 at 02:21pm, H. Peter Anvin wrote:
> On 09/30/2014 12:08 AM, Baoquan He wrote:
> > Function handle_relocations() is used to do the relocations handling
> > for i686 and kaslr of x86_64. For 32 bit the relocation handling is
> > mandotary to perform. For x86_64 only when kaslr is enabled and a
> > random kernel location is chosen successfully the relocation handling
> > shound be done. However previous implementation only compared the
> > kernel loading address and LOAD_PHYSICAL_ADDR where kernel were
> > compiled to run at. This would casue system to be exceptional in
> > few conditions like when delta between load address and compiled
> > address is bigger than what 32bit signed relocations can handle.
> > Also there will be limitations that delta can't be too big otherwise
> > kernel text virtual addresses will overflow in module address space.
> > 
> > So in this patch check if kernel location is changed after
> > choose_kernel_location() when x86_64. If and only if in x86_64
> > and kernel location is changed, we say a kaslr random kernel
> > location is chosen, then the relocation handling is needed.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > Acked-by: Kees Cook <keescook@chromium.org>
> > Tested-by: Thomas D. <whissi@whissi.de>
> > Cc: stable@vger.kernel.org
> 
> Could you clarify under what conditions we may end up with 32-bit signed
> overflow, and yet have a functional kernel?

Hi hpa,

As Vivek has replied, and from my test result, in x86_64 when kernel is
loaded above 2G by bootloader, the 32-bit signed overflow will happen.
Meanwhile the error() in arch/x86/boot/compressed/misc.c will be called,
this function will execute 'hlt' instruction, then system halted.

In current kaslr, there are 3 hebaviours. Since the candidate slot for
randomized kernel location is only between kernel loaded address and 1G,
it doesn't choose a new kernel location when kernel is loaded above 1G.

1) Kernel is loaded between 0x1000000 and 1G, kaslr works well.

2)kernel is loaded between 1G and 2G, kernel doesn't choose a new kernel
location, so the ourput_orig==output > 1G. But current code compares the
output with LOAD_PHYSICAL_ADDR, relocation handling is still taken and
this cause kenrel text mapping stomps module mapping space. So in this
case form printing we can see kaslr passed, but kernel reboot to bios
immediately.

3) When kernel is loaded above 2G, this is the 32-bit signed overflow as
we discussed above.

So this patch can fix case 2  and case 3. 

Thanks
Baoquan
> 
> 

Re: [resend Patch v3 1/2] kaslr: check if kernel location is changed

[Baoquan He]

Hi hpa,

Ping.

Do you have further plan or idea on this issue? Or could you
please merge this patch to fix reported bug for now?

Thanks
Baoquan

On 09/30/14 at 03:08pm, Baoquan He wrote:
> Function handle_relocations() is used to do the relocations handling
> for i686 and kaslr of x86_64. For 32 bit the relocation handling is
> mandotary to perform. For x86_64 only when kaslr is enabled and a
> random kernel location is chosen successfully the relocation handling
> shound be done. However previous implementation only compared the
> kernel loading address and LOAD_PHYSICAL_ADDR where kernel were
> compiled to run at. This would casue system to be exceptional in
> few conditions like when delta between load address and compiled
> address is bigger than what 32bit signed relocations can handle.
> Also there will be limitations that delta can't be too big otherwise
> kernel text virtual addresses will overflow in module address space.
> 
> So in this patch check if kernel location is changed after
> choose_kernel_location() when x86_64. If and only if in x86_64
> and kernel location is changed, we say a kaslr random kernel
> location is chosen, then the relocation handling is needed.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Acked-by: Kees Cook <keescook@chromium.org>
> Tested-by: Thomas D. <whissi@whissi.de>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/boot/compressed/misc.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 57ab74d..3bb2a17 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;
> @@ -239,6 +240,20 @@ static void handle_relocations(void *output, unsigned long output_len)
>  	unsigned long max_addr = min_addr + output_len;
>  
>  	/*
> +	* 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.
> +	*/
> +#if CONFIG_X86_64
> +	if (output_orig == output)
> +		return;
> +#endif
> +
> +	/*
>  	 * Calculate the delta between where vmlinux was linked to load
>  	 * and where it was actually loaded.
>  	 */
> @@ -299,7 +314,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 +376,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 +420,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
>