[PATCH] i386: handle iret faults better

[PATCH] i386: handle iret faults better

[Roland McGrath]

A user process can cause a kernel-mode fault in the iret instruction, by
setting an invalid %cs and such, via ptrace or sigreturn.  The fixup code
now calls do_exit(11), so a process will die with SIGSEGV but never
generate a core dump.  This is vastly more confusing in a multithreaded
program, because do_exit just kills that one thread and so it appears to
mysteriously disappear when it calls sigreturn.  

This patch makes faults in iret produce the normal signals that would
result from the same errors when executing some user-mode instruction.  
It requires some special case hair rather than just better fixup code,
because the full trap details that go into the signal info are not
available to the fixup code.


Thanks,
Roland


Signed-off-by: Roland McGrath <roland@redhat.com>

--- linux-2.6/arch/i386/kernel/entry.S
+++ linux-2.6/arch/i386/kernel/entry.S
@@ -254,14 +253,23 @@ restore_nocheck:
 	RESTORE_REGS
 	addl $4, %esp
 1:	iret
+	/*
+	 * Traps in iret mean that userland tried to restore a bogus
+	 * cs, eip, ss, esp, or eflags.  Some kinds of bogosity just cause
+	 * a trap after the iret returns, but some will cause a trap in
+	 * iret itself.  We want to treat those as if the restored user
+	 * state is what caused that trap, i.e. produce the appropriate signal.
+	 * Since normal .fixup code doesn't have access to the trap info,
+	 * traps.c has a special case for iret.  It's already generated the
+	 * signal before we resume at iret_exc.  Now we just need to recover
+	 * the whole frame we were trying to restore, so it can be seen on
+	 * our stack by the debugger.
+	 */
 .section .fixup,"ax"
-iret_exc:
-	sti
-	movl $__USER_DS, %edx
-	movl %edx, %ds
-	movl %edx, %es
-	movl $11,%eax
-	call do_exit
+ENTRY(iret_exc)
+	pushl $0			# orig_eax was lost
+	SAVE_ALL
+	jmp ret_from_exception
 .previous
 .section __ex_table,"a"
 	.align 4
--- linux-2.6/arch/i386/kernel/traps.c
+++ linux-2.6/arch/i386/kernel/traps.c
@@ -357,6 +357,46 @@ static inline void die_if_kernel(const c
 		die(str, regs, err);
 }
 
+/*
+ * When we get an exception in the iret instructions in entry.S, whatever
+ * fault it is really belongs to the user state we are restoring.  We want
+ * to turn it into a signal.  To make that signal's info exactly match what
+ * this same kind of fault in a user instruction would show, the fixup
+ * needs to know the trapno and error code.  But those are lost when we get
+ * back to the fixup entrypoint.  So we have a special case for the iret
+ * fixups, and generate the signal here like a normal user trap would.
+ * Then the fixup code restores the pt_regs on the base of the stack to
+ * the bogus user state it was trying to return to, before handling the signal.
+ */
+extern void iret_exc(void);	/* entry.S label for code in .fixup */
+static inline int is_iret(struct pt_regs *regs)
+{
+	return (regs->eip == (unsigned long)&iret_exc);
+}
+
+/*
+ * If the iret was actually trying to return to kernel mode,
+ * that should be an oops.
+ */
+static inline int iret_to_user(struct pt_regs *regs)
+{
+	/*
+	 * The frame being restored was all popped off and restored except
+	 * the last five words that iret pops.  Instead of popping, it
+	 * pushed another trap frame, clobbering the part of the old one
+	 * that we had already restored.  So the restored registers are now
+	 * all back in the new trap frame, but the eip et al show the
+	 * in-kernel state at the iret instruction.  The bad state we tried
+	 * to restore with iret is still on the stack, right below our
+	 * current trap frame.  The current trap frame was for an in-kernel
+	 * trap, and so doesn't include the esp and ss words--so &regs->esp
+	 * is where %esp was before the iret.
+	 */
+	struct pt_regs *oregs = container_of(&regs->esp, struct pt_regs, eip);
+
+	return likely((oregs->xcs & 3) == 3);
+}
+
 static void do_trap(int trapnr, int signr, char *str, int vm86,
 			   struct pt_regs * regs, long error_code, siginfo_t *info)
 {
@@ -381,8 +421,16 @@ static void do_trap(int trapnr, int sign
 	}
 
 	kernel_trap: {
-		if (!fixup_exception(regs))
+		if (!fixup_exception(regs)) {
+		die:
 			die(str, regs, error_code);
+		}
+		else if (is_iret(regs)) {
+			if (!iret_to_user(regs))
+				goto die;
+			local_irq_enable();
+			goto trap_signal;
+		}
 		return;
 	}
 
@@ -490,6 +538,7 @@ fastcall void do_general_protection(stru
 	if (!(regs->xcs & 3))
 		goto gp_in_kernel;
 
+gp_in_user:
 	current->thread.error_code = error_code;
 	current->thread.trap_no = 13;
 	force_sig(SIGSEGV, current);
@@ -502,11 +551,18 @@ gp_in_vm86:
 
 gp_in_kernel:
 	if (!fixup_exception(regs)) {
+	die:
 		if (notify_die(DIE_GPF, "general protection fault", regs,
 				error_code, 13, SIGSEGV) == NOTIFY_STOP)
 			return;
 		die("general protection fault", regs, error_code);
 	}
+	else if (is_iret(regs)) {
+		if (!iret_to_user(regs))
+			goto die;
+		local_irq_enable();
+		goto gp_in_user;
+	}
 }
 
 static void mem_parity_error(unsigned char reason, struct pt_regs * regs)

Re: [PATCH] i386: handle iret faults better

[Andi Kleen]

> I really ended up deciding that we can fix it with a simple one-liner 
> instead, which actually simplifies and cleans up the code, instead of 
> adding new special cases.
> 
> I just committed the appended, which actually removes one line more than 
> it adds.

Thanks looks great. I will add the equivalent to x86-64.

-Andi

Re: [PATCH] i386: handle iret faults better

[Linus Torvalds]

On Thu, 28 Apr 2005, Roland McGrath wrote:
>
> I was never very happy with the special-case check for iret_exc either.
> But for the first crack, I went for the fix that didn't touch other
> infrastructure code.
> 
> The fault.c changes here are really not necessary for the bug fix at all,
> it will never be used there.  But to make it a clean infrastructure
> upgrade, I made every caller of fixup_exception consistently pass in the
> complete info it uses for signals in the user-mode case.

I really ended up deciding that we can fix it with a simple one-liner 
instead, which actually simplifies and cleans up the code, instead of 
adding new special cases.

I just committed the appended, which actually removes one line more than 
it adds.

		Linus
-----
commit a879cbbb34cbecfa9707fbb6e5a00c503ac1ecb9
tree fdf247f8dedea8f04d0989aeab6922ed073eee11
parent c06fec5022ebe014af876da2df4a0eee836e97c8
author Linus Torvalds <torvalds@ppc970.osdl.org> Fri, 29 Apr 2005 09:38:44 -0700
committer Linus Torvalds <torvalds@ppc970.osdl.org> Fri, 29 Apr 2005 09:38:44 -0700

    x86: make traps on 'iret' be debuggable in user space

    This makes a trap on the 'iret' that returns us to user space
    cause a nice clean SIGSEGV, instead of just a hard (and silent)
    exit.

    That way a debugger can actually try to see what happened, and
    we also properly notify everybody who might be interested about
    us being gone.

    This loses the error code, but tells the debugger what happened
    with ILL_BADSTK in the siginfo.

--- k/arch/i386/kernel/entry.S  (mode:100644)
+++ l/arch/i386/kernel/entry.S  (mode:100644)
@@ -260,11 +260,9 @@ restore_nocheck:
 .section .fixup,"ax"
 iret_exc:
 	sti
-	movl $__USER_DS, %edx
-	movl %edx, %ds
-	movl %edx, %es
-	movl $11,%eax
-	call do_exit
+	pushl $0			# no error code
+	pushl $do_iret_error
+	jmp error_code
 .previous
 .section __ex_table,"a"
 	.align 4
--- k/arch/i386/kernel/traps.c  (mode:100644)
+++ l/arch/i386/kernel/traps.c  (mode:100644)
@@ -451,6 +451,7 @@ DO_ERROR(10, SIGSEGV, "invalid TSS", inv
 DO_ERROR(11, SIGBUS,  "segment not present", segment_not_present)
 DO_ERROR(12, SIGBUS,  "stack segment", stack_segment)
 DO_ERROR_INFO(17, SIGBUS, "alignment check", alignment_check, BUS_ADRALN, 0)
+DO_ERROR_INFO(32, SIGSEGV, "iret exception", iret_error, ILL_BADSTK, 0)
 
 fastcall void do_general_protection(struct pt_regs * regs, long error_code)
 {

Re: [PATCH] i386: handle iret faults better

[Andi Kleen]

On Thu, Apr 28, 2005 at 08:40:12PM -0700, Roland McGrath wrote:
> I was never very happy with the special-case check for iret_exc either.
> But for the first crack, I went for the fix that didn't touch other
> infrastructure code.
> 
> The fault.c changes here are really not necessary for the bug fix at all,
> it will never be used there.  But to make it a clean infrastructure
> upgrade, I made every caller of fixup_exception consistently pass in the
> complete info it uses for signals in the user-mode case.
> 
> 
> A user process can cause a kernel-mode fault in the iret instruction, by
> setting an invalid %cs and such, via ptrace or sigreturn.  The fixup code
> now calls do_exit(11), so a process will die with SIGSEGV but never
> generate a core dump.  This is vastly more confusing in a multithreaded
> program, because do_exit just kills that one thread and so it appears to
> mysteriously disappear when it calls sigreturn.
> 
> This patch makes faults in iret produce the normal signals that would
> result from the same errors when executing some user-mode instruction.
> To accomplish this, I've extended the exception_table mechanism to support
> "special fixups".  Instead of a PC location to jump to, these have a
> function called in the trap handler context and passed the full trap details.
> 

I think this is still far too complicated. Or at least I would
not accept a similar patch for x86-64 :) The infrastructure
seems also needlessly complicated to me, because I dont know
what it should be used for except for this.

Is it really that hard to just cause a signal in the 
iret exception handler in entry.S?  You can get the trapno
there anyways from orig_rax (at least on x86-64)

And having all that complicated code just to get the trapno
seems quite pointless to me. Especially since nobody
uses it it will be likely bitrotten more often than working
anyways, so it is not very useful. The error code is 
in the stack frame anyways and you can get it from there.
And the trapno can just be faked to something.

-Andi

[PATCH] i386: handle iret faults better

[Roland McGrath]

I was never very happy with the special-case check for iret_exc either.
But for the first crack, I went for the fix that didn't touch other
infrastructure code.

The fault.c changes here are really not necessary for the bug fix at all,
it will never be used there.  But to make it a clean infrastructure
upgrade, I made every caller of fixup_exception consistently pass in the
complete info it uses for signals in the user-mode case.


A user process can cause a kernel-mode fault in the iret instruction, by
setting an invalid %cs and such, via ptrace or sigreturn.  The fixup code
now calls do_exit(11), so a process will die with SIGSEGV but never
generate a core dump.  This is vastly more confusing in a multithreaded
program, because do_exit just kills that one thread and so it appears to
mysteriously disappear when it calls sigreturn.

This patch makes faults in iret produce the normal signals that would
result from the same errors when executing some user-mode instruction.
To accomplish this, I've extended the exception_table mechanism to support
"special fixups".  Instead of a PC location to jump to, these have a
function called in the trap handler context and passed the full trap details.

Signed-off-by: Roland McGrath <roland@redhat.com>

---
commit 66b1fbcee2c94a5fd9d199e48a25d5f6382f6f05
tree cb5e97a950137e15a400501ab3aa95fe539d0a45
parent f6bbd250c19529f6efebc6c8bd7d3495b98e5eca
author Roland McGrath <roland@redhat.com> 1114589292 -0700
committer Roland McGrath <roland@redhat.com> 1114589292 -0700

Index: arch/i386/kernel/entry.S
===================================================================
--- 23cf0cf93e225498eea50480a44b038be19b333d/arch/i386/kernel/entry.S  (mode:100644 sha1:3c73dc865ead3ad2323098a3b78b0fc8042e7489)
+++ cb5e97a950137e15a400501ab3aa95fe539d0a45/arch/i386/kernel/entry.S  (mode:100644 sha1:d8e602ca337b434e48f2014048f7ecdef221198f)
@@ -75,6 +75,9 @@
 NT_MASK		= 0x00004000
 VM_MASK		= 0x00020000
 
+/* This marks an __ex_table entry that has an exception_fixup_t handler.  */
+#define SPECIAL_FIXUP(function)	(function - __PAGE_OFFSET)
+
 #ifdef CONFIG_PREEMPT
 #define preempt_stop		cli
 #else
@@ -257,18 +260,27 @@
 	RESTORE_REGS
 	addl $4, %esp
 1:	iret
+	/*
+	 * Traps in iret mean that userland tried to restore a bogus
+	 * cs, eip, ss, esp, or eflags.  Some kinds of bogosity just cause
+	 * a trap after the iret returns, but some will cause a trap in
+	 * iret itself.  We want to treat those as if the restored user
+	 * state is what caused that trap, i.e. produce the appropriate signal.
+	 * Since normal .fixup code doesn't have access to the trap info,
+	 * traps.c has a special case for iret.  It's already generated the
+	 * signal before we resume at iret_exc.  Now we just need to recover
+	 * the whole frame we were trying to restore, so it can be seen on
+	 * our stack by the debugger.
+	 */
 .section .fixup,"ax"
-iret_exc:
-	sti
-	movl $__USER_DS, %edx
-	movl %edx, %ds
-	movl %edx, %es
-	movl $11,%eax
-	call do_exit
+ENTRY(iret_exc)
+	pushl $0			# orig_eax was lost
+	SAVE_ALL
+	jmp ret_from_exception
 .previous
 .section __ex_table,"a"
 	.align 4
-	.long 1b,iret_exc
+	.long 1b,SPECIAL_FIXUP(fixup_iret)
 .previous
 
 ldt_ss:
@@ -293,7 +305,7 @@
 1:	iret
 .section __ex_table,"a"
 	.align 4
-	.long 1b,iret_exc
+	.long 1b,SPECIAL_FIXUP(fixup_iret)
 .previous
 
 	# perform work that needs to be done immediately before resumption
@@ -589,7 +601,7 @@
 1:	iret
 .section __ex_table,"a"
 	.align 4
-	.long 1b,iret_exc
+	.long 1b,SPECIAL_FIXUP(fixup_iret)
 .previous
 
 ENTRY(int3)
Index: arch/i386/kernel/traps.c
===================================================================
--- 23cf0cf93e225498eea50480a44b038be19b333d/arch/i386/kernel/traps.c  (mode:100644 sha1:6c0e383915b6a0d9fc516a04f328020d18b575b5)
+++ cb5e97a950137e15a400501ab3aa95fe539d0a45/arch/i386/kernel/traps.c  (mode:100644 sha1:12b09606ace52f8d8525b6e4f1d2987ab8873308)
@@ -357,6 +357,60 @@
 		die(str, regs, err);
 }
 
+/*
+ * When we get an exception in the iret instructions in entry.S, whatever
+ * fault it is really belongs to the user state we are restoring.  We want
+ * to turn it into a signal.  To make that signal's info exactly match what
+ * this same kind of fault in a user instruction would show, the fixup
+ * needs to know the trapno and error code.  So, we use this special fixup
+ * hook for the iret instructions.  If the iret was returning to user mode,
+ * we force a signal like the same exception in user mode would have.  Then
+ * the fixup code at iret_exc (entry.S) restores the pt_regs on the base of
+ * the stack to the bogus user state it was trying to return to, before
+ * handling the signal.
+ */
+int fixup_iret(struct pt_regs *regs, int trapnr, int signr,
+	       unsigned long error_code, siginfo_t *info)
+{
+	/*
+	 * The frame being restored was all popped off and restored except
+	 * the last five words that iret pops.  Instead of popping, it
+	 * pushed another trap frame, clobbering the part of the old one
+	 * that we had already restored.  So the restored registers are now
+	 * all back in the new trap frame, but the eip et al show the
+	 * in-kernel state at the iret instruction.  The bad state we tried
+	 * to restore with iret is still on the stack, right below our
+	 * current trap frame.  The current trap frame was for an in-kernel
+	 * trap, and so doesn't include the esp and ss words--so &regs->esp
+	 * is where %esp was before the iret.
+	 */
+	struct pt_regs *oregs = container_of(&regs->esp, struct pt_regs, eip);
+	struct task_struct *tsk = current;
+
+	if (likely((oregs->xcs & 3) == 3)) {
+		/*
+		 * The iret was trying to return to user mode.
+		 * Turn this into a signal like do_trap would normally do.
+		 */
+		extern void iret_exc(void);
+		local_irq_enable();
+		tsk->thread.error_code = error_code;
+		tsk->thread.trap_no = trapnr;
+		if (info)
+			force_sig_info(signr, info, tsk);
+		else
+			force_sig(signr, tsk);
+		regs->eip = (unsigned long) &iret_exc;
+		return 1;
+	}
+
+	/*
+	 * The iret was actually trying to return to kernel mode,
+	 * so in fact we didn't handle this exception.
+	 */
+	return 0;
+}
+
 static void do_trap(int trapnr, int signr, char *str, int vm86,
 			   struct pt_regs * regs, long error_code, siginfo_t *info)
 {
@@ -381,7 +435,7 @@
 	}
 
 	kernel_trap: {
-		if (!fixup_exception(regs))
+		if (!fixup_exception(regs, trapnr, signr, error_code, info))
 			die(str, regs, error_code);
 		return;
 	}
@@ -501,7 +555,7 @@
 	return;
 
 gp_in_kernel:
-	if (!fixup_exception(regs)) {
+	if (!fixup_exception(regs, 13, SIGSEGV, error_code, NULL)) {
 		if (notify_die(DIE_GPF, "general protection fault", regs,
 				error_code, 13, SIGSEGV) == NOTIFY_STOP)
 			return;
Index: arch/i386/mm/extable.c
===================================================================
--- 23cf0cf93e225498eea50480a44b038be19b333d/arch/i386/mm/extable.c  (mode:100644 sha1:f706449319c4577e5d944561b295d199449bcc02)
+++ cb5e97a950137e15a400501ab3aa95fe539d0a45/arch/i386/mm/extable.c  (mode:100644 sha1:14eec19b079d5afcee76547b4e9a7e33ec384cff)
@@ -7,7 +7,8 @@
 #include <linux/spinlock.h>
 #include <asm/uaccess.h>
 
-int fixup_exception(struct pt_regs *regs)
+int fixup_exception(struct pt_regs *regs, int trapnr, int signr,
+		    unsigned long error_code, siginfo_t *info)
 {
 	const struct exception_table_entry *fixup;
 
@@ -28,6 +29,10 @@
 
 	fixup = search_exception_tables(regs->eip);
 	if (fixup) {
+		if (fixup_is_special(fixup)) {
+			exception_fixup_t *hook = special_fixup_handler(fixup);
+			return (*hook) (regs, trapnr, signr, error_code, info);
+		}
 		regs->eip = fixup->fixup;
 		return 1;
 	}
Index: arch/i386/mm/fault.c
===================================================================
--- 23cf0cf93e225498eea50480a44b038be19b333d/arch/i386/mm/fault.c  (mode:100644 sha1:a509237c4815ed6f8443e85b973dda2a0f2c0688)
+++ cb5e97a950137e15a400501ab3aa95fe539d0a45/arch/i386/mm/fault.c  (mode:100644 sha1:be670e7a9593e5a349408262400d6e31296db92d)
@@ -374,6 +374,12 @@
 	up_read(&mm->mmap_sem);
 
 bad_area_nosemaphore:
+	/* Set up info passed to force_sig_info or to fixup_exception.  */
+	info.si_signo = SIGSEGV;
+	info.si_errno = 0;
+	/* info.si_code has been set above */
+	info.si_addr = (void __user *)address;
+
 	/* User mode accesses just cause a SIGSEGV */
 	if (error_code & 4) {
 		/* 
@@ -387,10 +393,6 @@
 		/* Kernel addresses are always protection faults */
 		tsk->thread.error_code = error_code | (address >= TASK_SIZE);
 		tsk->thread.trap_no = 14;
-		info.si_signo = SIGSEGV;
-		info.si_errno = 0;
-		/* info.si_code has been set above */
-		info.si_addr = (void __user *)address;
 		force_sig_info(SIGSEGV, &info, tsk);
 		return;
 	}
@@ -413,7 +415,7 @@
 
 no_context:
 	/* Are we prepared to handle this kernel fault?  */
-	if (fixup_exception(regs))
+	if (fixup_exception(regs, 14, info.si_signo, error_code, &info))
 		return;
 
 	/* 
@@ -481,11 +483,20 @@
 	printk("VM: killing process %s\n", tsk->comm);
 	if (error_code & 4)
 		do_exit(SIGKILL);
+	info.si_signo = SIGKILL;
+	info.si_errno = 0;
+	info.si_addr = (void __user *)address;
 	goto no_context;
 
 do_sigbus:
 	up_read(&mm->mmap_sem);
 
+	/* Set up info passed to force_sig_info or to fixup_exception.  */
+	info.si_signo = SIGBUS;
+	info.si_errno = 0;
+	info.si_code = BUS_ADRERR;
+	info.si_addr = (void __user *)address;
+
 	/* Kernel mode? Handle exceptions or die */
 	if (!(error_code & 4))
 		goto no_context;
@@ -497,14 +508,15 @@
 	tsk->thread.cr2 = address;
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_no = 14;
-	info.si_signo = SIGBUS;
-	info.si_errno = 0;
-	info.si_code = BUS_ADRERR;
-	info.si_addr = (void __user *)address;
 	force_sig_info(SIGBUS, &info, tsk);
 	return;
 
 vmalloc_fault:
+	/* Set up info passed to fixup_exception.  */
+	info.si_signo = SIGSEGV;
+	info.si_errno = 0;
+	/* info.si_code has been set above */
+	info.si_addr = (void __user *)address;
 	{
 		/*
 		 * Synchronize this task's top level page-table
Index: include/asm-i386/uaccess.h
===================================================================
--- 23cf0cf93e225498eea50480a44b038be19b333d/include/asm-i386/uaccess.h  (mode:100644 sha1:886867aea947094185fcac17610acc67f9310caf)
+++ cb5e97a950137e15a400501ab3aa95fe539d0a45/include/asm-i386/uaccess.h  (mode:100644 sha1:50cd8556e8825a5c6f42ec4dc062e952c27317d3)
@@ -124,8 +124,19 @@
 {
 	unsigned long insn, fixup;
 };
-
-extern int fixup_exception(struct pt_regs *regs);
+static inline int fixup_is_special(const struct exception_table_entry *entry)
+{
+	return unlikely(entry->fixup < PAGE_OFFSET);
+}
+struct siginfo;
+typedef int exception_fixup_t(struct pt_regs *regs, int trapnr, int signr,
+			      unsigned long err, struct siginfo *info);
+static inline exception_fixup_t *
+special_fixup_handler(const struct exception_table_entry *entry)
+{
+	return (exception_fixup_t *) (entry->fixup + PAGE_OFFSET);
+}
+extern exception_fixup_t fixup_exception;
 
 /*
  * These are the main single-value transfer routines.  They automatically