The wake_up_process() call in ptrace_detach() is spurious and not
interlocked with the tracee state. IOW, the tracee could be running
or sleeping in any place in the kernel by the time wake_up_process()
is called. This can lead to the tracee waking up unexpectedly which
can be dangerous.
The wake_up is spurious and should be removed but for now reduce its
toxicity by only waking up if the tracee is in TRACED or STOPPED
state.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Roland McGrath <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: [email protected]
---
So, something like this. Roland, Oleg, can you guys please ack this?
Also, should these ptrace patches be routed? Shall I set up a git
tree?
Thank you.
kernel/ptrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: work/kernel/ptrace.c
===================================================================
--- work.orig/kernel/ptrace.c
+++ work/kernel/ptrace.c
@@ -313,7 +313,7 @@ int ptrace_detach(struct task_struct *ch
child->exit_code = data;
dead = __ptrace_detach(current, child);
if (!child->exit_state)
- wake_up_process(child);
+ wake_up_state(child, TASK_TRACED | TASK_STOPPED);
}
write_unlock_irq(&tasklist_lock);
On 02/01, Tejun Heo wrote:
>
> --- work.orig/kernel/ptrace.c
> +++ work/kernel/ptrace.c
> @@ -313,7 +313,7 @@ int ptrace_detach(struct task_struct *ch
> child->exit_code = data;
> dead = __ptrace_detach(current, child);
> if (!child->exit_state)
> - wake_up_process(child);
> + wake_up_state(child, TASK_TRACED | TASK_STOPPED);
Well, it can't be TASK_TRACED at this point. And of course this still
contradicts to __set_task_state(child, TASK_STOPPED) in ptrace_untrace().
IOW, to me the previous patch makes more sense.
But OK, I understand Roland's concerns. And, at least this change
fixes the bug mentioned in 95a3540d.
Acked-by: Oleg Nesterov <[email protected]>
Hello,
On Tue, Feb 01, 2011 at 02:40:37PM +0100, Oleg Nesterov wrote:
> On 02/01, Tejun Heo wrote:
> >
> > --- work.orig/kernel/ptrace.c
> > +++ work/kernel/ptrace.c
> > @@ -313,7 +313,7 @@ int ptrace_detach(struct task_struct *ch
> > child->exit_code = data;
> > dead = __ptrace_detach(current, child);
> > if (!child->exit_state)
> > - wake_up_process(child);
> > + wake_up_state(child, TASK_TRACED | TASK_STOPPED);
>
> Well, it can't be TASK_TRACED at this point. And of course this still
> contradicts to __set_task_state(child, TASK_STOPPED) in ptrace_untrace().
> IOW, to me the previous patch makes more sense.
Yeah, it can't be in TRACED but the whole point of the patch is
avoiding rude wakeups, so as long as it doesn't end up waking random
[un]interruptible sleeps... It will be removed later anyway.
> But OK, I understand Roland's concerns. And, at least this change
> fixes the bug mentioned in 95a3540d.
>
> Acked-by: Oleg Nesterov <[email protected]>
Oleg, Roland, you guys are the maintainers, so how do you guys want to
route the patches which have been acked? As it's likely that there
will be quite some number of ptrace patches, it will be better to have
a git tree.
Thank you.
--
tejun
On 02/01, Tejun Heo wrote:
>
> Hello,
>
> On Tue, Feb 01, 2011 at 02:40:37PM +0100, Oleg Nesterov wrote:
> > On 02/01, Tejun Heo wrote:
> > >
> > > --- work.orig/kernel/ptrace.c
> > > +++ work/kernel/ptrace.c
> > > @@ -313,7 +313,7 @@ int ptrace_detach(struct task_struct *ch
> > > child->exit_code = data;
> > > dead = __ptrace_detach(current, child);
> > > if (!child->exit_state)
> > > - wake_up_process(child);
> > > + wake_up_state(child, TASK_TRACED | TASK_STOPPED);
> >
> > Well, it can't be TASK_TRACED at this point. And of course this still
> > contradicts to __set_task_state(child, TASK_STOPPED) in ptrace_untrace().
> > IOW, to me the previous patch makes more sense.
>
> Yeah, it can't be in TRACED but the whole point of the patch is
> avoiding rude wakeups, so as long as it doesn't end up waking random
> [un]interruptible sleeps... It will be removed later anyway.
Yes, yes, I understand.
> > But OK, I understand Roland's concerns. And, at least this change
> > fixes the bug mentioned in 95a3540d.
> >
> > Acked-by: Oleg Nesterov <[email protected]>
>
> Oleg, Roland, you guys are the maintainers, so how do you guys want to
> route the patches which have been acked?
Well. I know only one way, send it to akpm ;)
> As it's likely that there
> will be quite some number of ptrace patches, it will be better to have
> a git tree.
Probably yes... but everything in this area goes through -mm so far.
Oleg.
On Tue, 1 Feb 2011 11:26:18 +0100
Tejun Heo <[email protected]> wrote:
> The wake_up_process() call in ptrace_detach() is spurious and not
> interlocked with the tracee state. IOW, the tracee could be running
> or sleeping in any place in the kernel by the time wake_up_process()
> is called. This can lead to the tracee waking up unexpectedly which
> can be dangerous.
>
> The wake_up is spurious and should be removed but for now reduce its
> toxicity by only waking up if the tracee is in TRACED or STOPPED
> state.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Cc: Roland McGrath <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: [email protected]
Am unable to work out why you tagged it for backporting. It fixes some
observed bug? Perhaps a regression?
> Index: work/kernel/ptrace.c
> ===================================================================
> --- work.orig/kernel/ptrace.c
> +++ work/kernel/ptrace.c
> @@ -313,7 +313,7 @@ int ptrace_detach(struct task_struct *ch
> child->exit_code = data;
> dead = __ptrace_detach(current, child);
> if (!child->exit_state)
> - wake_up_process(child);
> + wake_up_state(child, TASK_TRACED | TASK_STOPPED);
> }
> write_unlock_irq(&tasklist_lock);
>
Acked-by: Roland McGrath <[email protected]>
> Oleg, Roland, you guys are the maintainers, so how do you guys want to
> route the patches which have been acked? As it's likely that there
> will be quite some number of ptrace patches, it will be better to have
> a git tree.
In practice we are never trusted enough with changes in this area to get
them just pulled from a tree of ours. They have to go through akpm and
Linus for individual approval before they get in. So I see no point in me
or Oleg maintaining a git tree that won't get merged directly anyway.
Thanks,
Roland
> Am unable to work out why you tagged it for backporting. It fixes some
> observed bug? Perhaps a regression?
No observed bug, only theoretical ones (AFAIK, never even a ginned-up
synthetic test case has been demonstrated). Certainly not a regression,
since it has been this (wrong) way since the dawn of time. I don't think
this first change is dangerous for -stable, but I have seen no positive
rationale for pushing it there.
Thanks,
Roland
On Tue, 1 Feb 2011 21:33:31 -0800 (PST) Roland McGrath <[email protected]> wrote:
> > Am unable to work out why you tagged it for backporting. It fixes some
> > observed bug? Perhaps a regression?
>
> No observed bug, only theoretical ones (AFAIK, never even a ginned-up
> synthetic test case has been demonstrated). Certainly not a regression,
> since it has been this (wrong) way since the dawn of time. I don't think
> this first change is dangerous for -stable, but I have seen no positive
> rationale for pushing it there.
>
OK, thanks. I shall destabilize my copy of this patch.
Hello,
On Tue, Feb 01, 2011 at 09:38:28PM -0800, Andrew Morton wrote:
> On Tue, 1 Feb 2011 21:33:31 -0800 (PST) Roland McGrath <[email protected]> wrote:
>
> > > Am unable to work out why you tagged it for backporting. It fixes some
> > > observed bug? Perhaps a regression?
> >
> > No observed bug, only theoretical ones (AFAIK, never even a ginned-up
> > synthetic test case has been demonstrated). Certainly not a regression,
> > since it has been this (wrong) way since the dawn of time. I don't think
> > this first change is dangerous for -stable, but I have seen no positive
> > rationale for pushing it there.
> >
>
> OK, thanks. I shall destabilize my copy of this patch.
It can be used as an attack vector. I don't think it will take too
much effort to come up with an attack which triggers oops somewhere.
Most sleeps are wrapped in condition test loops and should be safe but
we have quite a number of places where sleep and wakeup conditions are
expected to be interlocked. Although the window of opportunity is
tiny, ptrace can be used by non-privileged users and with some loading
the window can definitely be extended and exploited.
The chance of this problem being visible under normal usage is
extremely low so no wonder there is no related bug report but that is
very different from being safe against targeted attacks.
As the likelihood of causing user noticeable breakage is very low, I
think we better push it through -stable.
Thanks.
--
tejun
Hello,
On Tue, Feb 01, 2011 at 09:31:44PM -0800, Roland McGrath wrote:
> > Oleg, Roland, you guys are the maintainers, so how do you guys want to
> > route the patches which have been acked? As it's likely that there
> > will be quite some number of ptrace patches, it will be better to have
> > a git tree.
>
> In practice we are never trusted enough with changes in this area to get
> them just pulled from a tree of ours. They have to go through akpm and
> Linus for individual approval before they get in. So I see no point in me
> or Oleg maintaining a git tree that won't get merged directly anyway.
Hmm... okay. Alright, through -mm then.
Thanks.
--
tejun
On Wed, 2 Feb 2011 11:34:02 +0100
Tejun Heo <[email protected]> wrote:
> Hello,
>
> On Tue, Feb 01, 2011 at 09:38:28PM -0800, Andrew Morton wrote:
> > On Tue, 1 Feb 2011 21:33:31 -0800 (PST) Roland McGrath <[email protected]> wrote:
> >
> > > > Am unable to work out why you tagged it for backporting. It fixes some
> > > > observed bug? Perhaps a regression?
> > >
> > > No observed bug, only theoretical ones (AFAIK, never even a ginned-up
> > > synthetic test case has been demonstrated). Certainly not a regression,
> > > since it has been this (wrong) way since the dawn of time. I don't think
> > > this first change is dangerous for -stable, but I have seen no positive
> > > rationale for pushing it there.
> > >
> >
> > OK, thanks. I shall destabilize my copy of this patch.
>
> It can be used as an attack vector. I don't think it will take too
> much effort to come up with an attack which triggers oops somewhere.
> Most sleeps are wrapped in condition test loops and should be safe but
> we have quite a number of places where sleep and wakeup conditions are
> expected to be interlocked. Although the window of opportunity is
> tiny, ptrace can be used by non-privileged users and with some loading
> the window can definitely be extended and exploited.
>
> The chance of this problem being visible under normal usage is
> extremely low so no wonder there is no related bug report but that is
> very different from being safe against targeted attacks.
>
> As the likelihood of causing user noticeable breakage is very low, I
> think we better push it through -stable.
>
We're learning some lessons about changelogging here :(
I added this:
: This bug can possibly be used as an attack vector. I don't think
: it will take too much effort to come up with an attack which triggers
: oops somewhere. Most sleeps are wrapped in condition test loops and
: should be safe but we have quite a number of places where sleep and
: wakeup conditions are expected to be interlocked. Although the
: window of opportunity is tiny, ptrace can be used by non-privileged
: users and with some loading the window can definitely be extended and
: exploited.
to the changelog so the -stable maintainers can understand why we're
sending this patch at them.
Hello,
On Wed, Feb 2, 2011 at 8:33 PM, Andrew Morton <[email protected]> wrote:
> We're learning some lessons about changelogging here :(
>
> I added this:
I was aiming for being a bit less explicit, which may or may not have
been a good idea. Anyways, sounds good to me.
Thanks.
--
tejun
On 02/01, Roland McGrath wrote:
>
> > Am unable to work out why you tagged it for backporting. It fixes some
> > observed bug? Perhaps a regression?
>
> No observed bug,
Well, this bug triggered the problem in practice, this was the reason
for 95a3540d.
> I don't think
> this first change is dangerous for -stable, but I have seen no positive
> rationale for pushing it there.
Agreed, I don't really think -stable needs this change, but otoh
it is obviously safe.
Oleg.