Re: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR)

[Thomas Gleixner]

On Wed, 27 Dec 2017, Linus Torvalds wrote:
> From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Peter Anvin <hpa@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
> 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