From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750813AbdK2SUJ (ORCPT ); Wed, 29 Nov 2017 13:20:09 -0500 Received: from h2.hallyn.com ([78.46.35.8]:45120 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751811AbdK2SUF (ORCPT ); Wed, 29 Nov 2017 13:20:05 -0500 Date: Wed, 29 Nov 2017 12:20:04 -0600 From: "Serge E. Hallyn" To: Kees Cook Cc: Andrew Morton , Ben Hutchings , James Morris , Serge Hallyn , Andy Lutomirski , Oleg Nesterov , Jiri Slaby , linux-kernel@vger.kernel.org Subject: Re: [PATCH] exec: Avoid RLIMIT_STACK races with prlimit() Message-ID: <20171129182004.GF14545@mail.hallyn.com> References: <20171127193457.GA11348@beast> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171127193457.GA11348@beast> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Reported-by: Brad Spengler > Fixes: 64701dee4178e ("exec: Use sane stack rlimit under secureexec") > Cc: stable@vger.kernel.org > Cc: James Morris > Cc: Serge Hallyn Acked-by: Serge Hallyn 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 > Cc: Oleg Nesterov > Cc: Jiri Slaby > Signed-off-by: Kees Cook > --- > 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