linux-kernel - Re: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Dec 2017 20:58:08 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
cc:     Alexandru Chirvasitu <achirvasub@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        Andy Lutomirski <luto@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Borislav Petkov <bp@...en8.de>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH] x86-32: fix kexec with stack canary
 (CONFIG_CC_STACKPROTECTOR)

On Wed, 27 Dec 2017, Linus Torvalds wrote:
> From: Linus Torvalds <torvalds@...ux-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@...il.com>
> Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation")
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: Peter Anvin <hpa@...or.com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Signed-off-by: Linus Torvalds <torvalds@...ux-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