[PATCH 0/5] hw_breakpoints: Fix racy ptrace breakpoint acccesses

[PATCH 0/5] hw_breakpoints: Fix racy ptrace breakpoint acccesses

[Frederic Weisbecker]

I should have fixed that a few month ago but got sidetracked.
Please have a look at this. x86 and powerpc already had support
for breakpoints before so it fixes a regression there (cf stable tag)
For the others it only a fix.

Other archs than x86 have been only compile tested.

Thanks.

Frederic Weisbecker (5):
  ptrace: Prepare to fix racy accesses on task breakpoints
  x86, hw_breakpoints: Fix racy access to ptrace breakpoints
  powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints
  arm, hw_breakpoints: Fix racy access to ptrace breakpoints
  sh, hw_breakpoints: Fix racy access to ptrace breakpoints

 arch/arm/kernel/ptrace.c     |    8 ++++++++
 arch/powerpc/kernel/ptrace.c |    3 +++
 arch/sh/kernel/ptrace_32.c   |    4 ++++
 arch/x86/kernel/ptrace.c     |   36 ++++++++++++++++++++++++++----------
 include/linux/ptrace.h       |   13 ++++++++++++-
 include/linux/sched.h        |    3 +++
 kernel/exit.c                |    2 +-
 kernel/ptrace.c              |   17 +++++++++++++++++
 8 files changed, 74 insertions(+), 12 deletions(-)

-- 
1.7.3.2

[PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Frederic Weisbecker]

When a task is traced and is in a stopped state, the tracer
may execute a ptrace request to examine the tracee state and
get its task struct. Right after, the tracee can be killed
and thus its breakpoints released.
This can happen concurrently when the tracer is in the middle
of reading or modifying these breakpoints, leading to dereferencing
a freed pointer.

Hence, to prepare the fix, create a generic breakpoint reference
holding API. When a reference on the breakpoints of a task is
held, the breakpoints won't be released until the last reference
is dropped. After that, no more ptrace request on the task's
breakpoints can be serviced for the tracer.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: v2.6.33.. <stable@kernel.org>
---
 include/linux/ptrace.h |   13 ++++++++++++-
 include/linux/sched.h  |    3 +++
 kernel/exit.c          |    2 +-
 kernel/ptrace.c        |   17 +++++++++++++++++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index a1147e5..9178d5c 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -189,6 +189,10 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
 		child->ptrace = current->ptrace;
 		__ptrace_link(child, current->parent);
 	}
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	atomic_set(&child->ptrace_bp_refcnt, 1);
+#endif
 }
 
 /**
@@ -350,6 +354,13 @@ extern int task_current_syscall(struct task_struct *target, long *callno,
 				unsigned long args[6], unsigned int maxargs,
 				unsigned long *sp, unsigned long *pc);
 
-#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+extern int ptrace_get_breakpoints(struct task_struct *tsk);
+extern void ptrace_put_breakpoints(struct task_struct *tsk);
+#else
+static inline void ptrace_put_breakpoints(struct task_struct *tsk) { }
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
+#endif /* __KERNEL */
 
 #endif
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 83bd2e2..15badfa 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1534,6 +1534,9 @@ struct task_struct {
 		unsigned long memsw_nr_pages; /* uncharged mem+swap usage */
 	} memcg_batch;
 #endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	atomic_t ptrace_bp_refcnt;
+#endif
 };
 
 /* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/exit.c b/kernel/exit.c
index 6a488ad..437e327 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1016,7 +1016,7 @@ NORET_TYPE void do_exit(long code)
 	/*
 	 * FIXME: do that only when needed, using sched_exit tracepoint
 	 */
-	flush_ptrace_hw_breakpoint(tsk);
+	ptrace_put_breakpoints(tsk);
 
 	exit_notify(tsk, group_dead);
 #ifdef CONFIG_NUMA
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 0fc1eed..dc7ab65 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
 #include <linux/regset.h>
+#include <linux/hw_breakpoint.h>
 
 
 /*
@@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
 	return ret;
 }
 #endif	/* CONFIG_COMPAT */
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+int ptrace_get_breakpoints(struct task_struct *tsk)
+{
+	if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
+		return 0;
+
+	return -1;
+}
+
+void ptrace_put_breakpoints(struct task_struct *tsk)
+{
+	if (atomic_dec_and_test(&tsk->ptrace_bp_refcnt))
+		flush_ptrace_hw_breakpoint(tsk);
+}
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
-- 
1.7.3.2

[PATCH 2/5] x86, hw_breakpoints: Fix racy access to ptrace breakpoints

[Frederic Weisbecker]

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: v2.6.33.. <stable@kernel.org>
---
 arch/x86/kernel/ptrace.c |   36 ++++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 45892dc..f65e5b5 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -608,6 +608,9 @@ static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)
 	unsigned len, type;
 	struct perf_event *bp;
 
+	if (ptrace_get_breakpoints(tsk) < 0)
+		return -ESRCH;
+
 	data &= ~DR_CONTROL_RESERVED;
 	old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
 restore:
@@ -655,6 +658,9 @@ restore:
 		}
 		goto restore;
 	}
+
+	ptrace_put_breakpoints(tsk);
+
 	return ((orig_ret < 0) ? orig_ret : rc);
 }
 
@@ -668,10 +674,17 @@ static unsigned long ptrace_get_debugreg(struct task_struct *tsk, int n)
 
 	if (n < HBP_NUM) {
 		struct perf_event *bp;
+
+		if (ptrace_get_breakpoints(tsk) < 0)
+			return -ESRCH;
+
 		bp = thread->ptrace_bps[n];
 		if (!bp)
-			return 0;
-		val = bp->hw.info.address;
+			val = 0;
+		else
+			val = bp->hw.info.address;
+
+		ptrace_put_breakpoints(tsk);
 	} else if (n == 6) {
 		val = thread->debugreg6;
 	 } else if (n == 7) {
@@ -686,6 +699,10 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 	struct perf_event *bp;
 	struct thread_struct *t = &tsk->thread;
 	struct perf_event_attr attr;
+	int err = 0;
+
+	if (ptrace_get_breakpoints(tsk) < 0)
+		return -ESRCH;
 
 	if (!t->ptrace_bps[nr]) {
 		ptrace_breakpoint_init(&attr);
@@ -709,24 +726,23 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 		 * writing for the user. And anyway this is the previous
 		 * behaviour.
 		 */
-		if (IS_ERR(bp))
-			return PTR_ERR(bp);
+		if (IS_ERR(bp)) {
+			err = PTR_ERR(bp);
+			goto put;
+		}
 
 		t->ptrace_bps[nr] = bp;
 	} else {
-		int err;
-
 		bp = t->ptrace_bps[nr];
 
 		attr = bp->attr;
 		attr.bp_addr = addr;
 		err = modify_user_hw_breakpoint(bp, &attr);
-		if (err)
-			return err;
 	}
 
-
-	return 0;
+put:
+	ptrace_put_breakpoints(tsk);
+	return err;
 }
 
 /*
-- 
1.7.3.2

[PATCH 3/5] powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints

[Frederic Weisbecker]

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: v2.6.33.. <stable@kernel.org>
---
 arch/powerpc/kernel/ptrace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 895b082..330df91 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1591,7 +1591,10 @@ long arch_ptrace(struct task_struct *child, long request,
 	}
 
 	case PTRACE_SET_DEBUGREG:
+		if (ptrace_get_breakpoints(child) < 0)
+			return -ESRCH;
 		ret = ptrace_set_debugreg(child, addr, data);
+		ptrace_put_breakpoints(child);
 		break;
 
 #ifdef CONFIG_PPC64
-- 
1.7.3.2

[PATCH 4/5] arm, hw_breakpoints: Fix racy access to ptrace breakpoints

[Frederic Weisbecker]

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 arch/arm/kernel/ptrace.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 2bf27f3..8182f45 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -767,12 +767,20 @@ long arch_ptrace(struct task_struct *child, long request,
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 		case PTRACE_GETHBPREGS:
+			if (ptrace_get_breakpoints(child) < 0)
+				return -ESRCH;
+
 			ret = ptrace_gethbpregs(child, addr,
 						(unsigned long __user *)data);
+			ptrace_put_breakpoints(child);
 			break;
 		case PTRACE_SETHBPREGS:
+			if (ptrace_get_breakpoints(child) < 0)
+				return -ESRCH;
+
 			ret = ptrace_sethbpregs(child, addr,
 						(unsigned long __user *)data);
+			ptrace_put_breakpoints(child);
 			break;
 #endif
 
-- 
1.7.3.2

[PATCH 5/5] sh, hw_breakpoints: Fix racy access to ptrace breakpoints

[Frederic Weisbecker]

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 arch/sh/kernel/ptrace_32.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 2130ca6..3d7b209 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -117,7 +117,11 @@ void user_enable_single_step(struct task_struct *child)
 
 	set_tsk_thread_flag(child, TIF_SINGLESTEP);
 
+	if (ptrace_get_breakpoints(child) < 0)
+		return;
+
 	set_single_step(child, pc);
+	ptrace_put_breakpoints(child);
 }
 
 void user_disable_single_step(struct task_struct *child)
-- 
1.7.3.2

Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Will Deacon]

Hi Frederic,

On Fri, 2011-04-08 at 18:34 +0100, Frederic Weisbecker wrote:
> When a task is traced and is in a stopped state, the tracer
> may execute a ptrace request to examine the tracee state and
> get its task struct. Right after, the tracee can be killed
> and thus its breakpoints released.
> This can happen concurrently when the tracer is in the middle
> of reading or modifying these breakpoints, leading to dereferencing
> a freed pointer.

Oo, that's nasty. Would an alternative solution be to free the
breakpoints only when the task_struct usage count is zero?

> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 0fc1eed..dc7ab65 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -22,6 +22,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/uaccess.h>
>  #include <linux/regset.h>
> +#include <linux/hw_breakpoint.h>
> 
> 
>  /*
> @@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
>         return ret;
>  }
>  #endif /* CONFIG_COMPAT */
> +
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +int ptrace_get_breakpoints(struct task_struct *tsk)
> +{
> +       if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
> +               return 0;
> +
> +       return -1;
> +}


Would it be better to return -ESRCH here instead?

Will

Re: [PATCH 5/5] sh, hw_breakpoints: Fix racy access to ptrace breakpoints

[Paul Mundt]

On Fri, Apr 08, 2011 at 07:34:27PM +0200, Frederic Weisbecker wrote:
> While the tracer accesses ptrace breakpoints, the child task may
> concurrently exit due to a SIGKILL and thus release its breakpoints
> at the same time. We can then dereference some freed pointers.
> 
> To fix this, hold a reference on the child breakpoints before
> manipulating them.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Prasad <prasad@linux.vnet.ibm.com>
> Cc: Paul Mundt <lethal@linux-sh.org>

Acked-by: Paul Mundt <lethal@linux-sh.org>

Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Frederic Weisbecker]

On Mon, Apr 11, 2011 at 11:47:57AM +0100, Will Deacon wrote:
> Hi Frederic,
> 
> On Fri, 2011-04-08 at 18:34 +0100, Frederic Weisbecker wrote:
> > When a task is traced and is in a stopped state, the tracer
> > may execute a ptrace request to examine the tracee state and
> > get its task struct. Right after, the tracee can be killed
> > and thus its breakpoints released.
> > This can happen concurrently when the tracer is in the middle
> > of reading or modifying these breakpoints, leading to dereferencing
> > a freed pointer.
> 
> Oo, that's nasty. Would an alternative solution be to free the
> breakpoints only when the task_struct usage count is zero?

Yeah my solution may look a bit gross. But the problem is
that perf events hold a ref on their task context. Thus the
task_struct usage will never be 0 until you release all the
perf events attached to it.

Normal perf events are released in two ways in the exit path:

- explicitly if they are inherited
- from the file release path if they are a parent

Now breakpoints are a bit specific because neither are they inherited,
nor do they have a file associated.

So we need to release them explicitly to release the task. And after that
we also need to ensure nobody will play with the breakpoints, otherwise there
will be a memory leak because those will never be freed.

So that patch protects against concurrent release of the breakpoints and
also against the possible memory leak.

May be we can think about a solution that involves not taking a ref
to the task when we allocate breakpoints, and then finally release
from the task_struct rcu release. But that may involve many corner
cases. Perhaps we can think about this later and for now opt for the
current solution that looks safe and without surprise. This fix needs
to be backported so it should stay KISS I think.

 
> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index 0fc1eed..dc7ab65 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/syscalls.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/regset.h>
> > +#include <linux/hw_breakpoint.h>
> > 
> > 
> >  /*
> > @@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
> >         return ret;
> >  }
> >  #endif /* CONFIG_COMPAT */
> > +
> > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > +int ptrace_get_breakpoints(struct task_struct *tsk)
> > +{
> > +       if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
> > +               return 0;
> > +
> > +       return -1;
> > +}
> 
> 
> Would it be better to return -ESRCH here instead?

So I'm going to be nitpicking there :)
The ptrace_get_breakpoints() function tells us whether
we can take a ref on the breakpoints or not.

Returning -ERSCH there would mean that the task struct doesn't exist,
or something confusing like this. Which is not true: the task exists.

OTOH, the caller, which is ptrace, needs to take a decision when he
can't take a reference to the breakpoints. The behaviour is
to act as if the process does not exist anymore, which is about to
happen for real but we anticipate because the task has reached a
state in its exiting path where we can't manipulate the breakpoints
anymore.

So the rationale behind it is that -ERSCH is an interpretation
of the caller.

Right?

Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Will Deacon]

Hi Frederic,

On Tue, 2011-04-12 at 18:54 +0100, Frederic Weisbecker wrote:
> > On Fri, 2011-04-08 at 18:34 +0100, Frederic Weisbecker wrote:
> > > When a task is traced and is in a stopped state, the tracer
> > > may execute a ptrace request to examine the tracee state and
> > > get its task struct. Right after, the tracee can be killed
> > > and thus its breakpoints released.
> > > This can happen concurrently when the tracer is in the middle
> > > of reading or modifying these breakpoints, leading to dereferencing
> > > a freed pointer.
> >
> > Oo, that's nasty. Would an alternative solution be to free the
> > breakpoints only when the task_struct usage count is zero?
> 
> Yeah my solution may look a bit gross. But the problem is
> that perf events hold a ref on their task context. Thus the
> task_struct usage will never be 0 until you release all the
> perf events attached to it.

Blimey, that explains the complications!

> Normal perf events are released in two ways in the exit path:
> 
> - explicitly if they are inherited
> - from the file release path if they are a parent
> 
> Now breakpoints are a bit specific because neither are they inherited,
> nor do they have a file associated.
> 
> So we need to release them explicitly to release the task. And after that
> we also need to ensure nobody will play with the breakpoints, otherwise there
> will be a memory leak because those will never be freed.
> 
> So that patch protects against concurrent release of the breakpoints and
> also against the possible memory leak.

Agreed.

> May be we can think about a solution that involves not taking a ref
> to the task when we allocate breakpoints, and then finally release
> from the task_struct rcu release. But that may involve many corner
> cases. Perhaps we can think about this later and for now opt for the
> current solution that looks safe and without surprise. This fix needs
> to be backported so it should stay KISS I think.

Avoiding taking the ref still means handling breakpoints specially so I
don't think you win much. I was just intrigued by your original patch.

> > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > > index 0fc1eed..dc7ab65 100644
> > > --- a/kernel/ptrace.c
> > > +++ b/kernel/ptrace.c
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/syscalls.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/regset.h>
> > > +#include <linux/hw_breakpoint.h>
> > >
> > >
> > >  /*
> > > @@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
> > >         return ret;
> > >  }
> > >  #endif /* CONFIG_COMPAT */
> > > +
> > > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > > +int ptrace_get_breakpoints(struct task_struct *tsk)
> > > +{
> > > +       if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
> > > +               return 0;
> > > +
> > > +       return -1;
> > > +}
> >
> >
> > Would it be better to return -ESRCH here instead?
> 
> So I'm going to be nitpicking there :)
> The ptrace_get_breakpoints() function tells us whether
> we can take a ref on the breakpoints or not.
> 
> Returning -ERSCH there would mean that the task struct doesn't exist,
> or something confusing like this. Which is not true: the task exists.

Sure, we need a way of saying `you can't take a reference to the
breakpoints for this task' without specifying why. So I guess -ESRCH is
wrong but I don't know that -1 is correct either (then again, I'm not
*too* bothered by it :).

> OTOH, the caller, which is ptrace, needs to take a decision when he
> can't take a reference to the breakpoints. The behaviour is
> to act as if the process does not exist anymore, which is about to
> happen for real but we anticipate because the task has reached a
> state in its exiting path where we can't manipulate the breakpoints
> anymore.
> 
> So the rationale behind it is that -ERSCH is an interpretation
> of the caller.
> 
> Right?

Yup.

For this and the ARM patch:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Frederic Weisbecker]

On Wed, Apr 13, 2011 at 03:34:18PM +0100, Will Deacon wrote:
> > Returning -ERSCH there would mean that the task struct doesn't exist,
> > or something confusing like this. Which is not true: the task exists.
> 
> Sure, we need a way of saying `you can't take a reference to the
> breakpoints for this task' without specifying why. So I guess -ESRCH is
> wrong but I don't know that -1 is correct either (then again, I'm not
> *too* bothered by it :).

-EBUSY perhaps? Well I took -1 by default...

> 
> > OTOH, the caller, which is ptrace, needs to take a decision when he
> > can't take a reference to the breakpoints. The behaviour is
> > to act as if the process does not exist anymore, which is about to
> > happen for real but we anticipate because the task has reached a
> > state in its exiting path where we can't manipulate the breakpoints
> > anymore.
> > 
> > So the rationale behind it is that -ERSCH is an interpretation
> > of the caller.
> > 
> > Right?
> 
> Yup.
> 
> For this and the ARM patch:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>

Great! Thanks!

[PATCH 3/5] powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints

[Frederic Weisbecker]

(resend with ppc list in cc)

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: v2.6.33.. <stable@kernel.org>
---
 arch/powerpc/kernel/ptrace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 55613e3..4edeeb3 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1591,7 +1591,10 @@ long arch_ptrace(struct task_struct *child, long request,
 	}
 
 	case PTRACE_SET_DEBUGREG:
+		if (ptrace_get_breakpoints(child) < 0)
+			return -ESRCH;
 		ret = ptrace_set_debugreg(child, addr, data);
+		ptrace_put_breakpoints(child);
 		break;
 
 #ifdef CONFIG_PPC64
-- 
1.7.3.2

Re: [PATCH 3/5] powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints

[K.Prasad]

On Fri, Apr 22, 2011 at 03:16:27PM +0200, Frederic Weisbecker wrote:
> (resend with ppc list in cc)
> 
> While the tracer accesses ptrace breakpoints, the child task may
> concurrently exit due to a SIGKILL and thus release its breakpoints
> at the same time. We can then dereference some freed pointers.
> 
> To fix this, hold a reference on the child breakpoints before
> manipulating them.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Prasad <prasad@linux.vnet.ibm.com>
> Cc: Paul Mundt <lethal@linux-sh.org>
> Cc: v2.6.33.. <stable@kernel.org>
> ---
>  arch/powerpc/kernel/ptrace.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 55613e3..4edeeb3 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -1591,7 +1591,10 @@ long arch_ptrace(struct task_struct *child, long request,
>  	}
> 
>  	case PTRACE_SET_DEBUGREG:
> +		if (ptrace_get_breakpoints(child) < 0)
> +			return -ESRCH;
>  		ret = ptrace_set_debugreg(child, addr, data);
> +		ptrace_put_breakpoints(child);
>  		break;
> 
>  #ifdef CONFIG_PPC64
> -- 
> 1.7.3.2
>

Hi Frederic,
	Looks fine to me.

Acked-by: K.Prasad <prasad@linux.vnet.ibm.com>

Thanks,
K.Prasad 

Re: [PATCH 0/5] hw_breakpoints: Fix racy ptrace breakpoint acccesses

[Frederic Weisbecker]

On Fri, Apr 08, 2011 at 07:34:22PM +0200, Frederic Weisbecker wrote:
> I should have fixed that a few month ago but got sidetracked.
> Please have a look at this. x86 and powerpc already had support
> for breakpoints before so it fixes a regression there (cf stable tag)
> For the others it only a fix.
> 
> Other archs than x86 have been only compile tested.
> 
> Thanks.
> 
> Frederic Weisbecker (5):
>   ptrace: Prepare to fix racy accesses on task breakpoints
>   x86, hw_breakpoints: Fix racy access to ptrace breakpoints
>   powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints
>   arm, hw_breakpoints: Fix racy access to ptrace breakpoints
>   sh, hw_breakpoints: Fix racy access to ptrace breakpoints

Thanks a lot guys for your acks. I'm reposting with these
and pushing for Ingo.

> 
>  arch/arm/kernel/ptrace.c     |    8 ++++++++
>  arch/powerpc/kernel/ptrace.c |    3 +++
>  arch/sh/kernel/ptrace_32.c   |    4 ++++
>  arch/x86/kernel/ptrace.c     |   36 ++++++++++++++++++++++++++----------
>  include/linux/ptrace.h       |   13 ++++++++++++-
>  include/linux/sched.h        |    3 +++
>  kernel/exit.c                |    2 +-
>  kernel/ptrace.c              |   17 +++++++++++++++++
>  8 files changed, 74 insertions(+), 12 deletions(-)
> 
> -- 
> 1.7.3.2
> 

Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Frederic Weisbecker]

Hi Oleg.

I realize you weren't in the Cc list, which wasn't definitly not
intended.

I think you were fine with the change. But to be sure, can I have your ack?

Thanks.

On Fri, Apr 08, 2011 at 07:34:23PM +0200, Frederic Weisbecker wrote:
> When a task is traced and is in a stopped state, the tracer
> may execute a ptrace request to examine the tracee state and
> get its task struct. Right after, the tracee can be killed
> and thus its breakpoints released.
> This can happen concurrently when the tracer is in the middle
> of reading or modifying these breakpoints, leading to dereferencing
> a freed pointer.
> 
> Hence, to prepare the fix, create a generic breakpoint reference
> holding API. When a reference on the breakpoints of a task is
> held, the breakpoints won't be released until the last reference
> is dropped. After that, no more ptrace request on the task's
> breakpoints can be serviced for the tracer.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Prasad <prasad@linux.vnet.ibm.com>
> Cc: Paul Mundt <lethal@linux-sh.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: v2.6.33.. <stable@kernel.org>
> ---
>  include/linux/ptrace.h |   13 ++++++++++++-
>  include/linux/sched.h  |    3 +++
>  kernel/exit.c          |    2 +-
>  kernel/ptrace.c        |   17 +++++++++++++++++
>  4 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index a1147e5..9178d5c 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -189,6 +189,10 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
>  		child->ptrace = current->ptrace;
>  		__ptrace_link(child, current->parent);
>  	}
> +
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	atomic_set(&child->ptrace_bp_refcnt, 1);
> +#endif
>  }
>  
>  /**
> @@ -350,6 +354,13 @@ extern int task_current_syscall(struct task_struct *target, long *callno,
>  				unsigned long args[6], unsigned int maxargs,
>  				unsigned long *sp, unsigned long *pc);
>  
> -#endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +extern int ptrace_get_breakpoints(struct task_struct *tsk);
> +extern void ptrace_put_breakpoints(struct task_struct *tsk);
> +#else
> +static inline void ptrace_put_breakpoints(struct task_struct *tsk) { }
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> +
> +#endif /* __KERNEL */
>  
>  #endif
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 83bd2e2..15badfa 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1534,6 +1534,9 @@ struct task_struct {
>  		unsigned long memsw_nr_pages; /* uncharged mem+swap usage */
>  	} memcg_batch;
>  #endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	atomic_t ptrace_bp_refcnt;
> +#endif
>  };
>  
>  /* Future-safe accessor for struct task_struct's cpus_allowed. */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 6a488ad..437e327 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1016,7 +1016,7 @@ NORET_TYPE void do_exit(long code)
>  	/*
>  	 * FIXME: do that only when needed, using sched_exit tracepoint
>  	 */
> -	flush_ptrace_hw_breakpoint(tsk);
> +	ptrace_put_breakpoints(tsk);
>  
>  	exit_notify(tsk, group_dead);
>  #ifdef CONFIG_NUMA
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 0fc1eed..dc7ab65 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -22,6 +22,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/uaccess.h>
>  #include <linux/regset.h>
> +#include <linux/hw_breakpoint.h>
>  
>  
>  /*
> @@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
>  	return ret;
>  }
>  #endif	/* CONFIG_COMPAT */
> +
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +int ptrace_get_breakpoints(struct task_struct *tsk)
> +{
> +	if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
> +		return 0;
> +
> +	return -1;
> +}
> +
> +void ptrace_put_breakpoints(struct task_struct *tsk)
> +{
> +	if (atomic_dec_and_test(&tsk->ptrace_bp_refcnt))
> +		flush_ptrace_hw_breakpoint(tsk);
> +}
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> -- 
> 1.7.3.2
> 

[tip:perf/urgent] ptrace: Prepare to fix racy accesses on task breakpoints

[tip-bot for Frederic Weisbecker]

Commit-ID:  bf26c018490c2fce7fe9b629083b96ce0e6ad019
Gitweb:     http://git.kernel.org/tip/bf26c018490c2fce7fe9b629083b96ce0e6ad019
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Thu, 7 Apr 2011 16:53:20 +0200
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Mon, 25 Apr 2011 17:28:24 +0200

ptrace: Prepare to fix racy accesses on task breakpoints

When a task is traced and is in a stopped state, the tracer
may execute a ptrace request to examine the tracee state and
get its task struct. Right after, the tracee can be killed
and thus its breakpoints released.
This can happen concurrently when the tracer is in the middle
of reading or modifying these breakpoints, leading to dereferencing
a freed pointer.

Hence, to prepare the fix, create a generic breakpoint reference
holding API. When a reference on the breakpoints of a task is
held, the breakpoints won't be released until the last reference
is dropped. After that, no more ptrace request on the task's
breakpoints can be serviced for the tracer.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: v2.6.33.. <stable@kernel.org>
Link: http://lkml.kernel.org/r/1302284067-7860-2-git-send-email-fweisbec@gmail.com
---
 include/linux/ptrace.h |   13 ++++++++++++-
 include/linux/sched.h  |    3 +++
 kernel/exit.c          |    2 +-
 kernel/ptrace.c        |   17 +++++++++++++++++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index a1147e5..9178d5c 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -189,6 +189,10 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
 		child->ptrace = current->ptrace;
 		__ptrace_link(child, current->parent);
 	}
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	atomic_set(&child->ptrace_bp_refcnt, 1);
+#endif
 }
 
 /**
@@ -350,6 +354,13 @@ extern int task_current_syscall(struct task_struct *target, long *callno,
 				unsigned long args[6], unsigned int maxargs,
 				unsigned long *sp, unsigned long *pc);
 
-#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+extern int ptrace_get_breakpoints(struct task_struct *tsk);
+extern void ptrace_put_breakpoints(struct task_struct *tsk);
+#else
+static inline void ptrace_put_breakpoints(struct task_struct *tsk) { }
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
+#endif /* __KERNEL */
 
 #endif
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 18d63ce..781abd1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1537,6 +1537,9 @@ struct task_struct {
 		unsigned long memsw_nr_pages; /* uncharged mem+swap usage */
 	} memcg_batch;
 #endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	atomic_t ptrace_bp_refcnt;
+#endif
 };
 
 /* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/exit.c b/kernel/exit.c
index f5d2f63..8dd8741 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1016,7 +1016,7 @@ NORET_TYPE void do_exit(long code)
 	/*
 	 * FIXME: do that only when needed, using sched_exit tracepoint
 	 */
-	flush_ptrace_hw_breakpoint(tsk);
+	ptrace_put_breakpoints(tsk);
 
 	exit_notify(tsk, group_dead);
 #ifdef CONFIG_NUMA
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 0fc1eed..dc7ab65 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
 #include <linux/regset.h>
+#include <linux/hw_breakpoint.h>
 
 
 /*
@@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
 	return ret;
 }
 #endif	/* CONFIG_COMPAT */
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+int ptrace_get_breakpoints(struct task_struct *tsk)
+{
+	if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
+		return 0;
+
+	return -1;
+}
+
+void ptrace_put_breakpoints(struct task_struct *tsk)
+{
+	if (atomic_dec_and_test(&tsk->ptrace_bp_refcnt))
+		flush_ptrace_hw_breakpoint(tsk);
+}
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */

[tip:perf/urgent] x86, hw_breakpoints: Fix racy access to ptrace breakpoints

[tip-bot for Frederic Weisbecker]

Commit-ID:  87dc669ba25777b67796d7262c569429e58b1ed4
Gitweb:     http://git.kernel.org/tip/87dc669ba25777b67796d7262c569429e58b1ed4
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Fri, 8 Apr 2011 17:29:36 +0200
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Mon, 25 Apr 2011 17:32:40 +0200

x86, hw_breakpoints: Fix racy access to ptrace breakpoints

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: v2.6.33.. <stable@kernel.org>
Link: http://lkml.kernel.org/r/1302284067-7860-3-git-send-email-fweisbec@gmail.com
---
 arch/x86/kernel/ptrace.c |   36 ++++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 45892dc..f65e5b5 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -608,6 +608,9 @@ static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)
 	unsigned len, type;
 	struct perf_event *bp;
 
+	if (ptrace_get_breakpoints(tsk) < 0)
+		return -ESRCH;
+
 	data &= ~DR_CONTROL_RESERVED;
 	old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
 restore:
@@ -655,6 +658,9 @@ restore:
 		}
 		goto restore;
 	}
+
+	ptrace_put_breakpoints(tsk);
+
 	return ((orig_ret < 0) ? orig_ret : rc);
 }
 
@@ -668,10 +674,17 @@ static unsigned long ptrace_get_debugreg(struct task_struct *tsk, int n)
 
 	if (n < HBP_NUM) {
 		struct perf_event *bp;
+
+		if (ptrace_get_breakpoints(tsk) < 0)
+			return -ESRCH;
+
 		bp = thread->ptrace_bps[n];
 		if (!bp)
-			return 0;
-		val = bp->hw.info.address;
+			val = 0;
+		else
+			val = bp->hw.info.address;
+
+		ptrace_put_breakpoints(tsk);
 	} else if (n == 6) {
 		val = thread->debugreg6;
 	 } else if (n == 7) {
@@ -686,6 +699,10 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 	struct perf_event *bp;
 	struct thread_struct *t = &tsk->thread;
 	struct perf_event_attr attr;
+	int err = 0;
+
+	if (ptrace_get_breakpoints(tsk) < 0)
+		return -ESRCH;
 
 	if (!t->ptrace_bps[nr]) {
 		ptrace_breakpoint_init(&attr);
@@ -709,24 +726,23 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 		 * writing for the user. And anyway this is the previous
 		 * behaviour.
 		 */
-		if (IS_ERR(bp))
-			return PTR_ERR(bp);
+		if (IS_ERR(bp)) {
+			err = PTR_ERR(bp);
+			goto put;
+		}
 
 		t->ptrace_bps[nr] = bp;
 	} else {
-		int err;
-
 		bp = t->ptrace_bps[nr];
 
 		attr = bp->attr;
 		attr.bp_addr = addr;
 		err = modify_user_hw_breakpoint(bp, &attr);
-		if (err)
-			return err;
 	}
 
-
-	return 0;
+put:
+	ptrace_put_breakpoints(tsk);
+	return err;
 }
 
 /*

[tip:perf/urgent] powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints

[tip-bot for Frederic Weisbecker]

Commit-ID:  07fa7a0a8a586c01a8b416358c7012dcb9dc688d
Gitweb:     http://git.kernel.org/tip/07fa7a0a8a586c01a8b416358c7012dcb9dc688d
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Fri, 8 Apr 2011 17:29:36 +0200
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Mon, 25 Apr 2011 17:33:54 +0200

powerpc, hw_breakpoints: Fix racy access to ptrace breakpoints

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: Prasad <prasad@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: v2.6.33.. <stable@kernel.org>
Link: http://lkml.kernel.org/r/1302284067-7860-4-git-send-email-fweisbec@gmail.com
---
 arch/powerpc/kernel/ptrace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 55613e3..4edeeb3 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1591,7 +1591,10 @@ long arch_ptrace(struct task_struct *child, long request,
 	}
 
 	case PTRACE_SET_DEBUGREG:
+		if (ptrace_get_breakpoints(child) < 0)
+			return -ESRCH;
 		ret = ptrace_set_debugreg(child, addr, data);
+		ptrace_put_breakpoints(child);
 		break;
 
 #ifdef CONFIG_PPC64

[tip:perf/urgent] arm, hw_breakpoints: Fix racy access to ptrace breakpoints

[tip-bot for Frederic Weisbecker]

Commit-ID:  bf0b8f4b55e591ba417c2dbaff42769e1fc773b0
Gitweb:     http://git.kernel.org/tip/bf0b8f4b55e591ba417c2dbaff42769e1fc773b0
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Fri, 8 Apr 2011 17:29:36 +0200
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Mon, 25 Apr 2011 17:35:18 +0200

arm, hw_breakpoints: Fix racy access to ptrace breakpoints

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Link: http://lkml.kernel.org/r/1302284067-7860-5-git-send-email-fweisbec@gmail.com
---
 arch/arm/kernel/ptrace.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 2bf27f3..8182f45 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -767,12 +767,20 @@ long arch_ptrace(struct task_struct *child, long request,
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 		case PTRACE_GETHBPREGS:
+			if (ptrace_get_breakpoints(child) < 0)
+				return -ESRCH;
+
 			ret = ptrace_gethbpregs(child, addr,
 						(unsigned long __user *)data);
+			ptrace_put_breakpoints(child);
 			break;
 		case PTRACE_SETHBPREGS:
+			if (ptrace_get_breakpoints(child) < 0)
+				return -ESRCH;
+
 			ret = ptrace_sethbpregs(child, addr,
 						(unsigned long __user *)data);
+			ptrace_put_breakpoints(child);
 			break;
 #endif
 

[tip:perf/urgent] sh, hw_breakpoints: Fix racy access to ptrace breakpoints

[tip-bot for Frederic Weisbecker]

Commit-ID:  e0ac8457d020c0289ea566917267da9e5e6d9865
Gitweb:     http://git.kernel.org/tip/e0ac8457d020c0289ea566917267da9e5e6d9865
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Fri, 8 Apr 2011 17:29:36 +0200
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Mon, 25 Apr 2011 17:36:12 +0200

sh, hw_breakpoints: Fix racy access to ptrace breakpoints

While the tracer accesses ptrace breakpoints, the child task may
concurrently exit due to a SIGKILL and thus release its breakpoints
at the same time. We can then dereference some freed pointers.

To fix this, hold a reference on the child breakpoints before
manipulating them.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: Paul Mundt <lethal@linux-sh.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Link: http://lkml.kernel.org/r/1302284067-7860-6-git-send-email-fweisbec@gmail.com
---
 arch/sh/kernel/ptrace_32.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/sh/kernel/ptrace_32.c b/arch/sh/kernel/ptrace_32.c
index 2130ca6..3d7b209 100644
--- a/arch/sh/kernel/ptrace_32.c
+++ b/arch/sh/kernel/ptrace_32.c
@@ -117,7 +117,11 @@ void user_enable_single_step(struct task_struct *child)
 
 	set_tsk_thread_flag(child, TIF_SINGLESTEP);
 
+	if (ptrace_get_breakpoints(child) < 0)
+		return;
+
 	set_single_step(child, pc);
+	ptrace_put_breakpoints(child);
 }
 
 void user_disable_single_step(struct task_struct *child)

Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Frederic Weisbecker]

On Wed, May 04, 2011 at 08:31:06AM +0200, Ingo Molnar wrote:
> 
> (Linus and Andrew Cc:-ed as well)
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > When a task is traced and is in a stopped state, the tracer
> > may execute a ptrace request to examine the tracee state and
> > get its task struct. Right after, the tracee can be killed
> > and thus its breakpoints released.
> > This can happen concurrently when the tracer is in the middle
> > of reading or modifying these breakpoints, leading to dereferencing
> > a freed pointer.
> > 
> > Hence, to prepare the fix, create a generic breakpoint reference
> > holding API. When a reference on the breakpoints of a task is
> > held, the breakpoints won't be released until the last reference
> > is dropped. After that, no more ptrace request on the task's
> > breakpoints can be serviced for the tracer.
> > 
> > Reported-by: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> Ok, this series looks a bit scary - and this ptrace.h change does not have 
> Oleg's Acked-by. (the arch bits all have maintaner Acked-by's)
> 
> The changes look a bit ugly as well: beyond the ugly ifdeffery, we have 
> ptrace.h::ptrace_init_task(), which is only used in 
> tracehook.h::tracehook_finish_clone() which is only used in 
> kernel/fork.c::copy_process().
> 
> That's two levels of obfuscation to do something rather simple - i think we 
> should get rid of the tracehook.h redirections, it did not work out in the end 
> as a method of capturing events - ftrace TRACE_EVENT() seems better structured 
> and more maintainable.
> 
> But i guess we could live with this fix for v2.6.39, if neither Oleg nor Linus 
> and Andrew are hating this further complication of the ptrace mess enough to 
> NAK it. Thoughts?
> 
> Plus, i'd really love it if you did some stress-testing of this change of a 
> mixed ptrace breakpoints and perf breakpoints workload, on some sufficiently 
> SMP box. gdb's hbreak is a very low freq way of testing thus such regressions 
> take ages to be reported.

But the perf breakpoints (those created using perf syscall) are not touched
at all by this patch. Only the ptrace ones.

What I can stress test is trying some ptrace breakpoint request and at the
same time SIGKILL the child, which is the only way to reproduce the bug
supposed to be fixed. And run that in a loop for one night. I'll try that.

Re: [PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Ingo Molnar]

(Linus and Andrew Cc:-ed as well)

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> When a task is traced and is in a stopped state, the tracer
> may execute a ptrace request to examine the tracee state and
> get its task struct. Right after, the tracee can be killed
> and thus its breakpoints released.
> This can happen concurrently when the tracer is in the middle
> of reading or modifying these breakpoints, leading to dereferencing
> a freed pointer.
> 
> Hence, to prepare the fix, create a generic breakpoint reference
> holding API. When a reference on the breakpoints of a task is
> held, the breakpoints won't be released until the last reference
> is dropped. After that, no more ptrace request on the task's
> breakpoints can be serviced for the tracer.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Ok, this series looks a bit scary - and this ptrace.h change does not have 
Oleg's Acked-by. (the arch bits all have maintaner Acked-by's)

The changes look a bit ugly as well: beyond the ugly ifdeffery, we have 
ptrace.h::ptrace_init_task(), which is only used in 
tracehook.h::tracehook_finish_clone() which is only used in 
kernel/fork.c::copy_process().

That's two levels of obfuscation to do something rather simple - i think we 
should get rid of the tracehook.h redirections, it did not work out in the end 
as a method of capturing events - ftrace TRACE_EVENT() seems better structured 
and more maintainable.

But i guess we could live with this fix for v2.6.39, if neither Oleg nor Linus 
and Andrew are hating this further complication of the ptrace mess enough to 
NAK it. Thoughts?

Plus, i'd really love it if you did some stress-testing of this change of a 
mixed ptrace breakpoints and perf breakpoints workload, on some sufficiently 
SMP box. gdb's hbreak is a very low freq way of testing thus such regressions 
take ages to be reported.

Thanks,

	Ingo

[PATCH 1/5] ptrace: Prepare to fix racy accesses on task breakpoints

[Frederic Weisbecker]

When a task is traced and is in a stopped state, the tracer
may execute a ptrace request to examine the tracee state and
get its task struct. Right after, the tracee can be killed
and thus its breakpoints released.
This can happen concurrently when the tracer is in the middle
of reading or modifying these breakpoints, leading to dereferencing
a freed pointer.

Hence, to prepare the fix, create a generic breakpoint reference
holding API. When a reference on the breakpoints of a task is
held, the breakpoints won't be released until the last reference
is dropped. After that, no more ptrace request on the task's
breakpoints can be serviced for the tracer.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Prasad <prasad@linux.vnet.ibm.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: v2.6.33.. <stable@kernel.org>
Link: http://lkml.kernel.org/r/1302284067-7860-2-git-send-email-fweisbec@gmail.com
---
 include/linux/ptrace.h |   13 ++++++++++++-
 include/linux/sched.h  |    3 +++
 kernel/exit.c          |    2 +-
 kernel/ptrace.c        |   17 +++++++++++++++++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index a1147e5..9178d5c 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -189,6 +189,10 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
 		child->ptrace = current->ptrace;
 		__ptrace_link(child, current->parent);
 	}
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	atomic_set(&child->ptrace_bp_refcnt, 1);
+#endif
 }
 
 /**
@@ -350,6 +354,13 @@ extern int task_current_syscall(struct task_struct *target, long *callno,
 				unsigned long args[6], unsigned int maxargs,
 				unsigned long *sp, unsigned long *pc);
 
-#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+extern int ptrace_get_breakpoints(struct task_struct *tsk);
+extern void ptrace_put_breakpoints(struct task_struct *tsk);
+#else
+static inline void ptrace_put_breakpoints(struct task_struct *tsk) { }
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
+#endif /* __KERNEL */
 
 #endif
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 18d63ce..781abd1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1537,6 +1537,9 @@ struct task_struct {
 		unsigned long memsw_nr_pages; /* uncharged mem+swap usage */
 	} memcg_batch;
 #endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	atomic_t ptrace_bp_refcnt;
+#endif
 };
 
 /* Future-safe accessor for struct task_struct's cpus_allowed. */
diff --git a/kernel/exit.c b/kernel/exit.c
index f5d2f63..8dd8741 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1016,7 +1016,7 @@ NORET_TYPE void do_exit(long code)
 	/*
 	 * FIXME: do that only when needed, using sched_exit tracepoint
 	 */
-	flush_ptrace_hw_breakpoint(tsk);
+	ptrace_put_breakpoints(tsk);
 
 	exit_notify(tsk, group_dead);
 #ifdef CONFIG_NUMA
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 0fc1eed..dc7ab65 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -22,6 +22,7 @@
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
 #include <linux/regset.h>
+#include <linux/hw_breakpoint.h>
 
 
 /*
@@ -879,3 +880,19 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
 	return ret;
 }
 #endif	/* CONFIG_COMPAT */
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+int ptrace_get_breakpoints(struct task_struct *tsk)
+{
+	if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
+		return 0;
+
+	return -1;
+}
+
+void ptrace_put_breakpoints(struct task_struct *tsk)
+{
+	if (atomic_dec_and_test(&tsk->ptrace_bp_refcnt))
+		flush_ptrace_hw_breakpoint(tsk);
+}
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
-- 
1.7.3.2