linux-kernel - Re: [PATCH, for 2.6.29] ptrace: fix the usage of ptrace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090210184014.GA30545@redhat.com>
Date:	Tue, 10 Feb 2009 19:40:14 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Metzger, Markus T" <markus.t.metzger@...el.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Roland McGrath <roland@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Markus Metzger <markus.t.metzger@...glemail.com>
Subject: Re: [PATCH, for 2.6.29] ptrace: fix the usage of ptrace_fork()

On 02/10, Metzger, Markus T wrote:
>
> If I understand this correctly, we have two problems left: 1. if the
> tracer thread dies without detaching, the process will not get the
> (locked) memory refunded.

Yes.

> 2. there is a race between a thread detaching
> and another thread releasing the same task.
>
>
> I do not really understand the second problem.
>
> As far as I know, there can only be one ptracer per task. This ptracer
> can either detach or release, but not both. That other thread that does
> do_wait() should not be able to see the tracee as long as it is ptraced
> (wait_consider_task() will ignore it).

Please note that do_wait() does

	tsk = current;
	do {

		ptrace_do_wait(tsk, ...);

		tsk = next_thread(tsk);
	} while (tsk != current);

So the sub-thread of the tracer can reap the tracee, please see below.

> Since ptrace_disable() is called
> before __ptrace_unlink(), we free the BTS buffer before do_wait() will
> consider the tracee.

They both can free it in parallel.

Suppose we have 2 threads T1 and T2, C is a child of T1 (this is not
strictly necessary, just for simplicity),

T1 attaches to C, does PTRACE_BTS_CONFIG, and then starts PTRACE_DETACH.
When it calls ptrace_detach(), C is TASK_TRACED.

C is killed by SIGKILL, C exits and becomes a zombie. Not a problem
for T1, it has a reference to task_struct.

T1 calls ptrace_disable()->ptrace_bts_detach().

T2 calls do_wait(), the second iteration of the "do while" loop above
finds the "eligible" child C, and calls wait_task_zombie(), which in
turn does release_task()->ptrace_unlink()->...->ptrace_bts_untrace().

Now, T1->ptrace_bts_detach() can race with T2->ptrace_bts_untrace(), they
both can see ->bts != NULL, and they both can do kfree/ds_release_bts.

(and we have another similar race with de_thread() which can call
 release_task() too).

> I do not see the race. Am I missing something?

Or perhaps it is me who missed something, I didn't try to verify the
problem...

> We could try to mimic that and add a ptrace_notify_exit() function that is
> called early in do_exit().  As long as I only put the ptrace_bts_detach()
> into the arch version of it, the changes should be relatively safe.

Yes, we can do untrace earlier, but we still have the problems with tasklist_lock.
Of course, we can add the special function which does ptrace_bts_untrace()
for each tracee under tasklist and returns the size of the freed buffer,
then we drop tasklist and update ->mm. But this is soooo ugly...

And this can't resolve the problem with do_wait/de_thread which
can do ptrace_bts_untrace() before us.

> What do you and Roland think about it? Do you have a better idea?

We should cleanup ptrace first ;) IOW, I don't have a good idea.

Perhaps, for 2.6.29, we can do something like the "patch" below?

(btw, do you agree with the change in copy_process() I sent? )

> I would appreciate, if
> you reviewed future patches in that area.

Please CC me, I'll try to review. But I only understand (more or
less) the process-management part of ptrace...

Oleg.

--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -810,11 +810,15 @@ static void ptrace_bts_untrace(struct ta

 static void ptrace_bts_detach(struct task_struct *child)
 {
+	// We can race with de_thread/do_wait which
+	// can do ptrace_bts_untrace() before us
 	if (unlikely(child->bts)) {
-		ds_release_bts(child->bts);
-		child->bts = NULL;
-
-		ptrace_bts_free_buffer(child);
+		// This all will be freed by ptrace_bts_untrace()
+		// later, but we should update ->mm
+		down_write(->mmap_sem);
+		mm->total_vm  -= bts_size;
+		mm->locked_vm -= bts_size);
+		up_write(->mmap_sem);
 	}
 }
 #else

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/