Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752262AbdL0T6P (ORCPT ); Wed, 27 Dec 2017 14:58:15 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:60359 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751184AbdL0T6N (ORCPT ); Wed, 27 Dec 2017 14:58:13 -0500 Date: Wed, 27 Dec 2017 20:58:08 +0100 (CET) From: Thomas Gleixner To: Linus Torvalds cc: Alexandru Chirvasitu , Ingo Molnar , Andy Lutomirski , kernel list , Borislav Petkov , Brian Gerst , Denys Vlasenko , "H. Peter Anvin" , Josh Poimboeuf , Peter Zijlstra , Steven Rostedt Subject: Re: [PATCH] x86-32: fix kexec with stack canary (CONFIG_CC_STACKPROTECTOR) In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3421 Lines: 78 On Wed, 27 Dec 2017, Linus Torvalds wrote: > From: Linus Torvalds > 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 > Fixes: e802a51ede91 ("x86/idt: Consolidate IDT invalidation") > Cc: Thomas Gleixner > Cc: Andy Lutomirski > Cc: Peter Anvin > Cc: Ingo Molnar > Signed-off-by: Linus Torvalds > --- > > 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