Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
On 11/24/25 00:22, Eric W. Biederman wrote:
> Oleg Nesterov 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.
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
Oleg Nesterov 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) {
--
2.41.0
Eric
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
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... Oleg.
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
Bernd Edlinger writes: > On 11/21/25 10:35, Bernd Edlinger wrote: >> On 11/21/25 08:18, Eric W. Biederman wrote: >>> Bernd Edlinger writes: >>> Hi Eric, thanks for you valuable input on the topic. On 11/21/25 00:50, Eric W. Biederman wrote: > "Eric W. Biederman" writes: > >> 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. > > I should add that the permission checks happen in open_exec, > everything that follows credential wise is just about representing in > struct cred the credentials the new executable will have. > > So I am really at a loss why we have had this complicated way of > computing of computed the credentials all of these years full of > time of check to time of use problems. > Well, I think I see a problem with your patch: When the security engine gets the LSM_UNSAFE_PTRACE flag, it might e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux or the smack security engines at least. Previously that callback was called before the point of no return, and the return code should be returned as a return code the the caller of execve. But if we move that check after the point of no return, the caller will get killed due to the failed security check. Or did I miss something? >>> >>> I think we definitely need to document this change in behavior. I would >>> call ending the exec with SIGSEGV vs -EPERM a quality of implementation >>> issue. The exec is failing one way or the other so I don't see it as a >>> correctness issue. >>> >>> In the case of ptrace in general I think it is a bug if the mere act of >>> debugging a program changes it's behavior. So which buggy behavior >>> should we prefer? SIGSEGV where it is totally clear that the behavior >>> has changed or -EPERM and ask the debugged program to handle it. >>> I lean towards SIGSEGV because then it is clear the code should not >>> handle it. >>> >>> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to >>> handle unexpected things happening is to terminate the application. >>> >>> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know >>> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside >>> of CLONE_THREAD. >>> >>> >>> Plus all of these things are only considerations if we are exec'ing a >>> program that transitions to a different set of credentials. Something >>> that happens but is quite rare itself. >>> >>> In practice I don't expect there is anything that depends on the exact >>> behavior of what happens when exec'ing a suid executable to gain >>> privileges when ptraced. The closes I can imagine is upstart and >>> I think upstart ran as root when ptracing other programs so there is no >>> gaining of privilege and thus no reason for a security module to >>> complain. >>> >>> Who knows I could be wrong, and someone could actually care. Which is >>> hy I think we should document it.>> >> >> >> Well, I dont know for sure, but the security engine could deny the execution >> for any reason, not only because of being ptraced. >> Maybe there can be a policy which denies user X to execute e.g. any suid >> programs. >> >> >> Bernd. >> > > Hmm, funny.. > > I installed this patch on top of > > commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, > origin/master, origin/HEAD) > Merge: c966813ea120 7b6216baae75 > Author: Linus Torvalds > Date: Thu Nov 20 11:04:37 2025 -0800 > > but it does panic when I try to boot: > > [ 0.870539] TERM=1inux > [ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error > -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing > i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for > guidance > [ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 > PREEMPT(voluntary) > [ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS > VirtualBo x 12/01/2006 > [ 0.870580] Call Trace: > [ 0.870590] > [ 0.870592] vpanic+0x36d/0x380 > [ 0.870607] ? __pfx_kernel_init+0x10/0x10 > [ 0.870615] panic+0x5b/0x60 > [ 0.870617] kernel_init+0x17d/0x1c0 > [ 0.870623] ret_from_fork+0x124/0x150 > [ 0.870625} ? __pfx_kernel_init+0x10/0x10 > [ 0.870627] ret_from_fork_asm+0x1a/0x30 > [ 0.870632] > [ 0.8706631 Kernel Offset: 0x3a80 from Ox8100 (relocation > ran ge: 0x8000-0xbfff) > [ 0.880034] ---[ end Kernel panic - not syncing: No working init found
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
On 11/21/25 10:35, Bernd Edlinger wrote: > On 11/21/25 08:18, Eric W. Biederman wrote: >> Bernd Edlinger writes: >> >>> Hi Eric, >>> >>> thanks for you valuable input on the topic. >>> >>> On 11/21/25 00:50, Eric W. Biederman wrote: "Eric W. Biederman" writes: > 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. I should add that the permission checks happen in open_exec, everything that follows credential wise is just about representing in struct cred the credentials the new executable will have. So I am really at a loss why we have had this complicated way of computing of computed the credentials all of these years full of time of check to time of use problems. >>> >>> Well, I think I see a problem with your patch: >>> >>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might >>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux >>> or the smack security engines at least. Previously that callback >>> was called before the point of no return, and the return code should >>> be returned as a return code the the caller of execve. But if we move >>> that check after the point of no return, the caller will get killed >>> due to the failed security check. >>> >>> Or did I miss something? >> >> I think we definitely need to document this change in behavior. I would >> call ending the exec with SIGSEGV vs -EPERM a quality of implementation >> issue. The exec is failing one way or the other so I don't see it as a >> correctness issue. >> >> In the case of ptrace in general I think it is a bug if the mere act of >> debugging a program changes it's behavior. So which buggy behavior >> should we prefer? SIGSEGV where it is totally clear that the behavior >> has changed or -EPERM and ask the debugged program to handle it. >> I lean towards SIGSEGV because then it is clear the code should not >> handle it. >> >> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to >> handle unexpected things happening is to terminate the application. >> >> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know >> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside >> of CLONE_THREAD. >> >> >> Plus all of these things are only considerations if we are exec'ing a >> program that transitions to a different set of credentials. Something >> that happens but is quite rare itself. >> >> In practice I don't expect there is anything that depends on the exact >> behavior of what happens when exec'ing a suid executable to gain >> privileges when ptraced. The closes I can imagine is upstart and >> I think upstart ran as root when ptracing other programs so there is no >> gaining of privilege and thus no reason for a security module to >> complain. >> >> Who knows I could be wrong, and someone could actually care. Which is >> hy I think we should document it.>> > > > Well, I dont know for sure, but the security engine could deny the execution > for any reason, not only because of being ptraced. > Maybe there can be a policy which denies user X to execute e.g. any suid > programs. > > > Bernd. > Hmm, funny.. I installed this patch on top of commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, origin/master, origin/HEAD) Merge: c966813ea120 7b6216baae75 Author: Linus Torvalds Date: Thu Nov 20 11:04:37 2025 -0800 but it does panic when I try to boot: [ 0.870539] TERM=1inux [ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance [ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 PREEMPT(voluntary) [ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBo x 12/01/2006 [ 0.870580] Call Trace: [ 0.870590] [ 0.870592] vpanic+0x36d/0x380 [ 0.870607] ? __pfx_kernel_init+0x10/0x10 [ 0.870615] panic+0x5b/0x60 [ 0.870617] kernel_init+0x17d/0x1c0 [ 0.870623] ret_from_fork+0x124/0x150 [ 0.870625} ? __pfx_kernel_init+0x10/0x10 [ 0.870627] ret_from_fork_asm+0x1a/0x30 [ 0.870632] [ 0.8706631 Kernel Offset: 0x3a80 from Ox8100 (relocation ran ge: 0x8000-0xbfff) [ 0.880034] ---[ end Kernel panic - not syncing: No working init found. Try passing init option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. 1---` Is that a known problem? Bernd.
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
On 11/21/25 08:18, Eric W. Biederman wrote: > Bernd Edlinger writes: > >> Hi Eric, >> >> thanks for you valuable input on the topic. >> >> On 11/21/25 00:50, Eric W. Biederman wrote: >>> "Eric W. Biederman" writes: >>> 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. >>> >>> I should add that the permission checks happen in open_exec, >>> everything that follows credential wise is just about representing in >>> struct cred the credentials the new executable will have. >>> >>> So I am really at a loss why we have had this complicated way of >>> computing of computed the credentials all of these years full of >>> time of check to time of use problems. >>> >> >> Well, I think I see a problem with your patch: >> >> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might >> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux >> or the smack security engines at least. Previously that callback >> was called before the point of no return, and the return code should >> be returned as a return code the the caller of execve. But if we move >> that check after the point of no return, the caller will get killed >> due to the failed security check. >> >> Or did I miss something? > > I think we definitely need to document this change in behavior. I would > call ending the exec with SIGSEGV vs -EPERM a quality of implementation > issue. The exec is failing one way or the other so I don't see it as a > correctness issue. > > In the case of ptrace in general I think it is a bug if the mere act of > debugging a program changes it's behavior. So which buggy behavior > should we prefer? SIGSEGV where it is totally clear that the behavior > has changed or -EPERM and ask the debugged program to handle it. > I lean towards SIGSEGV because then it is clear the code should not > handle it. > > In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to > handle unexpected things happening is to terminate the application. > > In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know > of any good uses of any good uses of sys_clone(CLONE_FS ...) outside > of CLONE_THREAD. > > > Plus all of these things are only considerations if we are exec'ing a > program that transitions to a different set of credentials. Something > that happens but is quite rare itself. > > In practice I don't expect there is anything that depends on the exact > behavior of what happens when exec'ing a suid executable to gain > privileges when ptraced. The closes I can imagine is upstart and > I think upstart ran as root when ptracing other programs so there is no > gaining of privilege and thus no reason for a security module to > complain. > > Who knows I could be wrong, and someone could actually care. Which is> hy I > think we should document it. > Well, I dont know for sure, but the security engine could deny the execution for any reason, not only because of being ptraced. Maybe there can be a policy which denies user X to execute e.g. any suid programs. Bernd.
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
Bernd Edlinger writes: > Hi Eric, > > thanks for you valuable input on the topic. > > On 11/21/25 00:50, Eric W. Biederman wrote: >> "Eric W. Biederman" writes: >> >>> 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. >> >> I should add that the permission checks happen in open_exec, >> everything that follows credential wise is just about representing in >> struct cred the credentials the new executable will have. >> >> So I am really at a loss why we have had this complicated way of >> computing of computed the credentials all of these years full of >> time of check to time of use problems. >> > > Well, I think I see a problem with your patch: > > When the security engine gets the LSM_UNSAFE_PTRACE flag, it might > e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux > or the smack security engines at least. Previously that callback > was called before the point of no return, and the return code should > be returned as a return code the the caller of execve. But if we move > that check after the point of no return, the caller will get killed > due to the failed security check. > > Or did I miss something? I think we definitely need to document this change in behavior. I would call ending the exec with SIGSEGV vs -EPERM a quality of implementation issue. The exec is failing one way or the other so I don't see it as a correctness issue. In the case of ptrace in general I think it is a bug if the mere act of debugging a program changes it's behavior. So which buggy behavior should we prefer? SIGSEGV where it is totally clear that the behavior has changed or -EPERM and ask the debugged program to handle it. I lean towards SIGSEGV because then it is clear the code should not handle it. In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to handle unexpected things happening is to terminate the application. In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know of any good uses of any good uses of sys_clone(CLONE_FS ...) outside of CLONE_THREAD. Plus all of these things are only considerations if we are exec'ing a program that transitions to a different set of credentials. Something that happens but is quite rare itself. In practice I don't expect there is anything that depends on the exact behavior of what happens when exec'ing a suid executable to gain privileges when ptraced. The closes I can imagine is upstart and I think upstart ran as root when ptracing other programs so there is no gaining of privilege and thus no reason for a security module to complain. Who knows I could be wrong, and someone could actually care. Which is hy I think we should document it. Eric
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
Hi Eric, thanks for you valuable input on the topic. On 11/21/25 00:50, Eric W. Biederman wrote: > "Eric W. Biederman" writes: > >> 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. > > I should add that the permission checks happen in open_exec, > everything that follows credential wise is just about representing in > struct cred the credentials the new executable will have. > > So I am really at a loss why we have had this complicated way of > computing of computed the credentials all of these years full of > time of check to time of use problems. > Well, I think I see a problem with your patch: When the security engine gets the LSM_UNSAFE_PTRACE flag, it might e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux or the smack security engines at least. Previously that callback was called before the point of no return, and the return code should be returned as a return code the the caller of execve. But if we move that check after the point of no return, the caller will get killed due to the failed security check. Or did I miss something? Thanks Bernd. > Eric
Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
"Eric W. Biederman" writes: > 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. I should add that the permission checks happen in open_exec, everything that follows credential wise is just about representing in struct cred the credentials the new executable will have. So I am really at a loss why we have had this complicated way of computing of computed the credentials all of these years full of time of check to time of use problems. Eric
