[PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Oleg Nesterov]

- Add PF_KTHREAD check to prevent attaching to the kernel thread
  with a borrowed ->mm.

  With or without this change we can race with daemonize() which
  can set PF_KTHREAD or clear ->mm after ptrace_attach() does the
  check, but this doesn't matter because reparent_to_kthreadd()
  does ptrace_unlink().

- Kill "!task->mm" check. We don't really care about ->mm != NULL,
  and the task can call exit_mm() right after we drop task_lock().
  What we need is to make sure we can't attach after exit_notify(),
  check task->exit_state != 0 instead.

Also, move the "already traced" check down for cosmetic reasons.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/ptrace.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

--- PTRACE/kernel/ptrace.c~1_KTHREADS	2009-05-05 21:37:26.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-05-05 23:17:53.000000000 +0200
@@ -182,6 +182,8 @@ int ptrace_attach(struct task_struct *ta
 	audit_ptrace(task);
 
 	retval = -EPERM;
+	if (unlikely(task->flags & PF_KTHREAD))
+		goto out;
 	if (same_thread_group(task, current))
 		goto out;
 
@@ -191,8 +193,6 @@ int ptrace_attach(struct task_struct *ta
 	retval = mutex_lock_interruptible(&task->cred_exec_mutex);
 	if (retval  < 0)
 		goto out;
-
-	retval = -EPERM;
 repeat:
 	/*
 	 * Nasty, nasty.
@@ -212,23 +212,24 @@ repeat:
 		goto repeat;
 	}
 
-	if (!task->mm)
-		goto bad;
-	/* the same process cannot be attached many times */
-	if (task->ptrace & PT_PTRACED)
-		goto bad;
 	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
 	if (retval)
 		goto bad;
 
-	/* Go */
+	retval = -EPERM;
+	if (unlikely(task->exit_state))
+		goto bad;
+	if (task->ptrace & PT_PTRACED)
+		goto bad;
+
 	task->ptrace |= PT_PTRACED;
 	if (capable(CAP_SYS_PTRACE))
 		task->ptrace |= PT_PTRACE_CAP;
 
 	__ptrace_link(task, current);
-
 	send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
+
+	retval = 0;
 bad:
 	write_unlock_irqrestore(&tasklist_lock, flags);
 	task_unlock(task);

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Andrew Morton]

On Wed, 6 May 2009 00:47:22 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> - Add PF_KTHREAD check to prevent attaching to the kernel thread
>   with a borrowed ->mm.
> 
>   With or without this change we can race with daemonize() which
>   can set PF_KTHREAD or clear ->mm after ptrace_attach() does the
>   check, but this doesn't matter because reparent_to_kthreadd()
>   does ptrace_unlink().
> 
> - Kill "!task->mm" check. We don't really care about ->mm != NULL,
>   and the task can call exit_mm() right after we drop task_lock().
>   What we need is to make sure we can't attach after exit_notify(),
>   check task->exit_state != 0 instead.
> 

These patches make a mess of utrace-core.patch.  Do we really want to do that?

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Oleg Nesterov]

On 05/05, Andrew Morton wrote:
>
> On Wed, 6 May 2009 00:47:22 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > - Add PF_KTHREAD check to prevent attaching to the kernel thread
> >   with a borrowed ->mm.
> >
> >   With or without this change we can race with daemonize() which
> >   can set PF_KTHREAD or clear ->mm after ptrace_attach() does the
> >   check, but this doesn't matter because reparent_to_kthreadd()
> >   does ptrace_unlink().
> >
> > - Kill "!task->mm" check. We don't really care about ->mm != NULL,
> >   and the task can call exit_mm() right after we drop task_lock().
> >   What we need is to make sure we can't attach after exit_notify(),
> >   check task->exit_state != 0 instead.
> >
>
> These patches make a mess of utrace-core.patch.  Do we really want to do that?

Aaaah. Sorry! forgot to clearify...

These patches depend on

	utrace-core-kill-exclude_xtrace-logic.patch

which hopefully can be folded into utrace-core.patch. In that case these
changes do not depend on utrace, and they can go ahead of utrace.

Is this acceptable for you ?

Oleg.

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Andrew Morton]

On Wed, 6 May 2009 01:57:36 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> On 05/05, Andrew Morton wrote:
> >
> > On Wed, 6 May 2009 00:47:22 +0200
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > - Add PF_KTHREAD check to prevent attaching to the kernel thread
> > >   with a borrowed ->mm.
> > >
> > >   With or without this change we can race with daemonize() which
> > >   can set PF_KTHREAD or clear ->mm after ptrace_attach() does the
> > >   check, but this doesn't matter because reparent_to_kthreadd()
> > >   does ptrace_unlink().
> > >
> > > - Kill "!task->mm" check. We don't really care about ->mm != NULL,
> > >   and the task can call exit_mm() right after we drop task_lock().
> > >   What we need is to make sure we can't attach after exit_notify(),
> > >   check task->exit_state != 0 instead.
> > >
> >
> > These patches make a mess of utrace-core.patch.  Do we really want to do that?
> 
> Aaaah. Sorry! forgot to clearify...
> 
> These patches depend on
> 
> 	utrace-core-kill-exclude_xtrace-logic.patch
> 
> which hopefully can be folded into utrace-core.patch. In that case these
> changes do not depend on utrace, and they can go ahead of utrace.
> 

oic.  Strange that the patches applied, but utrace didn't apply on them.

I now have:

#
# ptrace
#
ptrace-remove-pt_dtrace-from-arch-h8300.patch
ptrace-remove-pt_dtrace-from-avr32-mn10300-parisc-s390-sh-xtensa.patch
ptrace-remove-pt_dtrace-from-m68k-m68knommu.patch
ptrace-remove-pt_dtrace-from-arch-m32r.patch
ptrace-mm_need_new_owner-use-real_parent-to-search-in-the-siblings.patch
ptrace-tracehook_unsafe_exec-remove-the-stale-comment.patch
ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix.patch
ptrace-do-not-use-task-ptrace-directly-in-core-kernel.patch
#
# utrace
#
signals-tracehook_notify_jctl-change.patch
utrace-core.patch
utrace-core-kill-exclude_xtrace-logic.patch
#
ptrace-ptrace_attach-check-pf_kthread-exit_state-instead-of-mm.patch
ptrace-cleanup-check-set-of-pt_ptraced-during-attach.patch
ptrace-do-not-use-task_lock-for-attach.patch
ptrace_get_task_struct-s-tasklist-rcu-make-it-static.patch

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Roland McGrath]

This changes the order of the already-traced and security checks.
It would match the previous behavior to have the ->exit_state and ->ptrace
checks before __ptrace_may_access().  This is a small nit, but it could
affect whether some existing harmless usage pattern starts generating new
access failure logging from security modules (e.g. SELinux avc denials).

I don't see any reason you can't just swap the order back as it was before.
If you do that,
Acked-by: Roland McGrath <roland@redhat.com>


Thanks,
Roland

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Roland McGrath]

> I now have:
> 
> #
> # ptrace
> #
> ptrace-remove-pt_dtrace-from-arch-h8300.patch
> ptrace-remove-pt_dtrace-from-avr32-mn10300-parisc-s390-sh-xtensa.patch
> ptrace-remove-pt_dtrace-from-m68k-m68knommu.patch
> ptrace-remove-pt_dtrace-from-arch-m32r.patch
> ptrace-mm_need_new_owner-use-real_parent-to-search-in-the-siblings.patch
> ptrace-tracehook_unsafe_exec-remove-the-stale-comment.patch
> ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix.patch
> ptrace-do-not-use-task-ptrace-directly-in-core-kernel.patch
> #
> # utrace
> #
> signals-tracehook_notify_jctl-change.patch
> utrace-core.patch
> utrace-core-kill-exclude_xtrace-logic.patch

I'd swap these three to be after ...

> #
> ptrace-ptrace_attach-check-pf_kthread-exit_state-instead-of-mm.patch
> ptrace-cleanup-check-set-of-pt_ptraced-during-attach.patch
> ptrace-do-not-use-task_lock-for-attach.patch
> ptrace_get_task_struct-s-tasklist-rcu-make-it-static.patch

... all these.  Some more ptrace patches will come in and can also go after
these ptrace* ones but before the "utrace three".  When we get there we'll
replace those three including utrace with probably two (a revamp of the
tracehook change plus a single refreshed utrace.patch).


Thanks,
Roland

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Oleg Nesterov]

On 05/05, Roland McGrath wrote:
>
> This changes the order of the already-traced and security checks.
> It would match the previous behavior to have the ->exit_state and ->ptrace
> checks before __ptrace_may_access().  This is a small nit, but it could
> affect whether some existing harmless usage pattern starts generating new
> access failure logging from security modules (e.g. SELinux avc denials).

Another subtle change I forgot to comment.

> I don't see any reason you can't just swap the order back as it was before.

The last patch in series, "do not use task_lock()", is the reason.

We need tasklist for writing to check (and set) ->ptrace, but we need
task_lock() to call __ptrace_may_access().

We can preserve the current behaviour, we can do get_task_mm() beforehand,
modify __ptrace_may_access() a bit, and call __ptrace_may_access() under
tasklist later (in fact, this was the very first version of this patch which
I didn't send).

But do we really care? If selinux denies to ptrace this task, can't we
return -EACESS regardless of ->ptrace?

Oleg.

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Oleg Nesterov]

On 05/05, Roland McGrath wrote:
>
> > I now have:
> >
> > #
> > # ptrace
> > #
> > ptrace-remove-pt_dtrace-from-arch-h8300.patch
> > ptrace-remove-pt_dtrace-from-avr32-mn10300-parisc-s390-sh-xtensa.patch
> > ptrace-remove-pt_dtrace-from-m68k-m68knommu.patch
> > ptrace-remove-pt_dtrace-from-arch-m32r.patch
> > ptrace-mm_need_new_owner-use-real_parent-to-search-in-the-siblings.patch
> > ptrace-tracehook_unsafe_exec-remove-the-stale-comment.patch
> > ptrace-tracehook_unsafe_exec-remove-the-stale-comment-fix.patch
> > ptrace-do-not-use-task-ptrace-directly-in-core-kernel.patch
> > #
> > # utrace
> > #
> > signals-tracehook_notify_jctl-change.patch
> > utrace-core.patch
> > utrace-core-kill-exclude_xtrace-logic.patch
>
> I'd swap these three to be after ...
>
> > #
> > ptrace-ptrace_attach-check-pf_kthread-exit_state-instead-of-mm.patch
> > ptrace-cleanup-check-set-of-pt_ptraced-during-attach.patch
> > ptrace-do-not-use-task_lock-for-attach.patch
> > ptrace_get_task_struct-s-tasklist-rcu-make-it-static.patch

Yes, that was the plan.

But, to do this, utrace-core-kill-exclude_xtrace-logic.patch should be
folded into utrace-core.patch. Otherwise, utrace-core.patch alone can't
be applied after this new series.

Oleg.

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Andrew Morton]

On Wed, 6 May 2009 06:56:13 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> > > signals-tracehook_notify_jctl-change.patch
> > > utrace-core.patch
> > > utrace-core-kill-exclude_xtrace-logic.patch
> >
> > I'd swap these three to be after ...
> >
> > > #
> > > ptrace-ptrace_attach-check-pf_kthread-exit_state-instead-of-mm.patch
> > > ptrace-cleanup-check-set-of-pt_ptraced-during-attach.patch
> > > ptrace-do-not-use-task_lock-for-attach.patch
> > > ptrace_get_task_struct-s-tasklist-rcu-make-it-static.patch
> 
> Yes, that was the plan.
> 
> But, to do this, utrace-core-kill-exclude_xtrace-logic.patch should be
> folded into utrace-core.patch. Otherwise, utrace-core.patch alone can't
> be applied after this new series.

ooooh.

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Christoph Hellwig]

On Tue, May 05, 2009 at 04:47:33PM -0700, Andrew Morton wrote:
> These patches make a mess of utrace-core.patch.  Do we really want to do that?

Yes.  All these patches from Oleg are preparations for actually doing
a utrace merge in a sane way.  I don't really think utrace should be
in -mm at this point where active ptrace work is going on.

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, May 05, 2009 at 04:47:33PM -0700, Andrew Morton wrote:
> > These patches make a mess of utrace-core.patch.  Do we really want to do that?
> 
> Yes.  All these patches from Oleg are preparations for actually 
> doing a utrace merge in a sane way.  I don't really think utrace 
> should be in -mm at this point where active ptrace work is going 
> on.

Yes, as i said it in earlier threads, lacking other in-tree use, 
utrace is only acceptable for upstream if it materially cleans up 
the ptrace code here and now - without any wide #ifdefs like some of 
the earlier patches did.

The latest patches from Oleg clearly go in this direction, which is 
a very promising development!

	Ingo

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Roland McGrath]

> Another subtle change I forgot to comment.

You've got to leave some little things for me to notice and mention so I
can look like I'm paying attention. ;-)

> The last patch in series, "do not use task_lock()", is the reason.
> 
> We need tasklist for writing to check (and set) ->ptrace, but we need
> task_lock() to call __ptrace_may_access().

I see.

> We can preserve the current behaviour, we can do get_task_mm() beforehand,
> modify __ptrace_may_access() a bit, and call __ptrace_may_access() under
> tasklist later (in fact, this was the very first version of this patch which
> I didn't send).

Perhaps there is a way to reorder the patches that makes it simpler?

On second look, what does __ptrace_may_access() need task_lock() for anyway?

> But do we really care? If selinux denies to ptrace this task, can't we
> return -EACESS regardless of ->ptrace?

You're right that the return code is the same either way.  That's not the
issue.  The issue is whether this case calls security_ptrace_may_access()
at all, because doing so can have side effects.  Consider an application
that does:

	ptrace(PTRACE_ATTACH, pid);
	ptrace(PTRACE_ATTACH, pid);
	pid = wait(&status);

It works fine, because it doesn't care that the second call fails.
(A real-world example would be much more complex, with mounds of poorly
structured code so nobody can even tell what calls have already been made.
Maybe the first one would really have been the child doing PTRACE_TRACEME,
or inheriting via CLONE_PTRACE/PTRACE_O_TRACECLONE, etc.)

After your change, the application still works fine by itself.  But now,
the second call causes a security_ptrace_may_access() call that wasn't
there before.  This hits some crazy LSM arrangement we haven't even
thought of, and produces auditing complaints about improper ptrace
attempts.  Those log messages on a server trigger some fancy monitoring
thing that identifies them as unexpected and security-related, decides
that means they're urgent, and so pages the poor sysadmin and his boss
and his boss's boss with bright red "BREAK-IN ATTEMPTED IN THE
DATACENTER" pages vibrating their beltlines right when their hot dates
were about to get interesting.  The poor sysadmin spends the next month
of his life in rat holes of wild paranoia and reinstalling, and then
eventually in tedious explanations of how there was never any intrusion
but he'd just done an innocuous kernel upgrade that he knew was not
supposed to change any application behavior.

That is a ridiculous scenario that presupposes some huge, inscrutable,
and inanely-written applications in fragile arrangements with mindless
nonsense riddled with holes and called business-ready management and
monitoring infrastructure for "mission-critical security" purposes, and
then some admin reactions that range from clueless to irresponsible.
(That never happens in real deployments, right?)  But it illustrates the
pervasive law of unintended consequences.  That's what not perturbing
the deterministic aspects of system behavior without deliberate and
careful intent is all about.


Thanks,
Roland

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Oleg Nesterov]

On 05/06, Roland McGrath wrote:
>
> > We can preserve the current behaviour, we can do get_task_mm() beforehand,
> > modify __ptrace_may_access() a bit, and call __ptrace_may_access() under
> > tasklist later (in fact, this was the very first version of this patch which
> > I didn't send).
>
> Perhaps there is a way to reorder the patches that makes it simpler?
>
> On second look, what does __ptrace_may_access() need task_lock() for anyway?

Just for get_dumpable(task->mm), I think.

> > But do we really care? If selinux denies to ptrace this task, can't we
> > return -EACESS regardless of ->ptrace?
>
> You're right that the return code is the same either way.  That's not the
> issue.  The issue is whether this case calls security_ptrace_may_access()
> at all, because doing so can have side effects.  Consider an application
> that does:
>
> 	ptrace(PTRACE_ATTACH, pid);
> 	ptrace(PTRACE_ATTACH, pid);
> 	pid = wait(&status);
>
> It works fine, because it doesn't care that the second call fails.
> (A real-world example would be much more complex, with mounds of poorly
> structured code so nobody can even tell what calls have already been made.
> Maybe the first one would really have been the child doing PTRACE_TRACEME,
> or inheriting via CLONE_PTRACE/PTRACE_O_TRACECLONE, etc.)
>
> After your change, the application still works fine by itself.  But now,
> the second call causes a security_ptrace_may_access() call that wasn't
> there before.  This hits some crazy LSM arrangement we haven't even
> thought of, and produces auditing complaints about improper ptrace
> attempts.  Those log messages on a server trigger some fancy monitoring
> thing that identifies them as unexpected and security-related, decides
> that means they're urgent, and so pages the poor sysadmin and his boss
> and his boss's boss with bright red "BREAK-IN ATTEMPTED IN THE
> DATACENTER" pages vibrating their beltlines right when their hot dates
> were about to get interesting.  The poor sysadmin spends the next month
> of his life in rat holes of wild paranoia and reinstalling, and then
> eventually in tedious explanations of how there was never any intrusion
> but he'd just done an innocuous kernel upgrade that he knew was not
> supposed to change any application behavior.

OK, so this change is not purely cosmetic as I thought.

We can fix this in many ways. We can extract the ->cred and ->mm checks
from __ptrace_may_access() into another helper which is called before
write_lock(tasklist), and then call security_ptrace_may_access under tasklist.
Or we can do get_task_mm() in advance and call __ptrace_may_access() without
task_lock().

Or, perhaps, we can just check ->ptrace before __ptrace_may_access()
lockless (just to prevent the scenario above), and then check it again
under tasklist? This looks like a simplest option.

Oleg.

Re: [PATCH 1/3] ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm

[Roland McGrath]

> > On second look, what does __ptrace_may_access() need task_lock() for anyway?
> 
> Just for get_dumpable(task->mm), I think.

Ah.  That's really only for using the mm, right?  i.e. it could be using
get_task_mm() instead.  set_dumpable() does not use task_lock() to
synchronize the actual changes that affect get_dumpable().

> OK, so this change is not purely cosmetic as I thought.

Right.

> We can fix this in many ways. We can extract the ->cred and ->mm checks
> from __ptrace_may_access() into another helper which is called before
> write_lock(tasklist), and then call security_ptrace_may_access under tasklist.
> Or we can do get_task_mm() in advance and call __ptrace_may_access() without
> task_lock().

get_task_mm() makes sense to me.  It seems like those checks being under
the tasklist_lock (i.e. the lock governing ptrace attach) might matter.

> Or, perhaps, we can just check ->ptrace before __ptrace_may_access()
> lockless (just to prevent the scenario above), and then check it again
> under tasklist? This looks like a simplest option.

Doesn't that still have a possible race with PTRACE_TRACEME?  If
ptrace_traceme() runs between the lockless check and the lock-taking, we
want PTRACE_ATTACH to fail without calling security_ptrace_may_access().
Maybe the cred mutex should be excluding that here anyway.


Thanks,
Roland