From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752066AbdHPMRN (ORCPT ); Wed, 16 Aug 2017 08:17:13 -0400 Received: from mail-wr0-f194.google.com ([209.85.128.194]:36513 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751702AbdHPMRM (ORCPT ); Wed, 16 Aug 2017 08:17:12 -0400 Subject: Re: [PATCH] exec: Check stack space more strictly To: Andy Lutomirski , security@kernel.org, Andrew Morton Cc: linux-kernel@vger.kernel.org References: <34b776c32a3f75ff155bc85d3c1554e21be7bab2.1500330091.git.luto@kernel.org> From: Nikolay Borisov Message-ID: <6dc0cb42-a65b-5142-0f40-8f05b616e63e@gmail.com> Date: Wed, 16 Aug 2017 15:17:08 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <34b776c32a3f75ff155bc85d3c1554e21be7bab2.1500330091.git.luto@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org minor nit below On 18.07.2017 01:22, Andy Lutomirski wrote: > We can currently blow past the stack rlimit and cause odd behavior > if there are accounting bugs, rounding issues, or races. It's not > clear that the odd behavior is actually a problem, but it's nicer to > fail the exec instead of getting out of sync with stack limits. > > Improve the code to more carefully check for space and to abort if > our stack mm gets too large in setup_arg_pages(). > > Signed-off-by: Andy Lutomirski > --- > fs/exec.c | 44 ++++++++++++++++++++++++++++++++++---------- > 1 file changed, 34 insertions(+), 10 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 62175cbcc801..0c60c0495269 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -764,23 +764,47 @@ int setup_arg_pages(struct linux_binprm *bprm, > /* mprotect_fixup is overkill to remove the temporary stack flags */ > vma->vm_flags &= ~VM_STACK_INCOMPLETE_SETUP; > > - stack_expand = 131072UL; /* randomly 32*4k (or 2*64k) pages */ > - stack_size = vma->vm_end - vma->vm_start; > /* > * Align this down to a page boundary as expand_stack > * will align it up. > */ > rlim_stack = rlimit(RLIMIT_STACK) & PAGE_MASK; > + stack_size = vma->vm_end - vma->vm_start; > + > + if (stack_size > rlim_stack) { > + /* > + * If we've already used too much space (due to accounting > + * bugs, alignment, races, or any other cause), bail. > + */ > + ret = -ENOMEM; > + goto out_unlock; > + } > + > + /* > + * stack_expand is the amount of space beyond the space already used > + * that we're going to pre-allocate in our stack. For historical > + * reasons, it's 128kB, unless we have less space than that available > + * in our rlimit. > + * > + * This particular historical wart is wrong-headed, though, since > + * we haven't finished binfmt-specific setup, and the binfmt code > + * is going to eat up some or all of this space. > + */ > + stack_expand = min(rlim_stack - stack_size, 131072UL); nit: Use the SZ_128K from sizes.h. > + > #ifdef CONFIG_STACK_GROWSUP > - if (stack_size + stack_expand > rlim_stack) > - stack_base = vma->vm_start + rlim_stack; > - else > - stack_base = vma->vm_end + stack_expand; > + if (TASK_SIZE_MAX - vma->vm_end < stack_expand) { > + ret = -ENOMEM; > + goto out_unlock; > + } > + stack_base = vma->vm_end + stack_expand; > #else > - if (stack_size + stack_expand > rlim_stack) > - stack_base = vma->vm_end - rlim_stack; > - else > - stack_base = vma->vm_start - stack_expand; > + if (vma->vm_start < mmap_min_addr || > + vma->vm_start - mmap_min_addr < stack_expand) { > + ret = -ENOMEM; > + goto out_unlock; > + } > + stack_base = vma->vm_start - stack_expand; > #endif > current->mm->start_stack = bprm->p; > ret = expand_stack(vma, stack_base); >