linux-kernel - Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100908115728.GB11762@grsecurity.net>
Date:	Wed, 8 Sep 2010 07:57:28 -0400
From:	Brad Spengler <spender@...ecurity.net>
To:	Roland McGrath <roland@...hat.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, oss-security@...ts.openwall.com,
	Solar Designer <solar@...nwall.com>,
	Kees Cook <kees.cook@...onical.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Oleg Nesterov <oleg@...hat.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Neil Horman <nhorman@...driver.com>,
	linux-fsdevel@...r.kernel.org, pageexec@...email.hu,
	"Brad Spengler <spender@...ecurity.net> Eugene Teo" 
	<eugene@...hat.com>
Subject: Re: [PATCH 1/3] setup_arg_pages: diagnose excessive argument size

I still don't think this addresses the whole problem.  Without question,
the rlimit / 4 check is bogus.  If nobody agrees with the intent of that 
check, then it should be removed, but I think the better solution is to 
fix the check so that it matches its original intent: let the initial 
stack setup be up to 1/Xth of the min(rlimit, TASK_SIZE dependent upon 
personality), which allows space for additional stack setup in the ELF 
loader and then further growth once the process is live.  If that 
amount is overstepped, then the exec will return an error to the calling 
process instead of being terminated.

It might be useful to consult with the people who introduced/approved 
the check in the first place, as they seemed to have reasons for 
implementing it.

-Brad

On Tue, Sep 07, 2010 at 07:35:49PM -0700, Roland McGrath wrote:
> The CONFIG_STACK_GROWSDOWN variant of setup_arg_pages() does not
> check the size of the argument/environment area on the stack.
> When it is unworkably large, shift_arg_pages() hits its BUG_ON.
> This is exploitable with a very large RLIMIT_STACK limit, to
> create a crash pretty easily.
> 
> Check that the initial stack is not too large to make it possible
> to map in any executable.  We're not checking that the actual
> executable (or intepreter, for binfmt_elf) will fit.  So those
> mappings might clobber part of the initial stack mapping.  But
> that is just userland lossage that userland made happen, not a
> kernel problem.
> 
> Signed-off-by: Roland McGrath <roland@...hat.com>
> ---
>  fs/exec.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 2d94552..1b63237 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -594,6 +594,11 @@ int setup_arg_pages(struct linux_binprm *bprm,
>  #else
>  	stack_top = arch_align_stack(stack_top);
>  	stack_top = PAGE_ALIGN(stack_top);
> +
> +	if (unlikely(stack_top < mmap_min_addr) ||
> +	    unlikely(vma->vm_end - vma->vm_start >= stack_top - mmap_min_addr))
> +		return -ENOMEM;
> +
>  	stack_shift = vma->vm_end - stack_top;
>  
>  	bprm->p -= stack_shift;

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)