hw_breakpoints: Fix racy ptrace breakpoint acccesses

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.

kernel/ptrace.c +1 -0

index 0fc1eed..dc7ab65 100644
@@ -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;
 }

Hi Frederic,

Quoted 7 lines · 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.

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

Quoted 20 lines

kernel/ptrace.c +5 -0

index 0fc1eed..dc7ab65 100644
@@ -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)
+{

kernel/ptrace.c asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid, · line 891

Quoted 5 lines

kernel/ptrace.c +5 -0

+       if (atomic_inc_not_zero(&tsk->ptrace_bp_refcnt))
+               return 0;
+
+       return -1;
+}

Would it be better to return -ESRCH here instead?

Quoted 9 lines · Frederic Weisbecker

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 20 lines

Quoted 20 lines

kernel/ptrace.c +5 -0

index 0fc1eed..dc7ab65 100644
@@ -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)
+{

Quoted 8 lines

kernel/ptrace.c asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid, · line 891

Quoted 5 lines

kernel/ptrace.c +5 -0

+       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?

Quoted 2 lines

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 :).

Quoted 11 lines

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.

Quoted 1 line

Right?

Yup.

For this and the ARM patch:

Acked-by Will Deacon <[email protected]>

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 <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>

Cc Prasad <[email protected]>

Cc Paul Mundt <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc v2.6.33.. <[email protected]>

---
 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(-)

include/linux/ptrace.h +12 -1

index a1147e5..9178d5c 100644
@@ -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

include/linux/sched.h +3 -0

index 83bd2e2..15badfa 100644
@@ -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. */

kernel/exit.c +1 -1

index 6a488ad..437e327 100644
@@ -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

kernel/ptrace.c +17 -0

index 0fc1eed..dc7ab65 100644
@@ -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 */

Signature · 1 line

1.7.3.2

(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 <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>

Cc Prasad <[email protected]>

Cc Paul Mundt <[email protected]>

Cc v2.6.33.. <[email protected]>

---
 arch/powerpc/kernel/ptrace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

arch/powerpc/kernel/ptrace.c +3 -0

index 55613e3..4edeeb3 100644
@@ -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

Signature · 1 line

1.7.3.2

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 <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>

Cc Prasad <[email protected]>

Cc Paul Mundt <[email protected]>

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(-)

Signature · 1 line

1.7.3.2