LWN.net Logo

[PATCH 0/4] (Was: ptrace: prevent PTRACE_SETREGS from corrupting stack)

From:  Oleg Nesterov <oleg-AT-redhat.com>
To:  Linus Torvalds <torvalds-AT-linux-foundation.org>
Subject:  [PATCH 0/4] (Was: ptrace: prevent PTRACE_SETREGS from corrupting stack)
Date:  Sun, 20 Jan 2013 20:24:48 +0100
Message-ID:  <20130120192448.GA6771@redhat.com>
Cc:  Dan Carpenter <dan.carpenter-AT-oracle.com>, Kernel Security <security-AT-kernel.org>, Michael Davidson <md-AT-google.com>, Suleiman Souhlal <suleiman-AT-google.com>, Julien Tinnes <jln-AT-google.com>, Aaron Durbin <adurbin-AT-google.com>, Andrew Morton <akpm-AT-linux-foundation.org>, linux-kernel-AT-vger.kernel.org, Tejun Heo <tj-AT-kernel.org>, Roland McGrath <roland-AT-hack.frob.com>, Tony Luck <tony.luck-AT-intel.com>, Fenghua Yu <fenghua.yu-AT-intel.com>, Greg Kroah-Hartman <gregkh-AT-linuxfoundation.org>
Archive-link:  Article, Thread

add lkml/cc's.

On 01/18, Linus Torvalds wrote:
>
> On Fri, Jan 18, 2013 at 10:55 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Or we can do this after wait_task_inactive() but then we need to take
> > ->siglock again.
>
> Yes. We absolutely need siglock, since that would be exactly what
> would protect us against signal_wake_up() (which is, I *think* the
> only thing that can ever wake up the TASK_TRACED/WAKEKILL cases).

Yes. And thus 4/4 probably should be 1/4.

> And we'd need to make sure to re-set the WAKEKILL flag not just in all
> the callers of ptrace_check_attach(), but also in the failure case of
> wait_task_inactive(). I'm not sure it can actually fail if we cleared
> WAKEKILL, but it's all pretty subtle.

Afaics it can't fail if we clear WAKEKILL... So 2/4 assumes it should
always succeed and adds the warning.

> And when we *do* set the WAKEKILL bit again, we should make sure to
> wake the task in case the killable signal happened while it was clear.

Yes, yes, this is clear. And we need to ensure we can not race with
attach-after-detach...

> And I agree that this is all pretty scary and generally playing with
> another process' 'flags' field is some really nasty business. So I'm a
> bit worried about it.

Oh yes. And I was going to argue that (a much simpler) change which
doesn't allow the tracee to return from ptrace_stop() is better. But
then I recalled about set_task_blockstep() and changed my mind (see
the changelog in 2/4).

Greg, this doesn't look like -stable material. But please let me know
if you think 2/4 should be backported. With a couple of simple hacks
in PTRACE_DETACH/LISTEN paths we can do this without 1/4 and without
changes outside of ptrace.c. But again, probably we shouldn't do this.

Please review.

Oleg.



(Log in to post comments)

Copyright © 2013, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds