From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751235AbcBKTtw (ORCPT ); Thu, 11 Feb 2016 14:49:52 -0500 Received: from mail.skyhub.de ([78.46.96.112]:40586 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751164AbcBKTtu (ORCPT ); Thu, 11 Feb 2016 14:49:50 -0500 Date: Thu, 11 Feb 2016 20:49:43 +0100 From: Borislav Petkov To: Andy Lutomirski Cc: X86 ML , "linux-kernel@vger.kernel.org" , Brian Gerst , Denys Vlasenko , Stas Sergeev , Cyrill Gorcunov , Pavel Emelyanov , Linus Torvalds Subject: Re: [PATCH v3 3/4] x86/signal/64: Re-add support for SS in the 64-bit signal context Message-ID: <20160211194943.GH5565@pd.tnic> References: <40665bc51802a9976345f2a41dc6abb97dd944a5.1453754484.git.luto@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <40665bc51802a9976345f2a41dc6abb97dd944a5.1453754484.git.luto@kernel.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 25, 2016 at 01:34:14PM -0800, Andy Lutomirski wrote: > This is a second attempt to make the improvements from c6f2062935c8 > ("x86/signal/64: Fix SS handling for signals delivered to 64-bit > programs"), which was reverted by 51adbfbba5c6 ("x86/signal/64: Add > support for SS in the 64-bit signal context"). > > This adds two new uc_flags flags. UC_SIGCONTEXT_SS will be set for > all 64-bit signals (including x32). It indicates that the saved SS > field is valid and that the kernel supports the new behavior. > > The goal is to fix a problems with signal handling in 64-bit tasks: > SS wasn't saved in the 64-bit signal context, making it awkward to > determine what SS was at the time of signal delivery and making it > impossible to return to a non-flat SS (as calling sigreturn clobbers > SS). > > This also made it extremely difficult for 64-bit tasks to return to > fully-defined 16-bit contexts, because only the kernel can easily do > espfix64, but sigreturn was unable to set a non-flag SS:ESP. > (DOSEMU has a monstrous hack to partially work around this > limitation.) > > If we could go back in time, the correct fix would be to make 64-bit > signals work just like 32-bit signals with respect to SS: save it > in signal context, reset it when delivering a signal, and restore > it in sigreturn. > > Unfortunately, doing that (as I tried originally) breaks DOSEMU: > DOSEMU wouldn't reset the signal context's SS when clearing the LDT > and changing the saved CS to 64-bit mode, since it predates the SS > context field existing in the first place. > > This patch is a bit more complicated, and it tries to balance a > bunch of goals. It makes most cases of changing ucontext->ss during > signal handling work as expected. > > I do this by special-casing the interesting case. On sigreturn, > ucontext->ss will be honored by default, unless the ucontext was > created from scratch by an old program and had a 64-bit CS > (unfortunately, CRIU can do this) or was the result of changing a > 32-bit signal context to 64-bit without resetting SS (as DOSEMU > does). > > For the benefit of new 64-bit software that uses segmentation (new > versions of DOSEMU might), the new behavior can be detected with a > new ucontext flag UC_SIGCONTEXT_SS. > > To avoid compilation issues, __pad0 is left as an alias for ss in > ucontext. > > The nitty-gritty details are documented in the header file. > > This patch also re-enables the sigreturn_64 and ldt_gdt_64 selftests, > as the kernel change allows both of them to pass. > > Cc: Stas Sergeev > Cc: Linus Torvalds > Cc: Cyrill Gorcunov > Cc: Pavel Emelyanov > Tested-by: Stas Sergeev > Signed-off-by: Andy Lutomirski > --- > arch/x86/include/asm/sighandling.h | 1 - > arch/x86/include/uapi/asm/sigcontext.h | 7 ++-- > arch/x86/include/uapi/asm/ucontext.h | 43 ++++++++++++++++++++--- > arch/x86/kernel/signal.c | 63 ++++++++++++++++++++++++---------- > tools/testing/selftests/x86/Makefile | 7 ++-- > 5 files changed, 91 insertions(+), 30 deletions(-) > > diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h > index 89db46752a8f..452c88b8ad06 100644 > --- a/arch/x86/include/asm/sighandling.h > +++ b/arch/x86/include/asm/sighandling.h > @@ -13,7 +13,6 @@ > X86_EFLAGS_CF | X86_EFLAGS_RF) > > void signal_fault(struct pt_regs *regs, void __user *frame, char *where); > -int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc); > int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate, > struct pt_regs *regs, unsigned long mask); > > diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h > index 47dae8150520..bb0dde737b59 100644 > --- a/arch/x86/include/uapi/asm/sigcontext.h > +++ b/arch/x86/include/uapi/asm/sigcontext.h > @@ -256,7 +256,7 @@ struct sigcontext_64 { > __u16 cs; > __u16 gs; > __u16 fs; > - __u16 __pad0; > + __u16 ss; > __u64 err; > __u64 trapno; > __u64 oldmask; > @@ -362,7 +362,10 @@ struct sigcontext { > */ > __u16 gs; > __u16 fs; > - __u16 __pad0; > + union { > + __u16 ss; /* If UC_SAVED_SS */ > + __u16 __pad0; /* If !UC_SAVED_SS */ UC_SIGCONTEXT_SS ? > + }; > __u64 err; > __u64 trapno; > __u64 oldmask; > diff --git a/arch/x86/include/uapi/asm/ucontext.h b/arch/x86/include/uapi/asm/ucontext.h > index b7c29c8017f2..a5c1718ce65b 100644 > --- a/arch/x86/include/uapi/asm/ucontext.h > +++ b/arch/x86/include/uapi/asm/ucontext.h > @@ -1,11 +1,44 @@ > #ifndef _ASM_X86_UCONTEXT_H > #define _ASM_X86_UCONTEXT_H > > -#define UC_FP_XSTATE 0x1 /* indicates the presence of extended state > - * information in the memory layout pointed > - * by the fpstate pointer in the ucontext's > - * sigcontext struct (uc_mcontext). > - */ > +/* > + * indicates the presence of extended state > + * information in the memory layout pointed > + * by the fpstate pointer in the ucontext's > + * sigcontext struct (uc_mcontext). > + */ Please reflow that comment to match the rest of the comments in this file. > +#define UC_FP_XSTATE 0x1 > + > +#ifdef __x86_64__ > +/* > + * UC_SIGCONTEXT_SS will be set when delivering 64-bit or x32 signals on > + * kernels that save SS in the sigcontext. All kernels that set > + * UC_SIGCONTEXT_SS will correctly restore at least the low 32 bits of esp > + * regardless of SS (i.e. they implement espfix). > + * > + * Kernels that set UC_SIGCONTEXT_SS will also set UC_STRICT_RESTORE_SS > + * when delivering a signal that came from 64-bit code. > + * > + * Sigreturn modifies its behavior depending on the > + * UC_STRICT_RESTORE_SS flag. If UC_STRICT_RESTORE_SS is set, or if > + * the CS value in the signal context does not refer to a 64-bit > + * code segment, then the SS value in the signal context is restored > + * verbatim. If UC_STRICT_RESTORE_SS is not set, the CS value in > + * the signal context refers to a 64-bit code segment, and the > + * signal context's SS value is invalid, then SS it will be replaced s/it // > + * with a flat 32-bit selector. > + > + * This behavior serves two purposes. It ensures that older programs > + * that are unaware of the signal context's SS slot and either construct > + * a signal context from scratch or that catch signals from segmented > + * contexts and change CS to a 64-bit selector won't crash due to a bad > + * SS value. I'm having hard time parsing that sentence and especially placing all those "either", "or", "and" connectors at the proper levels. Would it be more understandable as pseudocode? > It also ensures that signal handlers that do not modify > + * the signal context at all return back to the exact CS and SS state > + * that they came from. So my brain is reliably in a knot after this text. > + */ > +#define UC_SIGCONTEXT_SS 0x2 > +#define UC_STRICT_RESTORE_SS 0x4 > +#endif > > #include > -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.