Messages in this thread Patch in this message | | | Date | Wed, 29 Nov 2017 12:20:04 -0600 | From | "Serge E. Hallyn" <> | Subject | Re: [PATCH] exec: Avoid RLIMIT_STACK races with prlimit() |
| |
Quoting Kees Cook (keescook@chromium.org): > 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 <ben.hutchings@codethink.co.uk> > Reported-by: Brad Spengler <spender@grsecurity.net> > Fixes: 64701dee4178e ("exec: Use sane stack rlimit under secureexec") > Cc: stable@vger.kernel.org > Cc: James Morris <james.l.morris@oracle.com> > Cc: Serge Hallyn <serge@hallyn.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
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 <luto@kernel.org> > Cc: Oleg Nesterov <oleg@redhat.com> > Cc: Jiri Slaby <jslaby@suse.cz> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > 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
| |