Re: [PATCH v18] exec: Fix dead-lock in de_thread with ptrace_attach

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 11/20/25 18:29, Eric W. Biederman wrote:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
> 
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>
>>> This introduces signal->exec_bprm, which is used to
>>> fix the case when at least one of the sibling threads
>>> is traced, and therefore the trace process may dead-lock
>>> in ptrace_attach, but de_thread will need to wait for the
>>> tracer to continue execution.
>>
>> A small quibble it isn't a dead lock.  It isn't even really a live lock,
>> as it is possible to SIGKILL our way out.
>>

That is of course what I meant to say with that sentence.  In my language an
application process can "dead-lock" by waiting on a mutex forever.
Indeed the original problem with the dead-lock (I think in mm_access) was
worse, as both involved processes were only killable by "kill -9", but with the
remaining problem in ptrace_attach, the debugger can be killed by a simple CTRL-C.

So if I understand you right, you want me use a different term here like
"the trace process may be blocked in trace_attach" or so?
Or could you please give me a concrete suggestion how to rephrase the patch
description.

BTW, unless there are objections I would also want to rephrase the description
of cred_guard_mutex to replace the term "Deprecated" with "Not recommended",
like this:

--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -260,11 +260,11 @@ struct signal_struct {
        struct mutex cred_guard_mutex;  /* guard against foreign influences on
                                         * credential calculations
                                         * (notably. ptrace)
                                         * Held while execve runs, except when
                                         * a sibling thread is being traced.
-                                        * Deprecated do not use in new code.
+                                        * Not recommended to use in new code.
                                         * Use exec_update_lock instead.
                                         */


>> Thinking about this there is a really silly and simple way we can deal
>> with this situation for PTRACE_ATTACH.  We can send SIGSTOP and wait for
>> the thread to stop before doing anything with cred_guard_mutex.
>>
>> PTRACE_ATTACH already implies sending SIGSTOP so as long as we have
>> enough permissions to send SIGSTOP I don't see that being a problem.
>>
>> The worst case I can see is that we get a case where we stop the
>> process, the permission check fails under cred_guard_mutex and
>> and ptrace attach has fails and has to send SIGCONT to undo it's
>> premature SIGSTOP.  That might almost be visible, but it would still
>> be legitimate because we can still check that we have permission to
>> send SIGSTOP.
> 
> Bah no I am full of it.
> 
> The challenging behavior is in the semantics of the kernel operations.
> We need to describe it as such please.
> 
> It is the same class of problem as a single threaded process calls exec
> with a pipe attached to both stdin and stdout of the new process.
> 
> For the stdin and stdout we can say just use pull and nonblocking I/O.
> 
> The problem is that both PTRACE_ATTACH and PTRACE_SEIZE block over
> the duration of exec, and if exec is waiting for a thread to exit,
> and that thread is blocked in PTRACE_EVENT_EXIT waiting for that very
> same tracer those processes will hang. Not deadlock.
> 
> 
> I haven't seen anyone clearly describe the problem lately so I am
> repeating it.
> 
> 
> Just looking at the code I don't think there is any fundamental reason
> to call commit_creds after de_thread.  If we can change that we can sort
> this out without any change in userspace semantics.
> 
> If we can't move commit_creds we have to either give
> PTRACE_ATTACH/PTRACE_SEIZE a non-block mode, or break out of
> PTRACE_EVENT_EXIT in de_thread.
> 
> I will post a proof of concept of moving commit_creds in just a minute.
> 
> Eric

Note: I forgot to add apparmor and selinux mailing list to this patch, previous
versions of this did try to avoid to touch the security engine code, and did
instead temporarily install the new credentials, mostiy for the benefit of the
security engines.  But that is considered an unacceptable solution, therefore
I want to use instead a new option to ptrace_may_access.
All security engines have to handle this option, but the advantage is, that the
engines could detect and maybe also deny the unsafe execve.

This is an alternative to Eric's patch: "exec: Move cred computation under
exec_update_lock" that is supposed to solve the same problem, but tries instead
to avoid any user visible API change.


Thanks
Bernd.