Quoting Kees Cook ([email protected]): > 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. > > Reported-by: Ben Hutchings <[email protected]> > Reported-by: Brad Spengler <[email protected]> > Fixes: 64701dee4178e ("exec: Use sane stack rlimit under secureexec") > Cc: [email protected] > Cc: James Morris <[email protected]> > Cc: Serge Hallyn <[email protected]>
Acked-by: 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? > 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(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 1d6243d9f2b6..6be2aa0ab26f 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -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); > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security
