[PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Denys Vlasenko]

Make PTRACE_SEIZE set ptrace options specified in 'data' parameter
    
This can be used to close a few corner cases in strace where we get
unwanted racy behavior after attach, but before we have a chance
to set options (the notorious post-execve SIGTRAP comes to mind),
and removes the need to track "did we set opts for this task" state
in strace internals.
    
While we are at it:
    
Make it possible to extend SEIZE in the future with more functionality
by passing non-zero 'addr' parameter.
To that end, error out if 'addr' is non-zero.
PTRACE_ATTACH did not (and still does not) have such check,
and users (strace) do pass garbage there... let's avoid repeating
this mistake with SEIZE.
    
Set all task->ptrace bits in one operation - before this change,
we were adding PT_SEIZED and PT_PTRACE_CAP with task->ptrace |= BIT.
This was probably ok (not a bug), but let's be on a safer side.
    
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 0316200..43aa09c 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -212,6 +212,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
 }
 
 static int ptrace_attach(struct task_struct *task, long request,
+			 unsigned long addr,
 			 unsigned long flags)
 {
 	bool seize = (request == PTRACE_SEIZE);
@@ -219,19 +220,27 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 	/*
 	 * SEIZE will enable new ptrace behaviors which will be implemented
-	 * gradually.  SEIZE_DEVEL is used to prevent applications
+	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
 	 * expecting full SEIZE behaviors trapping on kernel commits which
 	 * are still in the process of implementing them.
 	 *
 	 * Only test programs for new ptrace behaviors being implemented
 	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
 	 *
-	 * Once SEIZE behaviors are completely implemented, this flag and
-	 * the following test will be removed.
+	 * Once SEIZE behaviors are completely implemented, this flag
+	 * will be removed.
 	 */
 	retval = -EIO;
-	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
-		goto out;
+	if (seize) {
+		if (addr != 0)
+			goto out;
+		if ((flags & ~(long)PTRACE_O_MASK) != PTRACE_SEIZE_DEVEL)
+			goto out;
+		flags &= ~PTRACE_SEIZE_DEVEL;
+		flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT);
+	} else {
+		flags = PT_PTRACED;
+	}
 
 	audit_ptrace(task);
 
@@ -263,11 +272,9 @@ static int ptrace_attach(struct task_struct *task, long request,
 	if (task->ptrace)
 		goto unlock_tasklist;
 
-	task->ptrace = PT_PTRACED;
-	if (seize)
-		task->ptrace |= PT_SEIZED;
 	if (task_ns_capable(task, CAP_SYS_PTRACE))
-		task->ptrace |= PT_PTRACE_CAP;
+		flags |= PT_PTRACE_CAP;
+	task->ptrace = flags;
 
 	__ptrace_link(task, current);
 
@@ -865,7 +872,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 	}
 
 	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
-		ret = ptrace_attach(child, request, data);
+		ret = ptrace_attach(child, request, addr, data);
 		/*
 		 * Some architectures need to do book-keeping after
 		 * a ptrace attach.

Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Pedro Alves]

On Wednesday 07 September 2011 22:40:31, Denys Vlasenko wrote:
> Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

(I wonder if it'd be a good idea to be more future proof and pass in
a pointer to a `struct { int my_size; int options; }', so we can
add more things in the future other than option bits, if we need to.)

> This can be used to close a few corner cases in strace where we get
> unwanted racy behavior after attach, but before we have a chance
> to set options (the notorious post-execve SIGTRAP comes to mind),

(irrespective of the patch being a good idea or not)

>From previous discussions, I understood that PTRACE_SEIZE _always_ disables
the post-execve SIGTRAP, so I don't believe that race actually exists.
Or is that not the case?

-- 
Pedro Alves

Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Tejun Heo]

Hello,

On Wed, Sep 07, 2011 at 11:40:31PM +0200, Denys Vlasenko wrote:
> +	if (seize) {
> +		if (addr != 0)
> +			goto out;
> +		if ((flags & ~(long)PTRACE_O_MASK) != PTRACE_SEIZE_DEVEL)

Please use (unsigned long).  Also, wouldn't it be better to do the
following instead?

	if (!(flags & PTRACE_SEIZE_DEVEL))
		goto out;
	flags &= ~PTRACE_SEIZE_DEVEL;

	if ((flags & ~(unsigned long(PTRACE_O_MASK))))
		goto out;

Then, we can later just delete the first three lines when removing
SEIZE_DEVEL.

> @@ -263,11 +272,9 @@ static int ptrace_attach(struct task_struct *task, long request,
>  	if (task->ptrace)
>  		goto unlock_tasklist;
>  
> -	task->ptrace = PT_PTRACED;
> -	if (seize)
> -		task->ptrace |= PT_SEIZED;
>  	if (task_ns_capable(task, CAP_SYS_PTRACE))
> -		task->ptrace |= PT_PTRACE_CAP;
> +		flags |= PT_PTRACE_CAP;
> +	task->ptrace = flags;

Can you please put this in a separate patch?  Hmm... also I think we
probably want to set ->ptrace while holding siglock too.  There are
places which assume ->ptrace is protected by siglock.  We can move
siglock locking above so that both ->ptrace setting and linking are
protected by siglock and use send_signal() instead of send_sig_info()
for the implied SIGSTOP.  Note that __ptrace_unlink() would need
similar update too.

Thank you.

-- 
tejun

Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Oleg Nesterov]

On 09/08, Tejun Heo wrote:
>
> On Wed, Sep 07, 2011 at 11:40:31PM +0200, Denys Vlasenko wrote:
> > +	if (seize) {
> > +		if (addr != 0)
> > +			goto out;
> > +		if ((flags & ~(long)PTRACE_O_MASK) != PTRACE_SEIZE_DEVEL)
>
> Please use (unsigned long).  Also, wouldn't it be better to do the
> following instead?
>
> 	if (!(flags & PTRACE_SEIZE_DEVEL))
> 		goto out;
> 	flags &= ~PTRACE_SEIZE_DEVEL;
>
> 	if ((flags & ~(unsigned long(PTRACE_O_MASK))))
> 		goto out;
>
> Then, we can later just delete the first three lines when removing
> SEIZE_DEVEL.
>
> > @@ -263,11 +272,9 @@ static int ptrace_attach(struct task_struct *task, long request,
> >  	if (task->ptrace)
> >  		goto unlock_tasklist;
> >
> > -	task->ptrace = PT_PTRACED;
> > -	if (seize)
> > -		task->ptrace |= PT_SEIZED;
> >  	if (task_ns_capable(task, CAP_SYS_PTRACE))
> > -		task->ptrace |= PT_PTRACE_CAP;
> > +		flags |= PT_PTRACE_CAP;
> > +	task->ptrace = flags;
>
> Can you please put this in a separate patch?

Yes.

> Hmm... also I think we
> probably want to set ->ptrace while holding siglock too.

I thought about this too, and I agree this makes sense

> There are
> places which assume ->ptrace is protected by siglock.

Really? Once again, I agree. But _afaics_ currently this is not strictly
needed. PT_PTRACED/PT_SEIZED should not go away under ->siglock, yes, but
it seems that it is fine to set them.

> and linking are
> protected by siglock

Hmm. Could you explain this? Why do want __ptrace_link under ->siglock ?

Oleg.

Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Tejun Heo]

Hello,

On Thu, Sep 08, 2011 at 08:17:47PM +0200, Oleg Nesterov wrote:
> > There are places which assume ->ptrace is protected by siglock.
> 
> Really? Once again, I agree. But _afaics_ currently this is not strictly
> needed. PT_PTRACED/PT_SEIZED should not go away under ->siglock, yes, but
> it seems that it is fine to set them.

Hmmm.... I haven't checked each direction.  Maybe we don't strictly
need it on setting it but I definitely was assuming that ->ptrace was
protected by siglock while coding recent ptrace changes.  Can't the
following happen?

* ptracer seizes child, sets PT_PTRACED and then OR PT_SEIZED.

* ptracee enters signal delivery path with group stop scheduled.
  PT_PTRACED is visible and group stop is transformed into
  JOBCTL_TRAP_STOP.

* ptracee enters do_jobct_trap().  PT_SEIZED is still not visible and
  it takes the path for the old behavior.

* ptracer SEIZE'd and expects PTRACE_EVENT_STOP but it gets the old
  no-siginfo trap.

> > and linking are protected by siglock
> 
> Hmm. Could you explain this? Why do want __ptrace_link under ->siglock ?

Because it's much simpler to assume that w/ siglock locked, everything
including ->parent is set up properly w.r.t. ->ptrace.

Thanks.

-- 
tejun

Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Oleg Nesterov]

Hello,

On 09/11, Tejun Heo wrote:
>
> On Thu, Sep 08, 2011 at 08:17:47PM +0200, Oleg Nesterov wrote:
> > > There are places which assume ->ptrace is protected by siglock.
> >
> > Really? Once again, I agree. But _afaics_ currently this is not strictly
> > needed. PT_PTRACED/PT_SEIZED should not go away under ->siglock, yes, but
> > it seems that it is fine to set them.
>
> Hmmm.... I haven't checked each direction.  Maybe we don't strictly
> need it on setting it but I definitely was assuming that ->ptrace was
> protected by siglock while coding recent ptrace changes.  Can't the
> following happen?
>
> * ptracer seizes child, sets PT_PTRACED and then OR PT_SEIZED.
>
> * ptracee enters signal delivery path with group stop scheduled.
>   PT_PTRACED is visible and group stop is transformed into
>   JOBCTL_TRAP_STOP.
>
> * ptracee enters do_jobct_trap().  PT_SEIZED is still not visible and
>   it takes the path for the old behavior.
>
> * ptracer SEIZE'd and expects PTRACE_EVENT_STOP but it gets the old
>   no-siginfo trap.

Heh ;) Please look at http://marc.info/?l=linux-kernel&m=131541614232539

	> @@ -263,7 +267,7 @@ static int ptrace_attach(struct task_struct *task, long request,
	>  	if (task->ptrace)
	>  		goto unlock_tasklist;
	>
	> -	task->ptrace = PT_PTRACED;
	> +	task->ptrace = PT_PTRACED | (flags << PT_OPT_FLAG_SHIFT);
	>  	if (seize)
	>  		task->ptrace |= PT_SEIZED;

	Hmm. Tejun, Denys, this doesn't look exactly right.

	I already thought about this before, but somehow I convinced myself
	this is fine.

	I think we should set both PT_PTRACED | PT_SEIZED "atomically", at
	once. Otherwise, say, the tracee can do do_jobctl_trap() in between,
	no? Nothing really bad can happen, but we shouldn't lose EVENT_STOP
	code.

Yes, we need to set them both at once.

And yes, I agree, it is better to do this under ->siglock even if currently
this is not strictly necessary.

> > > and linking are protected by siglock
> >
> > Hmm. Could you explain this? Why do want __ptrace_link under ->siglock ?
>
> Because it's much simpler to assume that w/ siglock locked, everything
> including ->parent is set up properly w.r.t. ->ptrace.

Well, but then we shouldn't rely on tracee's ->siglock. The tracee simply
do not care about its ->ptrace_entry, only the tracer does.

We need to rework the locking, yes. But we need the lock which protects
the parent's list_head (currently we rely on tasklist). Yes, a single
lock can't help. We already use ->cred_guard_mutex though.

This needs more thinking, but imho child->siglock is pointless here.

Oleg.

Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Tejun Heo]

On Sun, Sep 11, 2011 at 08:14:42PM +0200, Oleg Nesterov wrote:
> We need to rework the locking, yes. But we need the lock which protects
> the parent's list_head (currently we rely on tasklist). Yes, a single
> lock can't help. We already use ->cred_guard_mutex though.
> 
> This needs more thinking, but imho child->siglock is pointless here.

But we can at least guarantee that if ->ptrace is set (or clear) while
tracee's siglock is held, its ->parent points to the tracer (or not).
At any rate, AFAICS, this currently doesn't really matter.  I
suggested it mainly because it would make the locking change easier.

Thanks.

-- 
tejun