Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

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.

index 0316200..43aa09c 100644
@@ -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.

Hello,

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

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

tejun

(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.)

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

the post-execve SIGTRAP, so I don't believe that race actually exists. Or is that not the case?

Pedro Alves

Yes.

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

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.

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

Oleg.

Hello,

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.

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

Thanks.

tejun

Hello,

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.

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.

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