On 11/24/25 00:22, Eric W. Biederman wrote:
Oleg Nesterov oleg@redhat.com writes:
Eric,
sorry for delay, I am on PTO, didn't read emails this week...
On 11/20, Eric W. Biederman wrote:
Instead of computing the new cred before we pass the point of no return compute the new cred just before we use it.
This allows the removal of fs_struct->in_exec and cred_guard_mutex.
I am not certain why we wanted to compute the cred for the new executable so early. Perhaps I missed something but I did not see any common errors being signaled. So I don't think we loose anything by computing the new cred later.
We gain a lot.
Yes. I LIKE your approach after a quick glance. And I swear, I thought about it too ;)
But is it correct? I don't know. I'll try to actually read your patch next week (I am on PTO untill the end of November), but I am not sure I can provide a valuable feedback.
One "obvious" problem is that, after this patch, the execing process can crash in a case when currently exec() returns an error...
Yes.
I have been testing and looking at it, and I have found a few issues, and I am trying to see if I can resolve them.
The good news is that with the advent of AT_EXECVE_CHECK we have a really clear API boundary between errors that must be diagnosed and errors of happenstance like running out of memory.
The bad news is that the implementation of AT_EXECVE_CHECK seems to been rather hackish especially with respect to security_bprm_creds_for_exec.
What I am hoping for is to get the 3 causes of errors of brpm->unsafe ( LSM_UNSAFE_SHARE, LSM_UNSAFE_PTRACE, and LSM_UNSAFE_NO_NEW_PRIVS ) handled cleanly outside of the cred_guard_mutex, and simply retested when it is time to build the credentials of the new process.
In practice that should get the same failures modes as we have now but it would get SIGSEGV in rare instances where things changed during exec. That feels acceptable.
I thought of one other approach that might be enough to put the issue to bed if cleaning up exec is too much work. We could have ptrace_attach use a trylock and fail when it doesn't succeed. That would solve the worst of the symptoms.
I think this would be a complete patch:
diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 75a84efad40f..5dd2144e5789 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -444,7 +444,7 @@ static int ptrace_attach(struct task_struct *task, long request, * SUID, SGID and LSM creds get determined differently * under ptrace. */
- scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
- scoped_cond_guard (mutex_try, return -EAGAIN, &task->signal->cred_guard_mutex) {
scoped_guard (task_lock, task) {
This is very similar to my initial attempt of fixing the problem, as you can see the test expectaion of the currently failing test in vmattach.c is that ptrace(PTRACE_ATTACH, pid, 0L, 0L) returns -1 with errno = EAGAIN.
The disadvantage of that approach was, that it is a user-visible API-change, but also that the debugger does not know when to retry the PTRACE_ATTACH, in worst case it will go into an endless loop not knowing that a waitpid and/or PTRACE_CONT is necessary to unblock the traced process.
But The main reason why I preferred the overlapping lifetime of the current and the new credentials, is that the tracee can escape the PTRACE_ATTACH if it is very short-lived, and indeed I had to cheat a little to make the test case function TEST(attach) pass reliably:
The traced process does execlp("sleep", "sleep", "2", NULL);
If it did execlp("true", "true", NULL); like the first test case, it would have failed randomly, because the debugger could not attach quickly enoguh, and IMHO the expectaion of the debugger is probably to be able to stop the new process at the first instruction after the execve.
Bernd.