Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1674577 > unrolled thread

Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Ingo Molnar <mingo@kernel.org> - 2017-06-26 11:50 +0200
    Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Baoquan He <bhe@redhat.com> - 2017-06-26 12:50 +0200
      Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Ingo Molnar <mingo@kernel.org> - 2017-06-27 10:40 +0200
        Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case Baoquan He <bhe@redhat.com> - 2017-06-27 11:00 +0200

#1674577 — Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case

* Baoquan He <bhe@redhat.com> wrote:

> Kdump kernel will reset to firmware after crash is trigered when
> crashkernel=xxM,high is added to kernel command line. Kexec has the
> same phenomenon. This only happened on system with kaslr code
> compiled in and kernel option 'nokaslr'is added. Both of them works
> well when kaslr is enabled.
> 
> When crashkernel high is set or kexec case, kexec/kdump kernel will be
> put above 4G. Since we assign the original loading address of kernel to
> virt_addr as initial value, the virt_addr will be larger than 1G if kaslr
> is disabled, it exceeds the kernel mapping size which is only 1G. Then
> it will cause relocation handling error in handle_relocations().

So instead of whacking yet another kexec mole, how could we turn this into a more 
debuggable warning (either during build or during the failed bootup) instead of a 
crash and reset (triple fault?) back to the BIOS screen?

If kexec/kdump wants to do crazy things they should at least be _debuggable_ in a 
straightforward manner.

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1674632

Hi Ingo,

Thanks for looking into this patch!

On 06/26/17 at 11:47am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > Kdump kernel will reset to firmware after crash is trigered when
> > crashkernel=xxM,high is added to kernel command line. Kexec has the
> > same phenomenon. This only happened on system with kaslr code
> > compiled in and kernel option 'nokaslr'is added. Both of them works
> > well when kaslr is enabled.
> > 
> > When crashkernel high is set or kexec case, kexec/kdump kernel will be
> > put above 4G. Since we assign the original loading address of kernel to
> > virt_addr as initial value, the virt_addr will be larger than 1G if kaslr
> > is disabled, it exceeds the kernel mapping size which is only 1G. Then
> > it will cause relocation handling error in handle_relocations().
> 
> So instead of whacking yet another kexec mole, how could we turn this into a more 
> debuggable warning (either during build or during the failed bootup) instead of a 
> crash and reset (triple fault?) back to the BIOS screen?

This is a good question.

In fact this might not be kexe only problem. It's actually a code bug.
For x86_64, kernel text kaslr is separated into physical and virtual
address randomization. And here the virtual addr randmoization, can only
be done inside [0xffffffff80000000, 0xffffffffc0000000), the 1G area.
When we do the virtual address randomization, we only randomize to get
an offset between 0 and 1G, then add this offset onto the starting
address, 0xffffffff80000000. In the current code, virt_addr represents
the offset, which is a little confusing. In my original patch, it's
named as virt_offset(the link of original patch is pasted below). Kees
helped refactor the code, that corner case could be forgotton.

https://github.com/baoquan-he/linux/commit/034fbed941c60099368a40058cdfd0b4cac76040

If from the point of view of the offset of virtual address, which is among
kernel mapping area [0xffffffff80000000, 0xffffffffc0000000), the 'output'
which is the original loading address of kernel, absolutely should not be
assigned to virt_addr (better renamed as virt_offset or something else).

Here, kexec/kdump is only a pratical use case. I know someone ever
modifed bootloader to make kernel can be loaded arbitrary address, which
is not 16M, decided at compiled time. Then kernel will fail too.

As you suggested, we can add a checking to see if the virt_addr is
bigger than 1G, and print warning if exceed or hang there with error
message.

> 
> If kexec/kdump wants to do crazy things they should at least be _debuggable_ in a 
> straightforward manner.

So, it may not be fault of kexec/kdump, it's a code bug. I guess we
allow kernel to be loaded at any address, but not the address decided at
compiled time, 16M, right?

Thanks
Baoquan

[toc] | [prev] | [next] | [standalone]

#1675411

* Baoquan He <bhe@redhat.com> wrote:

> As you suggested, we can add a checking to see if the virt_addr is
> bigger than 1G, and print warning if exceed or hang there with error
> message.

Could you try a patch for that, and see whether it catches this particular bug? 
(before the fix is applied.)

Could be a 2 patch series: first the patch that adds the warning, then the fix.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1675424

On 06/27/17 at 10:34am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > As you suggested, we can add a checking to see if the virt_addr is
> > bigger than 1G, and print warning if exceed or hang there with error
> > message.
> 
> Could you try a patch for that, and see whether it catches this particular bug? 
> (before the fix is applied.)

Sure, below code change should catch it. Just I am struggling to decide
if I should add a new local variable and assign
 max(output_len, kernel_total_size) to it, and the name of the new local
variable is really hard to choose. Let me run a test on below code.

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index c945acd8fa33..00241c815524 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -390,6 +390,8 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
 #ifdef CONFIG_X86_64
 	if (heap > 0x3fffffffffffUL)
 		error("Destination address too large");
+	if (virt_addr + max(output_len, kernel_total_size) > KERNEL_IMAGE_SIZE)
+		error("Destination virtual address is beyond the kernel mapping area");
 #else
 	if (heap > ((-__PAGE_OFFSET-(128<<20)-1) & 0x7fffffff))
 		error("Destination address too large");

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web