[patch] proposed fix for ptrace() SMP race

[patch] proposed fix for ptrace() SMP race

[David Mosberger]

There is currently a nasty race condition in ptrace().  The effect of
this varies from one platform to another but, for example, on ia64, it
could have the effect of corrupting the state of register f32-f127.
The problem is that ptrace() uses the expression (child->state ==
TASK_STOPPED) to determine whether or not a task has stopped
execution.  On SMP, this is not sufficient because the task may still
be executing while child->has_cpu is true.  This is easy to fix, but
there is a related concern: what would happen if a ptrace'd task was
woken up by another thread while ptrace() is running?  You could argue
that this is just a bug in the user-level program, but I'm worried
that the resulting race conditions could corrupt kernel data
structures and therefore could provide a means to attack the integrity
of the kernel.  The patch below is an attempt to address this issue by
clearing child->cpus_allowed while ptrace() is running.  This should
have the desired effect and as far as I know, it has no negative
impact.  However, it doesn't strike me as the most elegant solution
either and it could well be there there are some problems with it that
I'm overlooking.  Anyhow, I'd appreciate it if the maintainers of the
other platforms could take a look at the patch below and share their
thoughts.  The patch below fixes only the ia64 tree, but if the patch
is agreeable, I can make the same update for the other platforms as
well.

	--david

--- lia64/kernel/ptrace.c	Mon Jul 23 13:51:13 2001
+++ lia64-kdb/kernel/ptrace.c	Tue Sep  4 18:22:49 2001
@@ -60,6 +60,40 @@
 	return -EPERM;
 }
 
+long ptrace_freeze_child(struct task_struct *child, long request, unsigned long *freeze_info)
+{
+	unsigned long old_cpus_allowed;
+
+	if (request == PTRACE_KILL)
+		return 0;
+
+	if (child->state != TASK_STOPPED)
+		return -ESRCH;
+
+#ifdef CONFIG_SMP
+	while (child->has_cpu) {
+		if (child->state != TASK_STOPPED)
+			return -ESRCH;
+		barrier();
+	}
+#endif
+
+	old_cpus_allowed = xchg(&child->cpus_allowed, 0);
+
+	if (child->state != TASK_STOPPED) {
+		xchg(&child->cpus_allowed, old_cpus_allowed);
+		return -ESRCH;
+	}
+
+	*freeze_info = old_cpus_allowed;
+	return 0;
+}
+
+void ptrace_thaw_child(struct task_struct *child, long request, unsigned long freeze_info)
+{
+	if (request != PTRACE_KILL)
+		xchg(&child->cpus_allowed, freeze_info);
+}
 
 /*
  * Access another process' address space, one page at a time.
--- lia64/arch/ia64/kernel/ptrace.c	Mon Jul 23 13:50:57 2001
+++ lia64-kdb/arch/ia64/kernel/ptrace.c	Tue Sep  4 16:45:03 2001
@@ -794,7 +794,7 @@
 	    long arg4, long arg5, long arg6, long arg7, long stack)
 {
 	struct pt_regs *pt, *regs = (struct pt_regs *) &stack;
-	unsigned long flags, urbs_end;
+	unsigned long flags, urbs_end, freeze_info;
 	struct task_struct *child;
 	struct switch_stack *sw;
 	long ret;
@@ -840,6 +840,10 @@
 	if (child->p_pptr != current)
 		goto out_tsk;
 
+	ret = ptrace_freeze_child(child, request, &freeze_info);
+	if (ret < 0)
+		goto out_tsk;
+
 	pt = ia64_task_regs(child);
 	sw = (struct switch_stack *) (child->thread.ksp + 16);
 
@@ -856,7 +860,7 @@
 			ret = data;
 			regs->r8 = 0;	/* ensure "ret" is not mistaken as an error code */
 		}
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_POKETEXT:
 	      case PTRACE_POKEDATA:		/* write the word at location addr */
@@ -865,45 +869,45 @@
 			threads_sync_user_rbs(child, urbs_end, 1);
 
 		ret = ia64_poke(child, sw, urbs_end, addr, data);
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_PEEKUSR:		/* read the word at addr in the USER area */
 		if (access_uarea(child, addr, &data, 0) < 0) {
 			ret = -EIO;
-			goto out_tsk;
+			goto out_thaw;
 		}
 		ret = data;
 		regs->r8 = 0;	/* ensure "ret" is not mistaken as an error code */
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_POKEUSR:	      /* write the word at addr in the USER area */
 		if (access_uarea(child, addr, &data, 1) < 0) {
 			ret = -EIO;
-			goto out_tsk;
+			goto out_thaw;
 		}
 		ret = 0;
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_GETSIGINFO:
 		ret = -EIO;
 		if (!access_ok(VERIFY_WRITE, data, sizeof (siginfo_t)) || !child->thread.siginfo)
-			goto out_tsk;
+			goto out_thaw;
 		ret = copy_siginfo_to_user((siginfo_t *) data, child->thread.siginfo);
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_SETSIGINFO:
 		ret = -EIO;
 		if (!access_ok(VERIFY_READ, data, sizeof (siginfo_t))
 		    || child->thread.siginfo == 0)
-			goto out_tsk;
+			goto out_thaw;
 		ret = copy_siginfo_from_user(child->thread.siginfo, (siginfo_t *) data);
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_SYSCALL:	/* continue and stop at next (return from) syscall */
 	      case PTRACE_CONT:		/* restart after signal. */
 		ret = -EIO;
 		if (data > _NSIG)
-			goto out_tsk;
+			goto out_thaw;
 		if (request == PTRACE_SYSCALL)
 			child->ptrace |= PT_TRACESYS;
 		else
@@ -919,7 +923,7 @@
 
 		wake_up_process(child);
 		ret = 0;
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_KILL:
 		/*
@@ -928,7 +932,7 @@
 		 * that it wants to exit.
 		 */
 		if (child->state == TASK_ZOMBIE)		/* already dead */
-			goto out_tsk;
+			goto out_thaw;
 		child->exit_code = SIGKILL;
 
 		/* make sure the single step/take-branch tra bits are not set: */
@@ -940,13 +944,13 @@
 
 		wake_up_process(child);
 		ret = 0;
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_SINGLESTEP:		/* let child execute for one instruction */
 	      case PTRACE_SINGLEBLOCK:
 		ret = -EIO;
 		if (data > _NSIG)
-			goto out_tsk;
+			goto out_thaw;
 
 		child->ptrace &= ~PT_TRACESYS;
 		if (request == PTRACE_SINGLESTEP) {
@@ -962,12 +966,12 @@
 		/* give it a chance to run. */
 		wake_up_process(child);
 		ret = 0;
-		goto out_tsk;
+		goto out_thaw;
 
 	      case PTRACE_DETACH:		/* detach a process that was attached. */
 		ret = -EIO;
 		if (data > _NSIG)
-			goto out_tsk;
+			goto out_thaw;
 
 		child->ptrace &= ~(PT_PTRACED|PT_TRACESYS);
 		child->exit_code = data;
@@ -986,12 +990,14 @@
 
 		wake_up_process(child);
 		ret = 0;
-		goto out_tsk;
+		goto out_thaw;
 
 	      default:
 		ret = -EIO;
-		goto out_tsk;
+		goto out_thaw;
 	}
+  out_thaw:
+	ptrace_thaw_child(child, request, freeze_info);
   out_tsk:
 	free_task_struct(child);
   out:

Re: [patch] proposed fix for ptrace() SMP race

[Andrea Arcangeli]

On Thu, Sep 06, 2001 at 04:00:51PM -0700, David Mosberger wrote:
> There is currently a nasty race condition in ptrace().  The effect of

Last time I checked for such race it was looking ok (on x86). Do you
have a testcase to demonstrate it?

> this varies from one platform to another but, for example, on ia64, it
> could have the effect of corrupting the state of register f32-f127.
> The problem is that ptrace() uses the expression (child->state ==
> TASK_STOPPED) to determine whether or not a task has stopped
> execution.  On SMP, this is not sufficient because the task may still
> be executing while child->has_cpu is true.  This is easy to fix, but

but it cannot be running in "userspace" any longer once it is set to
TASK_STOPPED which should be the _only_ thing we care in ptrace. If it's
still running it's in its way to schedule() a few lines after the
setting of tsk->state to TASK_STOPPED and that's ok for ptrace.

> clearing child->cpus_allowed while ptrace() is running.  This should

abusing cpus_allowed to forbid scheduling is racy so quite unacceptable,
we want to preserve the cpus_allowed field for the administrator (he
could as well set it during the ptrace).

If on ia64 you really need to have switched the task away completly (not
only out of userspace) you could do this instead of messing with
cpus_allowed:

	if (not task_stopped)
		return
#ifdef CONFIG_SMP
	rmb(); /* read child->has_cpu after child->state */
	while (child->has_cpu);
	mb(); /* allowed to work on the task only when the task is been descheduled */
#endif

in ia64/kernel/ptrace.c

Actually one scary thing I can see in ptrace is the PTRACE_KILL case
that goes ahead doing the get_stack_long and put_stack_long even if the
task isn't out of userspace. I'd feel better with something like this:

--- 2.4.10pre4aa1/arch/i386/kernel/ptrace.c.~1~	Sat Jul 21 00:04:05 2001
+++ 2.4.10pre4aa1/arch/i386/kernel/ptrace.c	Fri Sep  7 02:14:45 2001
@@ -171,10 +171,8 @@
 	ret = -ESRCH;
 	if (!(child->ptrace & PT_PTRACED))
 		goto out_tsk;
-	if (child->state != TASK_STOPPED) {
-		if (request != PTRACE_KILL)
-			goto out_tsk;
-	}
+	if (child->state != TASK_STOPPED || child->state != TASK_ZOMBIE)
+		goto out_tsk;
 	if (child->p_pptr != current)
 		goto out_tsk;
 	switch (request) {


but OTOH I've no idea who is using PTRACE_KILL (but still it looks
saner or otherwise it means PTRACE_KILL implementation is at least
partly wrong anyways).

Andrea

Re: [patch] proposed fix for ptrace() SMP race

[David Mosberger]

>>>>> On Fri, 7 Sep 2001 02:19:00 +0200, Andrea Arcangeli <andrea@suse.de> said:

  Andrea> Last time I checked for such race it was looking ok (on
  Andrea> x86). Do you have a testcase to demonstrate it?

For ia64, yes.  I didn't look at x86.  Other platforms may or may not
be affected, that is true, but the races are so subtle, I'd rather
make sure this problem cannot occur on any platform.

  Andrea> but it cannot be running in "userspace" any longer once it
  Andrea> is set to TASK_STOPPED which should be the _only_ thing we
  Andrea> care in ptrace. If it's still running it's in its way to
  Andrea> schedule() a few lines after the setting of tsk->state to
  Andrea> TASK_STOPPED and that's ok for ptrace.

Yes, the code running in schedule() is sufficient.  On ia64, what
happened is that switch_to() may end up saving the floating-point
registers f32-f127.  After doing so, it will set a bit in the thread
flags and since this is done non-atomically (for good reason), it
creates a race with the code in ptrace() which also updates the thread
flags.

  Andrea> abusing cpus_allowed to forbid scheduling is racy so quite
  Andrea> unacceptable, we want to preserve the cpus_allowed field for
  Andrea> the administrator (he could as well set it during the
  Andrea> ptrace).

As long as the CPU manipulates cpus_allowed in an atomic fashion (xchg
or cmpxchg) this will be fine.  I'd argue it has to do this anyhow
(unless a task is changing its own cpus_allowed field).

If you don't like the cpus_allowed approach, please propose another
solution that ensures that the task does not get woken up while ptrace
is running.  I don't really care very much what the solution is, but I
do want to make sure we can guarantee that the task does not start
running while ptrace() is doing it's job.  Having to prove that race
conditions in ptrace() are harmless is far too painful (and bound to
be violated as code changes in the future...).

	--david

Re: [patch] proposed fix for ptrace() SMP race

[Andrea Arcangeli]

On Thu, Sep 06, 2001 at 05:40:54PM -0700, David Mosberger wrote:
>   Andrea> abusing cpus_allowed to forbid scheduling is racy so quite
>   Andrea> unacceptable, we want to preserve the cpus_allowed field for
>   Andrea> the administrator (he could as well set it during the
>   Andrea> ptrace).
> 
> As long as the CPU manipulates cpus_allowed in an atomic fashion (xchg
> or cmpxchg) this will be fine.  I'd argue it has to do this anyhow
> (unless a task is changing its own cpus_allowed field).

atomic updates of cpus_allowed it's not the point I was making, it's
still racy:

	ptrace				admin via /proc
	--------------			---------------
	save and clear
					set cpus_allowed to something
	restore cpus_allowed <destroy modification>

the modification of the user is been destroyed if he sets cpus_allowed
inside ptrace, this is the race condition I was thinking about.
	
> If you don't like the cpus_allowed approach, please propose another
> solution that ensures that the task does not get woken up while ptrace

For making sure the task isn't wakenup while it's under ptrace we should
just do that in kernel/signal.c::ignored_signal() as far I can tell.

To ensure the task just sleeps I suggest the one I mentioned in the
previous email. here a patch (possibly breaks PTRACE_KILL, I didn't
backed out the PTRACE_KILL change yet):

--- 2.4.10pre4aa1/arch/i386/kernel/ptrace.c.~1~	Sat Jul 21 00:04:05 2001
+++ 2.4.10pre4aa1/arch/i386/kernel/ptrace.c	Fri Sep  7 03:19:53 2001
@@ -171,12 +171,15 @@
 	ret = -ESRCH;
 	if (!(child->ptrace & PT_PTRACED))
 		goto out_tsk;
-	if (child->state != TASK_STOPPED) {
-		if (request != PTRACE_KILL)
-			goto out_tsk;
-	}
+	if (child->state != TASK_STOPPED && child->state != TASK_ZOMBIE)
+		goto out_tsk;
 	if (child->p_pptr != current)
 		goto out_tsk;
+#ifdef CONFIG_SMP
+	rmb(); /* read child->has_cpu after child->state */
+	while (child->has_cpu);
+	mb(); /* allowed to work on the task only when the task is been descheduled */
+#endif
 	switch (request) {
 	/* when I and D space are separate, these will need to be fixed. */
 	case PTRACE_PEEKTEXT: /* read word at location addr. */ 


Andrea

Re: [patch] proposed fix for ptrace() SMP race

[Alan Cox]

> +#ifdef CONFIG_SMP
> +	rmb(); /* read child->has_cpu after child->state */
> +	while (child->has_cpu);
		rep_nop();

otherwise your PIV will overheat

Re: [patch] proposed fix for ptrace() SMP race

[Andrea Arcangeli]

On Fri, Sep 07, 2001 at 02:41:11AM +0100, Alan Cox wrote:
> > +#ifdef CONFIG_SMP
> > +	rmb(); /* read child->has_cpu after child->state */
> > +	while (child->has_cpu);
> 		rep_nop();
> 
> otherwise your PIV will overheat

indeed correct (OTOH it's a so small window [not like a contended
spinlock] that I wonder if it will make a difference in real life ;)

Andrea

Re: [patch] proposed fix for ptrace() SMP race

[David Mosberger]

>>>>> On Fri, 7 Sep 2001 03:28:01 +0200, Andrea Arcangeli <andrea@suse.de> said:

  Andrea> For making sure the task isn't wakenup while it's under
  Andrea> ptrace we should just do that in
  Andrea> kernel/signal.c::ignored_signal() as far I can tell.

This doesn't make sense: ignored_signal() is too late as
handle_stop_signal() will already have woken up the task in response
to a SIGCONT.  Also, if you're suggesting to ignore SIGCONT while a
PT_PTRACED is set, that certainly wouldn't be right.  We only want to
*delay* the wakeup while the ptrace() system call is running (which is
much shorter than the period of time PT_PTRACED is set).  So, as far
as I can tell, you'd have to add more locking to the signal path,
which doesn't look attractive to me. Also, if pursuing approach, we'd
have to prove that we cover all possible paths that could wake up the
task.  E.g., PTRACE_SYSCALL and PTRACE_CONT are other ways the task
could be woken up.  These particular cases should be fine, because
they'll already be serialized by the BKL acquired during ptrace(), but
I'm not so sure there aren't any other cases.

So, I still think cpus_allowed is a safer and better approach at least
for 2.4.  Yes, we'd have to add locking for writing cpus_allowed, but
I'd say that makes sense anyhow given that it is being manipulated by
other tasks.

Hmmh, looking at ptrace() more closely, the entire locking situation
seems to be a bit confused.  For example, what's stopping wait4() from
releasing the task structure just after ptrace() released the
tasklist_lock and before it checked child->state?

	--david

Re: [patch] proposed fix for ptrace() SMP race

[Andrea Arcangeli]

On Thu, Sep 06, 2001 at 10:21:14PM -0700, David Mosberger wrote:
> >>>>> On Fri, 7 Sep 2001 03:28:01 +0200, Andrea Arcangeli <andrea@suse.de> said:
> 
>   Andrea> For making sure the task isn't wakenup while it's under
>   Andrea> ptrace we should just do that in
>   Andrea> kernel/signal.c::ignored_signal() as far I can tell.
> 
> This doesn't make sense: ignored_signal() is too late as
> handle_stop_signal() will already have woken up the task in response
> to a SIGCONT.  Also, if you're suggesting to ignore SIGCONT while a
> PT_PTRACED is set, that certainly wouldn't be right.  We only want to

correct, I suggest to ignore SIGCONT as well while PT_PTRACED is set.

> *delay* the wakeup while the ptrace() system call is running (which is
> much shorter than the period of time PT_PTRACED is set).  So, as far
> as I can tell, you'd have to add more locking to the signal path,

Not more locking, just an additional check:

--- 2.4.10pre4aa1/kernel/signal.c.~1~	Sun Sep  2 20:04:01 2001
+++ 2.4.10pre4aa1/kernel/signal.c	Fri Sep  7 15:22:23 2001
@@ -382,7 +382,7 @@
 	switch (sig) {
 	case SIGKILL: case SIGCONT:
 		/* Wake up the process if stopped.  */
-		if (t->state == TASK_STOPPED)
+		if (t->state == TASK_STOPPED && !(t->ptrace & PT_PTRACED))
 			wake_up_process(t);
 		t->exit_code = 0;
 		rm_sig_from_queue(SIGSTOP, t);

> So, I still think cpus_allowed is a safer and better approach at least

forget that.  Also when you restore the cpus_allowed you won't
effectively wakup the task, it will just keep floating in the runqueue
but we won't try to reschedule the other idle cpus it so it's broken.

> Hmmh, looking at ptrace() more closely, the entire locking situation
> seems to be a bit confused.  For example, what's stopping wait4() from
> releasing the task structure just after ptrace() released the
> tasklist_lock and before it checked child->state?

the get_task_struct()

Andrea

Re: [patch] proposed fix for ptrace() SMP race

[David Mosberger]

>>>>> On Fri, 7 Sep 2001 15:28:58 +0200, Andrea Arcangeli <andrea@suse.de> said:

  Andrea> correct, I suggest to ignore SIGCONT as well while
  Andrea> PT_PTRACED is set.

Do you really think it's acceptable?  With your patch, you couldn't
SIGKILL or SIGCONT a task that happens to be ptraced.  I certainly
would expect to be able to do this for a task that is being strace'd,
for example.

Also, other signals will still wake up the task.  Yes, it won't get
very far as do_signal() will notify the parent instead, but still, the
task will run and that could be enough to create some race condition.

What about the other wakeup paths that I had mentioned?  E.g., what if
the ptraced task is ptracing another task?  Couldn't notify_parent()
end up waking up the task as well?

  Andrea> Also when you restore the cpus_allowed you won't effectively
  Andrea> wakup the task, it will just keep floating in the runqueue
  Andrea> but we won't try to reschedule the other idle cpus it so
  Andrea> it's broken.

Ah, that's a good point, thanks.  Nothing that can't be fixed, though.

  >> Hmmh, looking at ptrace() more closely, the entire locking
  >> situation seems to be a bit confused.  For example, what's
  >> stopping wait4() from releasing the task structure just after
  >> ptrace() released the tasklist_lock and before it checked
  >> child->state?

  Andrea> the get_task_struct()

Yes, I missed that.

	--david

Re: [patch] proposed fix for ptrace() SMP race

[Andrea Arcangeli]

On Fri, Sep 07, 2001 at 08:35:31AM -0700, David Mosberger wrote:
> Also, other signals will still wake up the task.  Yes, it won't get
> very far as do_signal() will notify the parent instead, but still, the
> task will run and that could be enough to create some race condition.

this is the real issue, agreed.

However still I don't like the cpus_allowed racy approch. I either
prefer to force the deschedule with a new ptrace bitflag with new hooks
in the scheduler or with a blocker (delayer) to the signals again with a
new ptrace bitflag but in this case with hooks in the signal code. I
think putting the hooks in the signal code is better.

BTW, checking this stuff I found two bugs, one is the check for
cpus_allowed before calling reschedule_idle, such check has to be
removed, then it also seems the signals seems to wakeup the task two
times unless I've overlooked something.

You may want to make a new patch at the light of those considerations
otherwise I'll put this in my todo list once more important things are
solved.

Andrea

Re: [patch] proposed fix for ptrace() SMP race

[David Mosberger]

>>>>> On Sat, 8 Sep 2001 19:11:08 +0200, Andrea Arcangeli <andrea@suse.de> said:

  Andrea> On Fri, Sep 07, 2001 at 08:35:31AM -0700, David Mosberger
  Andrea> wrote:
  >> Also, other signals will still wake up the task.  Yes, it won't
  >> get very far as do_signal() will notify the parent instead, but
  >> still, the task will run and that could be enough to create some
  >> race condition.

  Andrea> this is the real issue, agreed.

Good.

  Andrea> However still I don't like the cpus_allowed racy approch. I
  Andrea> either prefer to force the deschedule with a new ptrace
  Andrea> bitflag with new hooks in the scheduler or with a blocker
  Andrea> (delayer) to the signals again with a new ptrace bitflag but
  Andrea> in this case with hooks in the signal code. I think putting
  Andrea> the hooks in the signal code is better.

Yes, though I don't really see how you could do this without any
change to the scheduler.

  Andrea> BTW, checking this stuff I found two bugs, one is the check
  Andrea> for cpus_allowed before calling reschedule_idle, such check
  Andrea> has to be removed, then it also seems the signals seems to
  Andrea> wakeup the task two times unless I've overlooked something.

  Andrea> You may want to make a new patch at the light of those
  Andrea> considerations otherwise I'll put this in my todo list once
  Andrea> more important things are solved.

Why don't you keep it on your todo list.  I too have a couple of other
things I need to finish first so it will be a while before I'd get to
this (not before November, I'd guess).  But I'll keep it in mind as
well.

Thanks,

	--david

Re: [patch] proposed fix for ptrace() SMP race

[Manfred Spraul]

>
> BTW, checking this stuff I found two bugs, one is the check for
> cpus_allowed before calling reschedule_idle,

The call in linux/kernel.c, around line 348?
348 if (!synchronous || !(p->cpus_allowed & (1 << smp_processor_id())))
349       reschedule_idle(p);

The test was added for wake_up_{,interruptible_}sync(): if the woken up
task is not permitted to run on the current cpu, reschedule() is
necessary, otherwise skip the reschedule.
If ptrace sets cpus_allowed to 0 between wake_up_sync() and schedule(),
the reschedule is lost. But always rescheduling would defeat the purpose
of wake_up_sync().

--
	Manfred