[v3,1/8] exec: introduce cred_guard_light

Commit Message

Eric W. Biederman Nov. 4, 2016, 1:26 p.m. UTC

Oleg Nesterov <[e-mail]> writes:

> On 11/02, Jann Horn wrote:
>>
>> On Wed, Nov 02, 2016 at 07:18:06PM +0100, Oleg Nesterov wrote:
>> > On 10/30, Jann Horn wrote:
>> > >
>> > > This is a new per-threadgroup lock that can often be taken instead of
>> > > cred_guard_mutex and has less deadlock potential. I'm doing this because
>> > > Oleg Nesterov mentioned the potential for deadlocks, in particular if a
>> > > debugged task is stuck in execve, trying to get rid of a ptrace-stopped
>> > > thread, and the debugger attempts to inspect procfs files of the debugged
>> > > task.
>> >
>> > Yes, but let me repeat that we need to fix this anyway. So I don't really
>> > understand why should we add yet another mutex.
>>
>> execve() only takes the new mutex immediately after de_thread(), so this
>> problem shouldn't occur there.
>
> Yes, I see.
>
>> Basically, I think that I'm not making the
>> problem worse with my patches this way.
>
> In a sense that it doesn't add the new deadlocks, I agree. But it adds
> yet another per-process mutex while we already have the similar one,
>
>> I believe that it should be possible to convert most existing users of the
>> cred_guard_mutex to the new cred_guard_light - exceptions to that that I
>> see are:
>>
>>  - PTRACE_ATTACH
>
> This is the main problem afaics. So "strace -f" can hang if it races
> with mt-exec. And we need to fix this. I constantly forget about this
> problem, but I tried many times to find a reasonable solution, still
> can't.
>
> IMO, it would be nice to rework the lsm hooks, so that we could take
> cred_guard_mutex after de_thread() (like your cred_guard_light) or
> at least drop it earlier, but unlikely this is possible...
>
> So the only plan I currently have is change de_thread() to wait until
> other threads pass exit_notify() or even exit_signals(), but I don't
> like this.
>
>>  - SECCOMP_FILTER_FLAG_TSYNC (sets NO_NEW_PRIVS on remote task)
>
> I forgot about this one... Need to re-check but at first glance this
> is not a real problem.
>
>> Beyond that, conceptually, the new cred_guard_light could also be turned
>> into a read-write mutex
>
> Not sure I understand how this can help... doesn't matter.
>
> My point is, imo you should not add the new mutex. Just use the old
> one in (say) 4/8 (which I do not personally like as you know ;), this
> won't add the new problem.
>
>
>> It seems to me like SECCOMP_FILTER_FLAG_TSYNC doesn't really have
>> deadlocking issues.
>
> Yes, agreed.
>
>> PTRACE_ATTACH isn't that clear to me; if a debugger
>> tries to attach to a newly spawned thread while another ptraced thread is
>> dying because of de_thread() in a third thread, that might still cause
>> the debugger to deadlock, right?
>
> This is the trivial test-case I wrote when the problem was initially
> reported. And damn, I always knew that cred_guard_mutex needs fixes,
> but somehow I completely forgot that it is used by PTRACE_ATTACH when
> I was going to try to remove from fs/proc a long ago.
>
> 	void *thread(void *arg)
> 	{
> 		ptrace(PTRACE_TRACEME, 0,0,0);
> 		return NULL;
> 	}
>
> 	int main(void)
> 	{
> 		int pid = fork();
>
> 		if (!pid) {
> 			pthread_t pt;
> 			pthread_create(&pt, NULL, thread, NULL);
> 			pthread_join(pt, NULL);
> 			execlp("echo", "echo", "passed", NULL);
> 		}
>
> 		sleep(1);
> 		// or anything else which needs ->cred_guard_mutex,
> 		// say open(/proc/$pid/mem)
> 		ptrace(PTRACE_ATTACH, pid, 0,0);
> 		kill(pid, SIGCONT);
>
> 		return 0;
> 	}
>
> The problem is trivial. The execing thread waits until its sub-thread
> goes away, it should be reaped by the tracer, the tracer waits for
> cred_guard_mutex.

There is a bug here but I don't believe it has anything to do with
the cred_guard_mutex.

If we reach zap_other_threads fundamentally the tracer should not
be able to block the traced thread from exiting.  Those are the
semantics described in the comments in the code.

I have poked things a little and have a half fix for that but
the fix appears to be the wrong, but enlightening.

AKA the following prevents the hang of your test case.

It looks like somewhere on the exit path the traced thread is blocking
without setting TASK_WAKEKILL.

Eric


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/kernel/signal.c b/kernel/signal.c
index 75761acc77cf..a6f83450500e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1200,7 +1200,7 @@  int zap_other_threads(struct task_struct *p)
 		if (t->exit_state)
 			continue;
 		sigaddset(&t->pending.signal, SIGKILL);
-		signal_wake_up(t, 1);
+		signal_wake_up_state(t, TASK_WAKEKILL | __TASK_TRACED);
 	}
 
 	return count;