Home Home > GIT Browse
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRoland McGrath <roland@redhat.com>2004-05-22 08:26:09 -0700
committerLinus Torvalds <torvalds@ppc970.osdl.org>2004-05-22 08:26:09 -0700
commitce34221ee271c07599f4e69141532c695b5847ad (patch)
tree3d0d87e7480326c544ab79237a0a417c1a8073f4
parentad9beb315728e494dd66943054426fd550fe44b8 (diff)
[PATCH] bogus sigaltstack calls by rt_sigreturn
There is a longstanding bug in the rt_sigreturn system call. This exists in both 2.4 and 2.6, and for almost every platform. I am referring to this code in sys_rt_sigreturn (arch/i386/kernel/signal.c): if (__copy_from_user(&st, &frame->uc.uc_stack, sizeof(st))) goto badframe; /* It is more difficult to avoid calling this function than to call it and ignore errors. */ /* * THIS CANNOT WORK! "&st" is a kernel address, and "do_sigaltstack()" * takes a user address (and verifies that it is a user address). End * result: it does exactly _nothing_. */ do_sigaltstack(&st, NULL, regs->esp); As the comment says, this is bogus. On vanilla i386 kernels, this is just harmlessly stupid--do_sigaltstack always does nothing and returns -EFAULT. However this code actually bites users on kernels using Ingo Molnar's 4G/4G address space layout changes. There some kernel stack address might very well be a lovely and readable user address as well. When that happens, we make a sigaltstack call with some random buffer, and then the fun begins. To my knowledge, this has produced trouble in the real world only for 4G i386 kernels (RHEL and Fedora "hugemem" kernels) on machines that actually have several GB of physical memory (and in programs that are actually using sigaltstack and handling a lot of signals). However, the same clearly broken code has been blindly copied to most other architecture ports, and off hand I don't know the address space details of any other well enough to know if real kernel stack addresses and real user addresses are in fact disjoint as they are on i386 when not using the nonstandard 4GB address space layout. The obvious intent of the call being there in the first place is to permit a signal handler to diddle its ucontext_t.uc_stack before returning, and have this effect a sigaltstack call on the signal handler return. This is not only an optimization vs doing the extra system call, but makes it possible to make a sigaltstack change when that handler itself was running on the signal stack. AFAICT this has never actually worked before, so certainly noone depends on it. But the code certainly suggests that someone intended at one time for that to be the behavior. Thus I am inclined to fix it so it works in that way, though it has not done so before. It would also be reasonable enough to simply rip out the bogus call and not have this functionality. From the current state of code in both 2.4 and 2.6, there is no fathoming how this broken code came about. It's actually much simpler to just make it work! I can only presume that at some point in the past the sigaltstack implementation functions were different such that this made sense. Of the few ports I've looked at briefly, only the ppc/pc64 porters (go paulus!) actually tried to understand what the i386 code was doing and implemented it correctly rather than just carefully transliterating the bug. The patch below fixes only the i386 and x86_64 versions. The x86_64 patches I have not actually tested. I think each and every arch (except ppc and ppc64) need to make the corresponding fixes as well. Note that there is a function to fix for each native arch, and then one for each emulation flavor. The details differ minutely for getting the calls right in each emulation flavor, but I think that most or all of the arch's with biarch/emulation support have similar enough code that each emulation flavor's fix will look very much like the arch/x86_64/ia32/ia32_signal.c patch here.
-rw-r--r--arch/i386/kernel/signal.c11
-rw-r--r--arch/x86_64/ia32/ia32_signal.c10
-rw-r--r--arch/x86_64/kernel/signal.c7
3 files changed, 3 insertions, 25 deletions
diff --git a/arch/i386/kernel/signal.c b/arch/i386/kernel/signal.c
index aa242e932ca9..90b2989b7efb 100644
--- a/arch/i386/kernel/signal.c
+++ b/arch/i386/kernel/signal.c
@@ -231,7 +231,6 @@ asmlinkage int sys_rt_sigreturn(unsigned long __unused)
struct pt_regs *regs = (struct pt_regs *) &__unused;
struct rt_sigframe __user *frame = (struct rt_sigframe __user *)(regs->esp - 4);
sigset_t set;
- stack_t st;
int eax;
if (verify_area(VERIFY_READ, frame, sizeof(*frame)))
@@ -248,16 +247,8 @@ asmlinkage int sys_rt_sigreturn(unsigned long __unused)
if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &eax))
goto badframe;
- if (__copy_from_user(&st, &frame->uc.uc_stack, sizeof(st)))
+ if (do_sigaltstack(&frame->uc.uc_stack, NULL, regs->esp) == -EFAULT)
goto badframe;
- /* It is more difficult to avoid calling this function than to
- call it and ignore errors. */
- /*
- * THIS CANNOT WORK! "&st" is a kernel address, and "do_sigaltstack()"
- * takes a user address (and verifies that it is a user address). End
- * result: it does exactly _nothing_.
- */
- do_sigaltstack(&st, NULL, regs->esp);
return eax;
diff --git a/arch/x86_64/ia32/ia32_signal.c b/arch/x86_64/ia32/ia32_signal.c
index 44491d1976d7..7e95c0a6eec1 100644
--- a/arch/x86_64/ia32/ia32_signal.c
+++ b/arch/x86_64/ia32/ia32_signal.c
@@ -323,16 +323,8 @@ asmlinkage long sys32_rt_sigreturn(struct pt_regs regs)
if (ia32_restore_sigcontext(&regs, &frame->uc.uc_mcontext, &eax))
goto badframe;
- if (__copy_from_user(&st, &frame->uc.uc_stack, sizeof(st)))
+ if (sys32_sigaltstack(&frame->uc.uc_stack, NULL, regs) == -EFAULT)
goto badframe;
- /* It is more difficult to avoid calling this function than to
- call it and ignore errors. */
- {
- mm_segment_t oldds = get_fs();
- set_fs(KERNEL_DS);
- do_sigaltstack(&st, NULL, regs.rsp);
- set_fs(oldds);
- }
return eax;
diff --git a/arch/x86_64/kernel/signal.c b/arch/x86_64/kernel/signal.c
index ecf0e46d74c5..fc81ceb324d5 100644
--- a/arch/x86_64/kernel/signal.c
+++ b/arch/x86_64/kernel/signal.c
@@ -138,7 +138,6 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs regs)
{
struct rt_sigframe *frame = (struct rt_sigframe *)(regs.rsp - 8);
sigset_t set;
- stack_t st;
long eax;
if (verify_area(VERIFY_READ, frame, sizeof(*frame))) {
@@ -162,12 +161,8 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs regs)
printk("%d sigreturn rip:%lx rsp:%lx frame:%p rax:%lx\n",current->pid,regs.rip,regs.rsp,frame,eax);
#endif
- if (__copy_from_user(&st, &frame->uc.uc_stack, sizeof(st))) {
+ if (do_sigaltstack(&frame->uc.uc_stack, NULL, regs.rsp) == -EFAULT)
goto badframe;
- }
- /* It is more difficult to avoid calling this function than to
- call it and ignore errors. */
- do_sigaltstack(&st, NULL, regs.rsp);
return eax;