x86: sigcontext fixes, again

This is take 2 at fixing x86 64-bit signals wrt SS. After a lot of thought, this is not controlled by any flags -- I would much prefer to avoid opt-in behavior. Instead, it just tries hard to avoid triggering the cases that break DOSEMU.

Stas, this now seems to pass the test you sent me. It works with stock dosemu2 (I haven't tested classic dosemu because I can't get it to work regardless). It also works with a patched dosemu2 that bypasses the userspace trampoline:

https://github.com/amluto/dosemu2/commit/571b4d08dc885b7a133e444a2ad23e0d21366206

With this applied, all of the x86 selftests pass on x86_64. That wasn't the case before -- ldt_gdt_64 was broken.

Quoted 3 lines

With this applied, all of the x86 selftests pass on x86_64. That wasn't the case before -- ldt_gdt_64 was broken.

I've been testing this series already. I guess ;) Anyway, gonna try it one more shot at the evening, just to make sure. From a glance everything looks just great!

Signals are always delivered to 64-bit tasks with CS set to a long mode segment. In long mode, SS doesn't matter as long as it's a present writable segment.

If SS starts out invalid (this can happen if the signal was caused by an IRET fault or was delivered on the way out of set_thread_area or modify_ldt), then IRET to the signal handler can fail, eventually killing the task.

The straightforward fix would be to simply reset SS when delivering a signal. That breaks DOSEMU, though: 64-bit builds of DOSEMU rely on SS being set to the faulting SS when signals are delivered.

As a compromise, this patch leaves SS alone so long as it's valid.

The net effect should be that the behavior of successfully delivered signals is unchanged. Some signals that would previously have failed to be delivered will now be delivered successfully.

This has no effect for x32 or 32-bit tasks: their signal handlers were already called with SS == __USER_DS.

(On Xen, there's a slight hole: if a task sets SS to a writable
*kernel* data segment, then we will fail to identify it as invalid and we'll still kill the task. If anyone cares, this could be fixed with a new paravirt hook.)

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/desc_defs.h | 23 ++++++++++++++++++
 arch/x86/kernel/signal.c         | 51 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 72 insertions(+), 2 deletions(-)

arch/x86/include/asm/desc_defs.h +7 -0

index 278441f39856..00971705a16d 100644
@@ -98,4 +98,27 @@ struct desc_ptr {
 
 #endif /* !__ASSEMBLY__ */
 
+/* Access rights as returned by LAR */
+#define AR_TYPE_RODATA		(0 * (1 << 9))
+#define AR_TYPE_RWDATA		(1 * (1 << 9))
+#define AR_TYPE_RODATA_EXPDOWN	(2 * (1 << 9))
+#define AR_TYPE_RWDATA_EXPDOWN	(3 * (1 << 9))
+#define AR_TYPE_XOCODE		(4 * (1 << 9))
+#define AR_TYPE_XRCODE		(5 * (1 << 9))

arch/x86/kernel/signal.c +38 -2

+/*
+ * If regs->ss will cause an IRET fault, change it.  Otherwise leave it
+ * alone.  Using this generally makes no sense unless
+ * user_64bit_mode(regs) would return true.
+ */
+static void force_valid_ss(struct pt_regs *regs)
+{
+	u32 ar;
+	asm volatile ("lar %[old_ss], %[ar]\n\t"
+		      "jz 1f\n\t"		/* If invalid: */
+		      "xorl %[ar], %[ar]\n\t"	/* set ar = 0 */
+		      "1:"
+		      : [ar] "=r" (ar)
+		      : [old_ss] "rm" ((u16)regs->ss));
+
+	/*
+	 * For a valid 64-bit user context, we need DPL 3, type
+	 * read-write data or read-write exp-down data, and S and P
+	 * set.  We can't use VERW because VERW doesn't check the
+	 * P bit.
+	 */
+	ar &= AR_DPL_MASK | AR_S | AR_P | AR_TYPE_MASK;
+	if (ar != (AR_DPL3 | AR_S | AR_P | AR_TYPE_RWDATA) &&
+	    ar != (AR_DPL3 | AR_S | AR_P | AR_TYPE_RWDATA_EXPDOWN))
+		regs->ss = __USER_DS;
+}
+#endif
+
 int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)
 {
 	unsigned long buf_val;

@@ -459,10 +488,28 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
 
 	regs->sp = (unsigned long)frame;
 
-	/* Set up the CS register to run signal handlers in 64-bit mode,
-	   even if the handler happens to be interrupting 32-bit code. */
+	/*
+	 * Set up the CS and SS registers to run signal handlers in
+	 * 64-bit mode, even if the handler happens to be interrupting
+	 * 32-bit or 16-bit code.
+	 *
+	 * SS is subtle.  In 64-bit mode, we don't need any particular
+	 * SS descriptor, but we do need SS to be valid.  It's possible
+	 * that the old SS is entirely bogus -- this can happen if the
+	 * signal we're trying to deliver is #GP or #SS caused by a bad
+	 * SS value.  We also have a compatbility issue here: DOSEMU

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 <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Tested-by Stas Sergeev <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 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(-)

arch/x86/include/asm/sighandling.h +0 -1

index 89db46752a8f..452c88b8ad06 100644
@@ -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);
 

arch/x86/include/uapi/asm/sigcontext.h +1 -1

index 47dae8150520..bb0dde737b59 100644
@@ -256,7 +256,7 @@ struct sigcontext_64 {
 	__u16				cs;
 	__u16				gs;
 	__u16				fs;
-	__u16				__pad0;
+	__u16				ss;
 	__u64				err;
 	__u64				trapno;
 	__u64				oldmask;

arch/x86/include/uapi/asm/sigcontext.h +1 -0

+	};
 	__u64				err;
 	__u64				trapno;
 	__u64				oldmask;

arch/x86/include/uapi/asm/ucontext.h +0 -1

index b7c29c8017f2..a5c1718ce65b 100644
@@ -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

arch/x86/include/uapi/asm/ucontext.h +9 -0

+#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).
+ *

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.

+ */

arch/x86/include/uapi/asm/ucontext.h +3 -0

+#define UC_SIGCONTEXT_SS	0x2
+#define UC_STRICT_RESTORE_SS	0x4
+#endif
 
 #include <asm-generic/ucontext.h>

This tests the two ABI-preserving cases that DOSEMU cares about, and it also explicitly tests the new UC_SIGCONTEXT_SS and UC_STRICT_RESTORE_SS flags.

Signed-off-by Andy Lutomirski <[email protected]>

---
 tools/testing/selftests/x86/sigreturn.c | 240 ++++++++++++++++++++++++++++----
 1 file changed, 212 insertions(+), 28 deletions(-)

tools/testing/selftests/x86/sigreturn.c +2 -0

index b5aa1bab7416..43e840470e32 100644
@@ -55,6 +55,47 @@
 #include <sys/user.h>
 
 /*
+ * Copied from asm/ucontext.h, as asm/ucontext.h conflicts badly with the glibc
+ * headers.

tools/testing/selftests/x86/sigreturn.c +17 -0

+ */
+#define UC_SIGCONTEXT_SS       0x2
+#define UC_STRICT_RESTORE_SS   0x4
+#endif
+
+/* Access rights as returned by LAR */
+#define AR_TYPE_RODATA		(0 * (1 << 9))
+#define AR_TYPE_RWDATA		(1 * (1 << 9))
+#define AR_TYPE_RODATA_EXPDOWN	(2 * (1 << 9))
+#define AR_TYPE_RWDATA_EXPDOWN	(3 * (1 << 9))
+#define AR_TYPE_XOCODE		(4 * (1 << 9))
+#define AR_TYPE_XRCODE		(5 * (1 << 9))
+#define AR_TYPE_XOCODE_CONF	(6 * (1 << 9))
+#define AR_TYPE_XRCODE_CONF	(7 * (1 << 9))
+#define AR_TYPE_MASK		(7 * (1 << 9))
+
+#define AR_DPL0			(0 * (1 << 13))

Quoted 16 lines · Andy Lutomirski

This tests the two ABI-preserving cases that DOSEMU cares about, and it also explicitly tests the new UC_SIGCONTEXT_SS and UC_STRICT_RESTORE_SS flags.

Signed-off-by Andy Lutomirski <[email protected]>

---
 tools/testing/selftests/x86/sigreturn.c | 240 ++++++++++++++++++++++++++++----
 1 file changed, 212 insertions(+), 28 deletions(-)

tools/testing/selftests/x86/sigreturn.c +0 -0

index b5aa1bab7416..43e840470e32 100644
@@ -55,6 +55,47 @@
 #include <sys/user.h>
 

tools/testing/selftests/x86/sigreturn.c line 69

Quoted 13 lines

tools/testing/selftests/x86/sigreturn.c +12 -0

 /*
+ * Copied from asm/ucontext.h, as asm/ucontext.h conflicts badly with the glibc
+ * headers.
+ */
+#ifdef __x86_64__
+/*
+ * UC_SAVED_SS will be set when delivering 64-bit or x32 signals on
+ * kernels that save SS in the sigcontext.  Kernels that set UC_SAVED_SS
+ * allow signal handlers to set UC_RESTORE_SS; if UC_RESTORE_SS is set,
+ * then sigreturn will restore SS.
+ *
+ * For compatibility with old programs, the kernel will *not* set
+ * UC_RESTORE_SS when delivering signals.

Those UC_SAVED_SS and UC_RESTORE_SS look stale to me.

Quoted 22 lines

tools/testing/selftests/x86/sigreturn.c +21 -0

+ */
+#define UC_SIGCONTEXT_SS       0x2
+#define UC_STRICT_RESTORE_SS   0x4
+#endif
+
+/* Access rights as returned by LAR */
+#define AR_TYPE_RODATA               (0 * (1 << 9))
+#define AR_TYPE_RWDATA               (1 * (1 << 9))
+#define AR_TYPE_RODATA_EXPDOWN       (2 * (1 << 9))
+#define AR_TYPE_RWDATA_EXPDOWN       (3 * (1 << 9))
+#define AR_TYPE_XOCODE               (4 * (1 << 9))
+#define AR_TYPE_XRCODE               (5 * (1 << 9))
+#define AR_TYPE_XOCODE_CONF  (6 * (1 << 9))
+#define AR_TYPE_XRCODE_CONF  (7 * (1 << 9))
+#define AR_TYPE_MASK         (7 * (1 << 9))
+
+#define AR_DPL0                      (0 * (1 << 13))
+#define AR_DPL3                      (3 * (1 << 13))
+#define AR_DPL_MASK          (3 * (1 << 13))
+
+#define AR_A                 (1 << 8)        /* A means "accessed" */

tools/testing/selftests/x86/sigreturn.c line 96

Quoted 6 lines

tools/testing/selftests/x86/sigreturn.c +6 -0

+#define AR_S                 (1 << 12)       /* S means "not system" */
+#define AR_P                 (1 << 15)       /* P means "present" */
+#define AR_AVL                       (1 << 20)       /* AVL does nothing */
+#define AR_L                 (1 << 21)       /* L means "long mode" */
+#define AR_DB                        (1 << 22)       /* D or B, depending on type */
+#define AR_G                 (1 << 23)       /* G means "limit in pages" */

Why not include the kernel header instead of repeating it here again?

Quoted 1 line · Andy Lutomirski

+ * with a flat 32-bit selector.

Quoted 6 lines

+
+ * 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?

Quoted 3 lines

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.

Quoted 3 lines · Borislav Petkov

Quoted 1 line · Andy Lutomirski

+ * with a flat 32-bit selector.

How about:

Sigreturn restores SS as follows:

if (saved SS is valid || UC_STRICT_RESTORE_SS is set || saved CS is not 64-bit)
new SS = saved SS
else
new SS = a flat 32-bit data segment

How about:

--- cut here ---

This behavior serves three purposes:

- Legacy programs that construct a 64-bit sigcontext from scratch
with zero or garbage in the SS slot (e.g. old CRIU) and call sigreturn will still work.

- Old DOSEMU versions sometimes catch a signal from a segmented
context, delete the old SS segment (with modify_ldt), and change the saved CS to a 64-bit segment. These DOSEMU versions expect sigreturn to send them back to 64-bit mode without killing them, despite the fact that the SS selector when the signal was raised is no longer valid. With UC_STRICT_RESTORE_SS clear, the kernel will fix up SS for these DOSEMU versions.

- Old and new programs that catch a signal and return without
modifying the saved context will end up in exactly the state they started in. Old kernels would lose track of the previous SS value.

--- cut here ---

FWIW, I have a DOSEMU patch that makes it use UC_STRICT_RESTORE_SS to get the behavior it actually wants on new kernels. It should make it faster and more reliable than was possible before.

Too tangled. Adding:

#include "../../../../arch/x86/include/asm/desc_defs.h"

Complains that u16 isn't a type. Trying to include types.h doesn't work well either.