Skip to contentSkip to conversation

x86: sigcontext fixes, again

PATCH Superseded Andy Lutomirski 4 participants 8 messages Feb 16, 2016

Subsystems (2)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v4 cover 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.

This is a bit risky, and another option would be to do nothing at all. Then we'd disable the problematic self-tests (sigh), and DOSEMU and similar tools will be stuck using gross hacks even on new kernels.

Changes from v3:
- Comment fixes from Borislav.
- Improve AR_xyz comments to better match SDM. (Borislav, is this good?)
- Get rid of duplicate AR_xyz defines in the selftest.
- (no non-comment code changes)

Changes from v2:
- Rebased (which wasn't quite trivial since the headers were rearranged)
- Slightly improved a comment in patch 1.
- Tidy up changelogs a bit.

Changes from v1:
- Comment fixes
- Fix screwed up uaccess that broke things

Andy Lutomirski (4):
x86/signal/64: Add a comment about sigcontext->fs and gs x86/signal/64: Fix SS if needed when delivering a 64-bit signal x86/signal/64: Re-add support for SS in the 64-bit signal context selftests/x86: Add tests for UC_SIGCONTEXT_SS and UC_STRICT_RESTORE_SS

 arch/x86/include/asm/desc_defs.h        |  23 ++++
 arch/x86/include/asm/sighandling.h      |   1 -
 arch/x86/include/uapi/asm/sigcontext.h  |  26 +++-
 arch/x86/include/uapi/asm/ucontext.h    |  54 +++++++-
 arch/x86/kernel/signal.c                | 114 +++++++++++++---
 tools/testing/selftests/x86/Makefile    |   7 +-
 tools/testing/selftests/x86/sigreturn.c | 222 ++++++++++++++++++++++++++++----
 7 files changed, 387 insertions(+), 60 deletions(-)
Signature · 1 line
2.5.0
#1 lore raw

PATCH v4 1/4 x86/signal/64: Add a comment about sigcontext->fs and gs

These fields have a strange history. This tries to document it.

This borrows from 9a036b93a344 ("x86/signal/64: Remove 'fs' and 'gs' from sigcontext"), which was reverted by ed596cde9425 ("Revert x86 sigcontext cleanups").

---
 arch/x86/include/uapi/asm/sigcontext.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/x86/include/uapi/asm/sigcontext.h +19 -0
index d485232f1e9f..47dae8150520 100644
@@ -341,6 +341,25 @@ struct sigcontext {
 	__u64				rip;
 	__u64				eflags;		/* RFLAGS */
 	__u16				cs;
+
+	/*
+	 * Prior to 2.5.64 ("[PATCH] x86-64 updates for 2.5.64-bk3"),
+	 * Linux saved and restored fs and gs in these slots.  This
+	 * was counterproductive, as fsbase and gsbase were never
+	 * saved, so arch_prctl was presumably unreliable.
+	 *
+	 * If these slots are ever needed for any other purpose, there
+	 * is some risk that very old 64-bit binaries could get
+	 * confused.  I doubt that many such binaries still work,
+	 * though, since the same patch in 2.5.64 also removed the
+	 * 64-bit set_thread_area syscall, so it appears that there is
+	 * no TLS API beyond modify_ldt that works in both pre- and
+	 * post-2.5.64 kernels.
+	 *
+	 * There is at least one additional concern if these slots are
+	 * recycled for another purpose: some DOSEMU versions stash fs
+	 * and gs in these slots manually.
+	 */
 	__u16				gs;
 	__u16				fs;
 	__u16				__pad0;
Signature · 1 line
2.5.0
#2 lore raw

Re: [PATCH v4 1/4] x86/signal/64: Add a comment about sigcontext->fs and gs

Quoted 30 lines · Andy Lutomirski

These fields have a strange history. This tries to document it.

This borrows from 9a036b93a344 ("x86/signal/64: Remove 'fs' and 'gs' from sigcontext"), which was reverted by ed596cde9425 ("Revert x86 sigcontext cleanups").

---
 arch/x86/include/uapi/asm/sigcontext.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/x86/include/uapi/asm/sigcontext.h +11 -0
index d485232f1e9f..47dae8150520 100644
@@ -341,6 +341,25 @@ struct sigcontext {
 	__u64				rip;
 	__u64				eflags;		/* RFLAGS */
 	__u16				cs;
+
+	/*
+	 * Prior to 2.5.64 ("[PATCH] x86-64 updates for 2.5.64-bk3"),
+	 * Linux saved and restored fs and gs in these slots.  This
+	 * was counterproductive, as fsbase and gsbase were never
+	 * saved, so arch_prctl was presumably unreliable.
+	 *
+	 * If these slots are ever needed for any other purpose, there
+	 * is some risk that very old 64-bit binaries could get
+	 * confused.  I doubt that many such binaries still work,
+	 * though, since the same patch in 2.5.64 also removed the

arch/x86/include/uapi/asm/sigcontext.h struct sigcontext { · line 364

Quoted 10 lines
arch/x86/include/uapi/asm/sigcontext.h +8 -0
+	 * 64-bit set_thread_area syscall, so it appears that there is
+	 * no TLS API beyond modify_ldt that works in both pre- and
+	 * post-2.5.64 kernels.
+	 *
+	 * There is at least one additional concern if these slots are
+	 * recycled for another purpose: some DOSEMU versions stash fs
+	 * and gs in these slots manually.
+	 */
 	__u16				gs;
 	__u16				fs;

So I think this comment should be a lot more assertive: it should state that due to these old legacies that user-space learned to rely on the kernel must not touch these fields. I.e. it is an ABI - no ifs and whens.

We should also rename them to __dosemu_gs_reserved/__dosemu_fs_reserved or so. These are ABI legacies for DOSEMU, no need to pretend otherwise. There's very little to be sorry about: ABI promises have consequences, we should codify that here and move on. Also please document it precisely which syscall(s) expose this ABI.

If we need more space for new, cleaner functionality we'll use other fields. (like your later patches do.)

Thanks,

Ingo

#3 lore raw

Re: [PATCH v4 1/4] x86/signal/64: Add a comment about sigcontext->fs and gs

Quoted 40 lines · Ingo Molnar
Quoted 36 lines · Andy Lutomirski

These fields have a strange history. This tries to document it.

This borrows from 9a036b93a344 ("x86/signal/64: Remove 'fs' and 'gs' from sigcontext"), which was reverted by ed596cde9425 ("Revert x86 sigcontext cleanups").

---
 arch/x86/include/uapi/asm/sigcontext.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/x86/include/uapi/asm/sigcontext.h +17 -0
index d485232f1e9f..47dae8150520 100644
@@ -341,6 +341,25 @@ struct sigcontext {
      __u64                           rip;
      __u64                           eflags;         /* RFLAGS */
      __u16                           cs;
+
+     /*
+      * Prior to 2.5.64 ("[PATCH] x86-64 updates for 2.5.64-bk3"),
+      * Linux saved and restored fs and gs in these slots.  This
+      * was counterproductive, as fsbase and gsbase were never
+      * saved, so arch_prctl was presumably unreliable.
+      *
+      * If these slots are ever needed for any other purpose, there
+      * is some risk that very old 64-bit binaries could get
+      * confused.  I doubt that many such binaries still work,
+      * though, since the same patch in 2.5.64 also removed the
+      * 64-bit set_thread_area syscall, so it appears that there is
+      * no TLS API beyond modify_ldt that works in both pre- and
+      * post-2.5.64 kernels.
+      *
+      * There is at least one additional concern if these slots are
+      * recycled for another purpose: some DOSEMU versions stash fs
Quoted 8 lines

arch/x86/include/uapi/asm/sigcontext.h struct sigcontext { · line 364

Quoted 4 lines
arch/x86/include/uapi/asm/sigcontext.h +2 -0
+      * and gs in these slots manually.
+      */
      __u16                           gs;
      __u16                           fs;

So I think this comment should be a lot more assertive: it should state that due to these old legacies that user-space learned to rely on the kernel must not touch these fields. I.e. it is an ABI - no ifs and whens.

We could still touch them to a limited extent. For example, we could save FS and GS there (but we probably can't restore them).

I'll improve the comment.

Quoted 2 lines

We should also rename them to __dosemu_gs_reserved/__dosemu_fs_reserved or so.

I suspect that DOSEMU won't build if we do that. In any event, I think it should be a separate patch so that it can be trivially reverted if needed.

--Andy

#4 lore raw

PATCH v4 2/4 x86/signal/64: Fix SS if needed when delivering a 64-bit signal

In reply to Andy Lutomirski

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

---
 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 +23 -0
index 278441f39856..eb5deb42484d 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))
+#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)   /* "Accessed" */
+#define AR_S			(1 << 12)  /* If clear, "System" segment */
+#define AR_P			(1 << 15)  /* "Present" */
+#define AR_AVL			(1 << 20)  /* "AVaiLable" (no HW effect) */
+#define AR_L			(1 << 21)  /* "Long mode" for code segments */
+#define AR_DB			(1 << 22)  /* D/B, effect depends on type */
+#define AR_G			(1 << 23)  /* "Granularity" (limit in pages) */
+
 #endif /* _ASM_X86_DESC_DEFS_H */
arch/x86/kernel/signal.c +49 -2

65 lines not on this page · raw message

Signature · 1 line
2.5.0
#5 lore raw

PATCH v4 3/4 x86/signal/64: Re-add support for SS in the 64-bit signal context

In reply to Andy Lutomirski

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]>
---
 arch/x86/include/asm/sighandling.h     |  1 -
 arch/x86/include/uapi/asm/sigcontext.h |  7 ++--
 arch/x86/include/uapi/asm/ucontext.h   | 54 ++++++++++++++++++++++++++---
 arch/x86/kernel/signal.c               | 63 ++++++++++++++++++++++++----------
 tools/testing/selftests/x86/Makefile   |  7 ++--
 5 files changed, 102 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 +5 -2
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;
@@ -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 */
+	};
 	__u64				err;
 	__u64				trapno;
 	__u64				oldmask;
arch/x86/include/uapi/asm/ucontext.h +49 -5

60 lines not on this page · raw message

arch/x86/kernel/signal.c +44 -19

137 lines not on this page · raw message

tools/testing/selftests/x86/Makefile +4 -3
index d0c473f65850..f5a02f19546c 100644
@@ -4,10 +4,11 @@ include ../lib.mk
 
 .PHONY: all all_32 all_64 warn_32bit_failure clean
 
-TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_syscall
-TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault sigreturn test_syscall_vdso unwind_vdso \
+TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt ptrace_syscall \
+	sigreturn \
+	ldt_gdt
+TARGETS_C_32BIT_ONLY := entry_from_vm86 syscall_arg_fault test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
-			ldt_gdt \
 			vdso_restorer
 
 TARGETS_C_32BIT_ALL := $(TARGETS_C_BOTHBITS) $(TARGETS_C_32BIT_ONLY)
Signature · 1 line
2.5.0
#6 lore raw

PATCH v4 4/4 selftests/x86: Add tests for UC_SIGCONTEXT_SS and UC_STRICT_RESTORE_SS

In reply to 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.

---
 tools/testing/selftests/x86/sigreturn.c | 222 ++++++++++++++++++++++++++++----
 1 file changed, 194 insertions(+), 28 deletions(-)
tools/testing/selftests/x86/sigreturn.c +194 -28
index b5aa1bab7416..92a522420a5c 100644
@@ -54,6 +54,29 @@
 #include <sys/ptrace.h>
 #include <sys/user.h>
 
+/* Pull in AR_xyz defines. */
+typedef unsigned int u32;
+typedef unsigned short u16;
+#include "../../../../arch/x86/include/asm/desc_defs.h"
+
+/*
+ * 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.
+ */
+#define UC_SIGCONTEXT_SS       0x2
+#define UC_STRICT_RESTORE_SS   0x4
+#endif
+
 /*
  * In principle, this test can run on Linux emulation layers (e.g.
  * Illumos "LX branded zones").  Solaris-based kernels reserve LDT
@@ -267,6 +290,9 @@ static gregset_t initial_regs, requested_regs, resulting_regs;
 /* Instructions for the SIGUSR1 handler. */
 static volatile unsigned short sig_cs, sig_ss;
 static volatile sig_atomic_t sig_trapped, sig_err, sig_trapno;
+#ifdef __x86_64__
+static volatile sig_atomic_t sig_corrupt_final_ss;
+#endif
 
 /* Abstractions for some 32-bit vs 64-bit differences. */
 #ifdef __x86_64__
@@ -305,9 +331,105 @@ static greg_t *csptr(ucontext_t *ctx)
 }
 #endif
 
+/*
+ * Checks a given selector for its code bitness or returns -1 if it's not
+ * a usable code segment selector.
+ */
+int cs_bitness(unsigned short cs)
+{
+	uint32_t valid = 0, ar;
+	asm ("lar %[cs], %[ar]\n\t"
+	     "jnz 1f\n\t"
+	     "mov $1, %[valid]\n\t"
+	     "1:"
+	     : [ar] "=r" (ar), [valid] "+rm" (valid)
+	     : [cs] "r" (cs));
+
+	if (!valid)
+		return -1;
+
+	bool db = (ar & (1 << 22));
+	bool l = (ar & (1 << 21));
+
+	if (!(ar & (1<<11)))
+	    return -1;	/* Not code. */
+
+	if (l && !db)
+		return 64;
+	else if (!l && db)
+		return 32;
+	else if (!l && !db)
+		return 16;
+	else
+		return -1;	/* Unknown bitness. */
+}
+
+/*
+ * Checks a given selector for its code bitness or returns -1 if it's not
+ * a usable code segment selector.
+ */
+bool is_valid_ss(unsigned short cs)
+{
+	uint32_t valid = 0, ar;
+	asm ("lar %[cs], %[ar]\n\t"
+	     "jnz 1f\n\t"
+	     "mov $1, %[valid]\n\t"
+	     "1:"
+	     : [ar] "=r" (ar), [valid] "+rm" (valid)
+	     : [cs] "r" (cs));
+
+	if (!valid)
+		return false;
+
+	if ((ar & AR_TYPE_MASK) != AR_TYPE_RWDATA &&
+	    (ar & AR_TYPE_MASK) != AR_TYPE_RWDATA_EXPDOWN)
+		return false;
+
+	return (ar & AR_P);
+}
+
 /* Number of errors in the current test case. */
 static volatile sig_atomic_t nerrs;
 
+static void validate_signal_ss(int sig, ucontext_t *ctx)
+{
+#ifdef __x86_64__
+	bool was_64bit = (cs_bitness(*csptr(ctx)) == 64);
+
+	if (!(ctx->uc_flags & UC_SIGCONTEXT_SS)) {
+		printf("[FAIL]\tUC_SIGCONTEXT_SS was not set\n");
+		nerrs++;
+
+		/*
+		 * This happens on Linux 4.1.  The rest will fail, too, so
+		 * return now to reduce the noise.
+		 */
+		return;
+	}
+
+	/* UC_STRICT_RESTORE_SS is set iff we came from 64-bit mode. */
+	if (!!(ctx->uc_flags & UC_STRICT_RESTORE_SS) != was_64bit) {
+		printf("[FAIL]\tUC_STRICT_RESTORE_SS was wrong in signal %d\n",
+		       sig);
+		nerrs++;
+	}
+
+	if (is_valid_ss(*ssptr(ctx))) {
+		/*
+		 * DOSEMU was written before 64-bit sigcontext had SS, and
+		 * it tries to figure out the signal source SS by looking at
+		 * the physical register.  Make sure that keeps working.
+		 */
+		unsigned short hw_ss;
+		asm ("mov %%ss, %0" : "=rm" (hw_ss));
+		if (hw_ss != *ssptr(ctx)) {
+			printf("[FAIL]\tHW SS didn't match saved SS\n");
+			nerrs++;
+		}
+	}
+#endif
+}
+
 /*
  * SIGUSR1 handler.  Sets CS and SS as requested and points IP to the
  * int3 trampoline.  Sets SP to a large known value so that we can see
@@ -317,6 +439,8 @@ static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 {
 	ucontext_t *ctx = (ucontext_t*)ctx_void;
 
+	validate_signal_ss(sig, ctx);
+
 	memcpy(&initial_regs, &ctx->uc_mcontext.gregs, sizeof(gregset_t));
 
 	*csptr(ctx) = sig_cs;
@@ -334,13 +458,16 @@ static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 }
 
 /*
- * Called after a successful sigreturn.  Restores our state so that
- * the original raise(SIGUSR1) returns.
+ * Called after a successful sigreturn (via int3) or from a failed
+ * sigreturn (directly by kernel).  Restores our state so that the
+ * original raise(SIGUSR1) returns.
  */
 static void sigtrap(int sig, siginfo_t *info, void *ctx_void)
 {
 	ucontext_t *ctx = (ucontext_t*)ctx_void;
 
+	validate_signal_ss(sig, ctx);
+
 	sig_err = ctx->uc_mcontext.gregs[REG_ERR];
 	sig_trapno = ctx->uc_mcontext.gregs[REG_TRAPNO];
 
@@ -358,41 +485,62 @@ static void sigtrap(int sig, siginfo_t *info, void *ctx_void)
 	memcpy(&resulting_regs, &ctx->uc_mcontext.gregs, sizeof(gregset_t));
 	memcpy(&ctx->uc_mcontext.gregs, &initial_regs, sizeof(gregset_t));
 
+#ifdef __x86_64__
+	if (sig_corrupt_final_ss) {
+		if (ctx->uc_flags & UC_STRICT_RESTORE_SS) {
+			printf("[FAIL]\tUC_STRICT_RESTORE_SS was set inappropriately\n");
+			nerrs++;
+		} else {
+			/*
+			 * DOSEMU transitions from 32-bit to 64-bit mode by
+			 * adjusting sigcontext, and it requires that this work
+			 * even if the saved SS is bogus.
+			 */
+			printf("\tCorrupting SS on return to 64-bit mode\n");
+			*ssptr(ctx) = 0;
+		}
+	}
+#endif
+
 	sig_trapped = sig;
 }
 
-/*
- * Checks a given selector for its code bitness or returns -1 if it's not
- * a usable code segment selector.
- */
-int cs_bitness(unsigned short cs)
+#ifdef __x86_64__
+/* Tests recovery if !UC_STRICT_RESTORE_SS */
+static void sigusr2(int sig, siginfo_t *info, void *ctx_void)
 {
-	uint32_t valid = 0, ar;
-	asm ("lar %[cs], %[ar]\n\t"
-	     "jnz 1f\n\t"
-	     "mov $1, %[valid]\n\t"
-	     "1:"
-	     : [ar] "=r" (ar), [valid] "+rm" (valid)
-	     : [cs] "r" (cs));
+	ucontext_t *ctx = (ucontext_t*)ctx_void;
 
-	if (!valid)
-		return -1;
+	if (!(ctx->uc_flags & UC_STRICT_RESTORE_SS)) {
+		printf("[FAIL]\traise(2) didn't set UC_STRICT_RESTORE_SS\n");
+		nerrs++;
+		return;  /* We can't do the rest. */
+	}
 
-	bool db = (ar & (1 << 22));
-	bool l = (ar & (1 << 21));
+	ctx->uc_flags &= ~UC_STRICT_RESTORE_SS;
+	*ssptr(ctx) = 0;
 
-	if (!(ar & (1<<11)))
-	    return -1;	/* Not code. */
+	/* Return.  The kernel should recover without sending another signal. */
+}
 
-	if (l && !db)
-		return 64;
-	else if (!l && db)
-		return 32;
-	else if (!l && !db)
-		return 16;
-	else
-		return -1;	/* Unknown bitness. */
+static int test_nonstrict_ss(void)
+{
+	clearhandler(SIGUSR1);
+	clearhandler(SIGTRAP);
+	clearhandler(SIGSEGV);
+	clearhandler(SIGILL);
+	sethandler(SIGUSR2, sigusr2, 0);
+
+	nerrs = 0;
+
+	printf("[RUN]\tClear UC_STRICT_RESTORE_SS and corrupt SS\n");
+	raise(SIGUSR2);
+	if (!nerrs)
+		printf("[OK]\tIt worked\n");
+
+	return nerrs;
 }
+#endif
 
 /* Finds a usable code segment of the requested bitness. */
 int find_cs(int bitness)
@@ -576,6 +724,12 @@ static int test_bad_iret(int cs_bits, unsigned short ss, int force_cs)
 		       errdesc, strsignal(sig_trapped));
 		return 0;
 	} else {
+		/*
+		 * This also implicitly tests UC_STRICT_RESTORE_SS:
+		 * We check that these signals set UC_STRICT_RESTORE_SS and,
+		 * if UC_STRICT_RESTORE_SS doesn't cause strict behavior,
+		 * then we won't get SIGSEGV.
+		 */
 		printf("[FAIL]\tDid not get SIGSEGV\n");
 		return 1;
 	}
@@ -632,6 +786,14 @@ int main()
 						    GDT3(gdt_data16_idx));
 	}
 
+#ifdef __x86_64__
+	/* Nasty ABI case: check SS corruption handling. */
+	sig_corrupt_final_ss = 1;
+	total_nerrs += test_valid_sigreturn(32, false, -1);
+	total_nerrs += test_valid_sigreturn(32, true, -1);
+	sig_corrupt_final_ss = 0;
+#endif
+
 	/*
 	 * We're done testing valid sigreturn cases.  Now we test states
 	 * for which sigreturn itself will succeed but the subsequent
@@ -680,5 +842,9 @@ int main()
 	if (gdt_npdata32_idx)
 		test_bad_iret(32, GDT3(gdt_npdata32_idx), -1);
 
+#ifdef __x86_64__
+	total_nerrs += test_nonstrict_ss();
+#endif
+
 	return total_nerrs ? 1 : 0;
 }
Signature · 1 line
2.5.0
maintainer
#7 lore raw
In reply to Andy Lutomirski
Quoted 15 lines · Andy Lutomirski

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 8 lines

This is a bit risky, and another option would be to do nothing at all. Then we'd disable the problematic self-tests (sigh), and DOSEMU and similar tools will be stuck using gross hacks even on new kernels.

Changes from v3:
- Comment fixes from Borislav.
- Improve AR_xyz comments to better match SDM. (Borislav, is this good?)

Yep, nice.

Thanks!

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.