Skip to contentSkip to conversation

x86_64 signal handling for 64-bit apps w/ mixed 32-bit code - trivial fix

PATCH In review Bryan Ford 3 participants 5 messages Oct 5, 2005 – Oct 7, 2005

lore
Share & tools
lore

Bryan Ford

author
#0 lore raw

PATCH 1/1 x86_64 signal handling for 64-bit apps w/ mixed 32-bit code - trivial fix

The following trivial patch fixes a bug in signal handling on x86-64: the kernel currently fails to save and restore the CS and SS segment registers on user-mode signal handler dispatch/return, which makes it impossible for 64-bit applications to catch and handle signals properly that occur while running 32-bit code fragments in compatibility mode.

The proposed patch doesn't affect any performance-critical paths (e.g., syscall or interrupt entry/exit), and merely involves a couple more moves to/from user space on signal frame setup and sigreturn. It also doesn't affect the size or shape of the sigcontext at all, since there already was an (unused) slot for CS, and I've assigned the convenient __pad0 field as a slot for SS. The existing, unused slots for FS and GS remain unused for now, and I don't see any urgent need to change that. The only way this might break an existing app is if the app tries to cons up its own signal frame (not generated by the kernel) and pass it to sigreturn, but this is presumably a no-no anyway.

The patch is against linux-2.6.13.3.
Author: Bryan Ford, [email protected]
No copyright claimed; public domain.

include/asm-x86_64/sigcontext.h +1 -1
diff -ur o13/include/asm-x86_64/sigcontext.h linux-2.6.13.3/include/asm-x86_64/sigcontext.h
@@ -43,7 +43,7 @@
 	unsigned short cs;
 	unsigned short gs;
 	unsigned short fs;
-	unsigned short __pad0; 
+	unsigned short ss; 
 	unsigned long err;
 	unsigned long trapno;
 	unsigned long oldmask;
arch/x86_64/kernel/signal.c +19 -0
diff -ur o13/arch/x86_64/kernel/signal.c linux-2.6.13.3/arch/x86_64/kernel/signal.c
@@ -110,6 +110,15 @@
 	COPY(r14);
 	COPY(r15);
 
+	/* Kernel saves and restores only CS and DS segments on signals,
+	 * which are the bare essentials needed to allow mixed 32/64-bit code.
+	 * App's signal handler can save/restore other segments if needed. */
+	unsigned short cs, ss;
+	err |= __get_user(cs, &sc->cs);
+	err |= __get_user(ss, &sc->ss);
+	regs->cs = cs | 3;	/* Force into user mode */
+	regs->ss = ss | 3;
+
 	{
 		unsigned int tmpflags;
 		err |= __get_user(tmpflags, &sc->eflags);

@@ -187,6 +196,8 @@
 {
 	int err = 0;
 
+	err |= __put_user(regs->cs, &sc->cs);
+	err |= __put_user(regs->ss, &sc->ss);
 	err |= __put_user(0, &sc->gs);
 	err |= __put_user(0, &sc->fs);
 

@@ -318,7 +329,15 @@
 
 	regs->rsp = (unsigned long)frame;
 
+	/* Set up segment registers to run signal handlers in 64-bit mode,
+	   even if the handler happens to be interrupting 32-bit code. */
+	regs->cs = __USER_CS;
+	regs->ss = __USER_DS;
+
+	/* This, by contrast, has nothing to do with segment registers -
+	   see include/asm-x86_64/uaccess.h for details. */
 	set_fs(USER_DS);
+
 	regs->eflags &= ~TF_MASK;
 	if (test_thread_flag(TIF_SINGLESTEP))
 		ptrace_notify(SIGTRAP);
#1 lore raw
Quoted 60 lines · Bryan Ford

The following trivial patch fixes a bug in signal handling on x86-64: the kernel currently fails to save and restore the CS and SS segment registers on user-mode signal handler dispatch/return, which makes it impossible for 64-bit applications to catch and handle signals properly that occur while running 32-bit code fragments in compatibility mode.

The proposed patch doesn't affect any performance-critical paths (e.g., syscall or interrupt entry/exit), and merely involves a couple more moves to/from user space on signal frame setup and sigreturn. It also doesn't affect the size or shape of the sigcontext at all, since there already was an
(unused) slot for CS, and I've assigned the convenient __pad0 field as a slot for SS. The existing, unused slots for FS and GS remain unused for now, and I don't see any urgent need to change that. The only way this might break an existing app is if the app tries to cons up its own signal frame (not generated by the kernel) and pass it to sigreturn, but this is presumably a no-no anyway.

The patch is against linux-2.6.13.3.
Author: Bryan Ford, [email protected]
No copyright claimed; public domain.

------------------------------------------------------------------------

diff -ur o13/include/asm-x86_64/sigcontext.h linux-2.6.13.3/include/asm-x86_64/sigcontext.h

include/asm-x86_64/sigcontext.h +1 -1
@@ -43,7 +43,7 @@
 	unsigned short cs;
 	unsigned short gs;
 	unsigned short fs;
-	unsigned short __pad0; 
+	unsigned short ss; 
 	unsigned long err;
 	unsigned long trapno;
 	unsigned long oldmask;

diff -ur o13/arch/x86_64/kernel/signal.c linux-2.6.13.3/arch/x86_64/kernel/signal.c

arch/x86_64/kernel/signal.c +13 -0
@@ -110,6 +110,15 @@
 	COPY(r14);
 	COPY(r15);
 
+	/* Kernel saves and restores only CS and DS segments on signals,
+	 * which are the bare essentials needed to allow mixed 32/64-bit code.
+	 * App's signal handler can save/restore other segments if needed. */
+	unsigned short cs, ss;
+	err |= __get_user(cs, &sc->cs);
+	err |= __get_user(ss, &sc->ss);
+	regs->cs = cs | 3;	/* Force into user mode */
+	regs->ss = ss | 3;
+
 	{
 		unsigned int tmpflags;
 		err |= __get_user(tmpflags, &sc->eflags);

@@ -187,6 +196,8 @@
 {
 	int err = 0;
 
+	err |= __put_user(regs->cs, &sc->cs);
+	err |= __put_user(regs->ss, &sc->ss);
 	err |= __put_user(0, &sc->gs);
 	err |= __put_user(0, &sc->fs);
 

@@ -318,7 +329,15 @@
 
 	regs->rsp = (unsigned long)frame;
 
+	/* Set up segment registers to run signal handlers in 64-bit mode,
+	   even if the handler happens to be interrupting 32-bit code. */

arch/x86_64/kernel/signal.c line 343

Quoted 10 lines
arch/x86_64/kernel/signal.c +6 -0
+	regs->cs = __USER_CS;
+	regs->ss = __USER_DS;
+
+	/* This, by contrast, has nothing to do with segment registers -
+	   see include/asm-x86_64/uaccess.h for details. */
 	set_fs(USER_DS);
+
 	regs->eflags &= ~TF_MASK;
 	if (test_thread_flag(TIF_SINGLESTEP))
 		ptrace_notify(SIGTRAP);

What about the opposite? Are there things that would break if the app depends on compatibility mode signal handler?

--Mika

author
#2 lore raw
Quoted 18 lines · Mika Penttilä
Quoted 6 lines · Bryan Ford

The following trivial patch fixes a bug in signal handling on x86-64: the kernel currently fails to save and restore the CS and SS segment registers on user-mode signal handler dispatch/return, which makes it impossible for 64-bit applications to catch and handle signals properly that occur while running 32-bit code fragments in compatibility mode.

Quoted 10 lines

The proposed patch doesn't affect any performance-critical paths (e.g., syscall or interrupt entry/exit), and merely involves a couple more moves to/from user space on signal frame setup and sigreturn. It also doesn't affect the size or shape of the sigcontext at all, since there already was an (unused) slot for CS, and I've assigned the convenient __pad0 field as a slot for SS. The existing, unused slots for FS and GS remain unused for now, and I don't see any urgent need to change that. The only way this might break an existing app is if the app tries to cons up its own signal frame (not generated by the kernel) and pass it to sigreturn, but this is presumably a no-no anyway.

Quoted 2 lines

What about the opposite? Are there things that would break if the app depends on compatibility mode signal handler?

If you're thinking about 32-bit compatibility mode apps, this patch doesn't affect them at all, because signal handling for 32-bit apps is already handled by completely separate code paths (in arch/x86_64/ia32/ia32_signal.c instead of arch/x86_64/kernel/signal.c). And note that the 32-bit ABI's signal handling code path already saves the CS and SS properly, in exactly the same way as my proposed patch does for the 64-bit ABI; my patch effectively just brings the two in line with each other.

There is already no way for a 64-bit app to register and use a compatibility-mode signal handler: the kernel's signal handling code path for the 64-bit ABI always sets up a signal handling frame assuming that the signal handler will be 64-bit, and I see no reason this should be changed. I would merely like it to be possible for a 64-bit app to run snippets of 32-bit code when it needs to, and be able to catch signals that may occur while running that 32-bit code, without immediately dying a horrible flaming death as it does now because of the kernel trying to run a 64-bit signal handler with a 32-bit code segment still loaded.

Cheers,
Bryan

#3 lore raw
In reply to Bryan Ford
Quoted 10 lines · Bryan Ford

The proposed patch doesn't affect any performance-critical paths (e.g., syscall or interrupt entry/exit), and merely involves a couple more moves to/from user space on signal frame setup and sigreturn. It also doesn't affect the size or shape of the sigcontext at all, since there already was an (unused) slot for CS, and I've assigned the convenient __pad0 field as a slot for SS. The existing, unused slots for FS and GS remain unused for now, and I don't see any urgent need to change that. The only way this might break an existing app is if the app tries to cons up its own signal frame (not generated by the kernel) and pass it to sigreturn, but this is presumably a no-no anyway.

I see the point of saving/restore cs, but why ss and not es/ds ?

-Andi

author
#4 lore raw
Quoted 12 lines · Andi Kleen
Quoted 10 lines · Bryan Ford

The proposed patch doesn't affect any performance-critical paths (e.g., syscall or interrupt entry/exit), and merely involves a couple more moves to/from user space on signal frame setup and sigreturn. It also doesn't affect the size or shape of the sigcontext at all, since there already was an (unused) slot for CS, and I've assigned the convenient __pad0 field as a slot for SS. The existing, unused slots for FS and GS remain unused for now, and I don't see any urgent need to change that. The only way this might break an existing app is if the app tries to cons up its own signal frame (not generated by the kernel) and pass it to sigreturn, but this is presumably a no-no anyway.

Quoted 1 line

I see the point of saving/restore cs, but why ss and not es/ds ?

Hmm - I remember at one point thinking there was a reason it was necessary for the kernel to save SS as well as CS, but now I can't remember what it was - and now that you mention it, it does seem unnecessary for the kernel to save SS since its value is irrelevant to the CPU while running the 64-bit signal handler code, and the 64-bit signal handler itself can save and restore it if necessary (as it can with ES/DS/FS/GS).

Saving SS along with CS would make the 64-bit signal path more consistent with the 32-bit signal path and with the way the processor's interrupt/IRET mechanism works, but neither of these considerations seem very important. So on the assumption you'd prefer the code path to do the absolute minimum it can get away with, here's a new version of my previously posted patch, trimmed so it only saves and restores CS. This of course makes it unnecessary even to rename the __pad0 field in struct sigcontext.

Thanks,
Bryan

arch/x86_64/kernel/signal.c +15 -0
diff -ur orig/arch/x86_64/kernel/signal.c linux-2.6.13.3/arch/x86_64/kernel/signal.c
@@ -110,6 +110,13 @@
 	COPY(r14);
 	COPY(r15);
 
+	/* Kernel saves and restores only the CS segment register on signals,
+	 * which is the bare minimum needed to allow mixed 32/64-bit code.
+	 * App's signal handler can save/restore other segments if needed. */
+	unsigned short cs;
+	err |= __get_user(cs, &sc->cs);
+	regs->cs = cs | 3;	/* Force into user mode */
+
 	{
 		unsigned int tmpflags;
 		err |= __get_user(tmpflags, &sc->eflags);

@@ -187,6 +194,7 @@
 {
 	int err = 0;
 
+	err |= __put_user(regs->cs, &sc->cs);
 	err |= __put_user(0, &sc->gs);
 	err |= __put_user(0, &sc->fs);
 

@@ -318,7 +326,14 @@
 
 	regs->rsp = (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. */
+	regs->cs = __USER_CS;
+
+	/* This, by contrast, has nothing to do with segment registers -
+	   see include/asm-x86_64/uaccess.h for details. */
 	set_fs(USER_DS);
+
 	regs->eflags &= ~TF_MASK;
 	if (test_thread_flag(TIF_SINGLESTEP))
 		ptrace_notify(SIGTRAP);