lkml.org 
[lkml]   [2017]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
On Wed, 27 Dec 2017, Linus Torvalds wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 27 Dec 2017 11:41:30 -0800
> Subject: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)
>
> Commit e802a51ede91 ("x86/idt: Consolidate IDT invalidation") cleaned up
> and unified the IDT invalidation that existed in a couple of places. It
> changed no actual real code.
>
> Despite not changing any actual real code, it _did_ change code
> generation: by implementing the common idt_invalidate() function in
> archx86/kernel/idt.c, it made the use of the function in
> arch/x86/kernel/machine_kexec_32.c be a real function call rather than
> an (accidental) inlining of the function.

Duh. I just got around reading that thread. Yes, that was definitely not
intended and the situation before my change was not really obvious ...

> That, in turn, exposed two issues:
>
> - in load_segments(), we had incorrectly reset all the segment
> registers, which then made the stack canary load (which gcc does
> using offset of %gs) cause a trap. Instead of %gs pointing to the
> stack canary, it will be the normal zero-based kernel segment, and
> the stack canary load will take a page fault at address 0x14.
>
> - to make this even harder to debug, we had invalidated the GDT just
> before calling idt_invalidate(), which meant that the fault happened
> with an invalid GDT, which in turn causes a triple fault and
> immediate reboot.

Nice detective work.

> Fix this by
>
> (a) not reloading the special segments in load_segments(). We currently
> don't do any percpu accesses (which would require %fs on x86-32) in
> this area, but there's no reason to think that we might not want to
> do them, and like %gs, it's pointless to break it.
>
> (b) doing idt_invalidate() before invalidating the GDT, to keep things
> at least _slightly_ more debuggable for a bit longer. Without a
> IDT, traps will not work. Without a GDT, traps also will not work,
> but neither will any segment loads etc. So in a very real sense,
> the GDT is even more core than the IDT.
>
> Reported-and-tested-by: Alexandru Chirvasitu <achirvasub@gmail.com>
> Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Anvin <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>
> I wrote "Reported-and-tested-by: Alexandru" because while this isn't
> exactly the same patch as anything Alexandru tested, it's pretty close,
> and I'm pretty sure this version will fix his issues too.
>
> I decided to try to just do the minimal changes: the GDT invalidation last
> (because of the debugging) and _only_ removing the resetting of fs/gs
> rather than removing load_segments() entirely.
>
> I think making idt_invalidate() be inline would be a good thing as well,
> and I do think that all those "phys_to_virt(0)" things are garbage, but I
> also think they are independent issues, so I didn't touch any of that.

I've put that on the list of stuff to look at once my actual time waster is
done.

> I'm assuming I'll get this patch back through the x86 tree, and will not
> be applying it to my own git tree unless the x86 people ask me to.

I'll pick it up.

Thanks,

tglx

\
 
 \ /
  Last update: 2017-12-27 20:59    [W:0.068 / U:6.408 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site