exec: Avoid RLIMIT_STACK races with prlimit()
Subsystems (2)
PATCH 1/1 exec: Avoid RLIMIT_STACK races with prlimit()
While the defense-in-depth RLIMIT_STACK limit on setuid processes was protected against races from other threads calling setrlimit(), I missed protecting it against races from external processes calling prlimit(). This adds locking around the change and makes sure that rlim_max is set too.
--- fs/exec.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
fs/exec.c +6 -1
@@ -1340,10 +1340,15 @@ void setup_new_exec(struct linux_binprm * bprm)* avoid bad behavior from the prior rlimits. This has to * happen before arch_pick_mmap_layout(), which examines * RLIMIT_STACK, but after the point of no return to avoid -* needing to clean up the change on failure. +* races from other threads changing the limits. This also +* must be protected from races with prlimit() calls. */ +task_lock (current->group_leader);if (current->signal->rlim[RLIMIT_STACK ].rlim_cur > _STK_LIM) current->signal->rlim[RLIMIT_STACK ].rlim_cur = _STK_LIM; +if (current->signal->rlim[RLIMIT_STACK ].rlim_max > _STK_LIM) + current->signal->rlim[RLIMIT_STACK ].rlim_max = _STK_LIM; +task_unlock (current->group_leader); }arch_pick_mmap_layout (current->mm);
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 6 lines · Kees Cook
While the defense-in-depth
RLIMIT_STACKlimit on setuid processes was protected against races from other threads callingsetrlimit(), I missed protecting it against races from external processes callingprlimit(). This adds locking around the change and makes sure thatrlim_maxis set too.
Quoted 6 lines
Reported-by Ben Hutchings <[email protected]>Reported-by Brad Spengler <[email protected]>Fixes 64701dee4178e ("exec: Use sane stack rlimit under secureexec")Cc James Morris <[email protected]>Cc Serge Hallyn <[email protected]>
The only thing i'm wondering is in do_prlimit():
. 1480 if (new_rlim) {
. 1481 if (new_rlim->rlim_cur > new_rlim->rlim_max)
. 1482 return -EINVAL;
that bit is done not under the lock. Does that still allow a race, if this check is done before the below block, and then the rest proceeds after?
I *think* not, because later in do_prlimit() it will return -EPERM if
. 1500 if (new_rlim->rlim_max > rlim->rlim_max &&
. 1501 !capable(CAP_SYS_RESOURCE))
Although rlim is gathered before the lock, but that is a struct * so should be ok?
Quoted 36 lines
Cc Andy Lutomirski <[email protected]>Cc Oleg Nesterov <[email protected]>Cc Jiri Slaby <[email protected]>Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)fs/exec.c +6
-1@@ -1340,10 +1340,15 @@ void setup_new_exec(struct linux_binprm * bprm)* avoid bad behavior from the prior rlimits. This has to * happen before arch_pick_mmap_layout(), which examines * RLIMIT_STACK, but after the point of no return to avoid -* needing to clean up the change on failure. +* races from other threads changing the limits. This also +* must be protected from races with prlimit() calls. */ +task_lock (current->group_leader);if (current->signal->rlim[RLIMIT_STACK ].rlim_cur > _STK_LIM) current->signal->rlim[RLIMIT_STACK ].rlim_cur = _STK_LIM; +if (current->signal->rlim[RLIMIT_STACK ].rlim_max > _STK_LIM) + current->signal->rlim[RLIMIT_STACK ].rlim_max = _STK_LIM; +task_unlock (current->group_leader); }arch_pick_mmap_layout (current->mm);Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 7 lines · Serge E. Hallyn
Quoted 6 lines · Kees Cook
While the defense-in-depth
RLIMIT_STACKlimit on setuid processes was protected against races from other threads callingsetrlimit(), I missed protecting it against races from external processes callingprlimit(). This adds locking around the change and makes sure thatrlim_maxis set too.
Quoted 8 lines
Quoted 6 lines
Reported-by Ben Hutchings <[email protected]>Reported-by Brad Spengler <[email protected]>Fixes 64701dee4178e ("exec: Use sane stack rlimit under secureexec")Cc James Morris <[email protected]>Cc Serge Hallyn <[email protected]>Acked-by Serge Hallyn <[email protected]>
Thanks!
Quoted 11 lines
The only thing i'm wondering is in
do_prlimit():. 1480 if (
new_rlim) {
. 1481 if (new_rlim->rlim_cur>new_rlim->rlim_max)
. 1482 return-EINVAL;that bit is done not under the lock. Does that still allow a race, if this check is done before the below block, and then the rest proceeds after?
Quoted 7 lines
I *think* not, because later in
do_prlimit()it will return-EPERMif. 1500 if (
new_rlim->rlim_max>rlim->rlim_max&&
. 1501 !capable(CAP_SYS_RESOURCE))Although rlim is gathered before the lock, but that is a struct * so should be ok?
I stared at this for a while too. I think it's okay because the max is checked under the lock, so the max can't be raced to be raised. The cur value could get raced, though, but I don't think that's a problem, since it's the "soft" limit.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 35 lines · Kees Cook
Quoted 7 lines · Serge E. Hallyn
Quoted 6 lines · Kees Cook
While the defense-in-depth
RLIMIT_STACKlimit on setuid processes was protected against races from other threads callingsetrlimit(), I missed protecting it against races from external processes callingprlimit(). This adds locking around the change and makes sure thatrlim_maxis set too.Quoted 8 lines
Quoted 6 lines
Reported-by Ben Hutchings <[email protected]>Reported-by Brad Spengler <[email protected]>Fixes 64701dee4178e ("exec: Use sane stack rlimit under secureexec")Cc James Morris <[email protected]>Cc Serge Hallyn <[email protected]>Acked-by Serge Hallyn <[email protected]>Thanks!
Quoted 16 lines
The only thing i'm wondering is in
do_prlimit():. 1480 if (
new_rlim) {
. 1481 if (new_rlim->rlim_cur>new_rlim->rlim_max)
. 1482 return-EINVAL;that bit is done not under the lock. Does that still allow a race, if this check is done before the below block, and then the rest proceeds after?
I *think* not, because later in
do_prlimit()it will return-EPERMif. 1500 if (
new_rlim->rlim_max>rlim->rlim_max&&
. 1501 !capable(CAP_SYS_RESOURCE))
Quoted 7 lines
Quoted 2 lines
Although rlim is gathered before the lock, but that is a struct * so should be ok?
I stared at this for a while too. I think it's okay because the max is checked under the lock, so the max can't be raced to be raised. The cur value could get raced, though, but I don't think that's a problem, since it's the "soft" limit.
Oh, right, and so if soft > hard that will just end up ignored... ok.