[patch] x86, ptrace: fix double-free on race

[patch] x86, ptrace: fix double-free on race

[Markus Metzger]

Ptrace_detach() races with __ptrace_unlink() if the traced task is
reaped while detaching. This might cause a double-free of the BTS
buffer.

Change the ptrace_detach() path to only do the memory accounting in
ptrace_bts_detach() and leave the buffer free to ptrace_bts_untrace()
which will be called from __ptrace_unlink().

The fix follows a proposal from Oleg Nesterov.


Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Markus Metzger <markus.t.metzger@intel.com>
---

Index: gits/arch/x86/kernel/ptrace.c
===================================================================
--- gits.orig/arch/x86/kernel/ptrace.c	2009-01-22 13:09:37.000000000 +0100
+++ gits/arch/x86/kernel/ptrace.c	2009-02-11 08:55:49.000000000 +0100
@@ -810,12 +810,14 @@
 
 static void ptrace_bts_detach(struct task_struct *child)
 {
-	if (unlikely(child->bts)) {
-		ds_release_bts(child->bts);
-		child->bts = NULL;
-
-		ptrace_bts_free_buffer(child);
-	}
+	/* Ptrace_detach() races with ptrace_untrace() in case
+	 * the child dies and is reaped by another thread.
+	 *
+	 * We only do the memory accounting at this point and
+	 * leave the buffer deallocation and the bts tracer
+	 * release to ptrace_bts_untrace() which will be called
+	 * later on with tasklist_lock held. */
+	release_locked_buffer(child->bts_buffer, child->bts_size);
 }
 #else
 static inline void ptrace_bts_fork(struct task_struct *tsk) {}
Index: gits/include/linux/mm.h
===================================================================
--- gits.orig/include/linux/mm.h	2009-02-11 08:40:37.000000000 +0100
+++ gits/include/linux/mm.h	2009-02-11 08:51:28.000000000 +0100
@@ -1304,5 +1304,6 @@
 
 extern void *alloc_locked_buffer(size_t size);
 extern void free_locked_buffer(void *buffer, size_t size);
+extern void release_locked_buffer(void *buffer, size_t size);
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */
Index: gits/mm/mlock.c
===================================================================
--- gits.orig/mm/mlock.c	2009-02-11 08:40:37.000000000 +0100
+++ gits/mm/mlock.c	2009-02-11 08:49:17.000000000 +0100
@@ -660,7 +660,7 @@
 	return buffer;
 }
 
-void free_locked_buffer(void *buffer, size_t size)
+void release_locked_buffer(void *buffer, size_t size)
 {
 	unsigned long pgsz = PAGE_ALIGN(size) >> PAGE_SHIFT;
 
@@ -670,6 +670,11 @@
 	current->mm->locked_vm -= pgsz;
 
 	up_write(&current->mm->mmap_sem);
+}
+
+void free_locked_buffer(void *buffer, size_t size)
+{
+	release_locked_buffer(buffer, size);
 
 	kfree(buffer);
 }
---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [patch] x86, ptrace: fix double-free on race

[Ingo Molnar]

* Markus Metzger <markus.t.metzger@intel.com> wrote:

> Ptrace_detach() races with __ptrace_unlink() if the traced task is
> reaped while detaching. This might cause a double-free of the BTS
> buffer.
> 
> Change the ptrace_detach() path to only do the memory accounting in
> ptrace_bts_detach() and leave the buffer free to ptrace_bts_untrace()
> which will be called from __ptrace_unlink().
> 
> The fix follows a proposal from Oleg Nesterov.
> 
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Markus Metzger <markus.t.metzger@intel.com>

Applied to tip:x86/urgent, thanks Markus!

Note, i fixed up the comment style to match the rest of ptrace.c,
see the final commit below.

	Ingo

-------------------->
>From 9f339e7028e2855717af3193c938f9960ad13b38 Mon Sep 17 00:00:00 2001
From: Markus Metzger <markus.t.metzger@intel.com>
Date: Wed, 11 Feb 2009 15:10:27 +0100
Subject: [PATCH] x86, ptrace, mm: fix double-free on race

Ptrace_detach() races with __ptrace_unlink() if the traced task is
reaped while detaching. This might cause a double-free of the BTS
buffer.

Change the ptrace_detach() path to only do the memory accounting in
ptrace_bts_detach() and leave the buffer free to ptrace_bts_untrace()
which will be called from __ptrace_unlink().

The fix follows a proposal from Oleg Nesterov.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Markus Metzger <markus.t.metzger@intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/ptrace.c |   16 ++++++++++------
 include/linux/mm.h       |    1 +
 mm/mlock.c               |    7 ++++++-
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 0a5df5f..5a4c23d 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -810,12 +810,16 @@ static void ptrace_bts_untrace(struct task_struct *child)
 
 static void ptrace_bts_detach(struct task_struct *child)
 {
-	if (unlikely(child->bts)) {
-		ds_release_bts(child->bts);
-		child->bts = NULL;
-
-		ptrace_bts_free_buffer(child);
-	}
+	/*
+	 * Ptrace_detach() races with ptrace_untrace() in case
+	 * the child dies and is reaped by another thread.
+	 *
+	 * We only do the memory accounting at this point and
+	 * leave the buffer deallocation and the bts tracer
+	 * release to ptrace_bts_untrace() which will be called
+	 * later on with tasklist_lock held.
+	 */
+	release_locked_buffer(child->bts_buffer, child->bts_size);
 }
 #else
 static inline void ptrace_bts_fork(struct task_struct *tsk) {}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index e8ddc98..3d7fb44 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1305,5 +1305,6 @@ void vmemmap_populate_print_last(void);
 
 extern void *alloc_locked_buffer(size_t size);
 extern void free_locked_buffer(void *buffer, size_t size);
+extern void release_locked_buffer(void *buffer, size_t size);
 #endif /* __KERNEL__ */
 #endif /* _LINUX_MM_H */
diff --git a/mm/mlock.c b/mm/mlock.c
index 028ec48..2b57f7e 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -657,7 +657,7 @@ void *alloc_locked_buffer(size_t size)
 	return buffer;
 }
 
-void free_locked_buffer(void *buffer, size_t size)
+void release_locked_buffer(void *buffer, size_t size)
 {
 	unsigned long pgsz = PAGE_ALIGN(size) >> PAGE_SHIFT;
 
@@ -667,6 +667,11 @@ void free_locked_buffer(void *buffer, size_t size)
 	current->mm->locked_vm -= pgsz;
 
 	up_write(&current->mm->mmap_sem);
+}
+
+void free_locked_buffer(void *buffer, size_t size)
+{
+	release_locked_buffer(buffer, size);
 
 	kfree(buffer);
 }

Re: [patch] x86, ptrace: fix double-free on race

[Oleg Nesterov]

On 02/11, Markus Metzger wrote:
>
> Ptrace_detach() races with __ptrace_unlink() if the traced task is
> reaped while detaching. This might cause a double-free of the BTS
> buffer.
>
> Change the ptrace_detach() path to only do the memory accounting in
> ptrace_bts_detach() and leave the buffer free to ptrace_bts_untrace()
> which will be called from __ptrace_unlink().

Thanks Markus, I think this should close the "really bad" problems
for 2.6.29.


Off-topic. This is subjective, so please feel to ignore, but personally
I dislike the usage of ptrace_fork() in copy_process(). And ptrace_fork()
itself.

To me, this has nothing to do with ptrace at all. copy_process() or
dup_task_struct() just must clear/tweak some fields after memcpy().

Perhaps it is better to kill all these ptrace_fork/arch_ptrace_fork/
x86_ptrace_fork/ptrace_bts_fork helpers, and make a single function

	static inline void arch_bts_init(struct task_struct *p)
	{
	#ifdef CONFIG_X86_PTRACE_BTS
		if (unlikely(p->bts)) {
			p->bts = NULL;
			p->bts_buffer = NULL;
			p->bts_size = 0;
			p->thread.bts_ovfl_signal = 0;
		}
	#endif /* CONFIG_X86_PTRACE_BTS */
	}

which is called by arch_dup_task_struct(). The "if (ptrace)" check
in copy_process() is just wrong imho, even if it is currently correct.
Let's suppose we add, say, bts_spinlock to the fields above. In that
case we must initialize it even if the forking task is not ptraced.

Of course, in any case this is not 2.6.29 material.


Btw, just curious, bts_ovfl_signal is not used, except the user can
set/get it via PTRACE_BTS_CONFIG/PTRACE_BTS_STATUS ?

Oleg.

RE: [patch] x86, ptrace: fix double-free on race

[Metzger, Markus T]

>-----Original Message-----
>From: Oleg Nesterov [mailto:oleg@redhat.com]
>Sent: Wednesday, February 11, 2009 11:08 PM
>To: Metzger, Markus T


>Off-topic. This is subjective, so please feel to ignore, but personally
>I dislike the usage of ptrace_fork() in copy_process(). And ptrace_fork()
>itself.
>
>To me, this has nothing to do with ptrace at all. copy_process() or
>dup_task_struct() just must clear/tweak some fields after memcpy().
>
>Perhaps it is better to kill all these ptrace_fork/arch_ptrace_fork/
>x86_ptrace_fork/ptrace_bts_fork helpers, and make a single function
>
>	static inline void arch_bts_init(struct task_struct *p)
>	{
>	#ifdef CONFIG_X86_PTRACE_BTS
>		if (unlikely(p->bts)) {
>			p->bts = NULL;
>			p->bts_buffer = NULL;
>			p->bts_size = 0;
>			p->thread.bts_ovfl_signal = 0;
>		}
>	#endif /* CONFIG_X86_PTRACE_BTS */
>	}

It looked cleaner and more consistent to me, that way.

Ptrace passes things down to arch in other cases already,
e.g. ptrace_detach()->ptrace_disable(), ptrace()->arch_ptrace(),
ptrace_attach()->arch_ptrace_attach().

I think the function should be in arch/x86/ptrace.c - if we call it from
ptrace or from arch_dup_task_struct() doesn't really matter.

Do you want me to move the call to arch_dup_task_struct()?


>which is called by arch_dup_task_struct(). The "if (ptrace)" check
>in copy_process() is just wrong imho, even if it is currently correct.

You're right, it is not strictly necessary and would actually be in the
way if we added more fields. I'm OK to remove it.


>Btw, just curious, bts_ovfl_signal is not used, except the user can
>set/get it via PTRACE_BTS_CONFIG/PTRACE_BTS_STATUS ?

It's not really necessary for debugging, but we wanted to get the interface
general enough to support other use cases.

There's already a PMI interrupt handler in perfmon which handles the PEBS
buffer overflow. Stephane was planning to extract it so it can be used by
ds.c. Once that is done, we were planning to move the buffer overflow
handling for BTS and PEBS into ds.c and enable the feature in ptrace.


regards,
markus.

---------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen Germany
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 Ust.-IdNr.
VAT Registration No.: DE129385895
Citibank Frankfurt (BLZ 502 109 00) 600119052

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [patch] x86, ptrace: fix double-free on race

[Oleg Nesterov]

On 02/12, Metzger, Markus T wrote:
>
> >-----Original Message-----
> >From: Oleg Nesterov [mailto:oleg@redhat.com]
> >
> >	static inline void arch_bts_init(struct task_struct *p)
> >	{
> >	#ifdef CONFIG_X86_PTRACE_BTS
> >		if (unlikely(p->bts)) {
> >			p->bts = NULL;
> >			p->bts_buffer = NULL;
> >			p->bts_size = 0;
> >			p->thread.bts_ovfl_signal = 0;
> >		}
> >	#endif /* CONFIG_X86_PTRACE_BTS */
> >	}
>
> It looked cleaner and more consistent to me, that way.
>
> Ptrace passes things down to arch in other cases already,
> e.g. ptrace_detach()->ptrace_disable(), ptrace()->arch_ptrace(),
> ptrace_attach()->arch_ptrace_attach().
>
> I think the function should be in arch/x86/ptrace.c - if we call it from
> ptrace or from arch_dup_task_struct() doesn't really matter.
>
> Do you want me to move the call to arch_dup_task_struct()?

Please do what you like more. We can call this from dup_task_struct()
or even from copy_process(). Actually, the initialization above is not
arch dependent, it only depends on CONFIG_BTS.

> >Btw, just curious, bts_ovfl_signal is not used, except the user can
> >set/get it via PTRACE_BTS_CONFIG/PTRACE_BTS_STATUS ?
>
> It's not really necessary for debugging, but we wanted to get the interface
> general enough to support other use cases.
>
> There's already a PMI interrupt handler in perfmon which handles the PEBS
> buffer overflow. Stephane was planning to extract it so it can be used by
> ds.c. Once that is done, we were planning to move the buffer overflow
> handling for BTS and PEBS into ds.c and enable the feature in ptrace.

OK, thanks. (of course I don't understand this magic, but hopefully
can understand your answer ;)

Oleg.