linux-kernel - Re: [PATCH] Restrict initial stack space expansion to rlimit

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: <10733.1265752289@neuling.org>
Date:	Wed, 10 Feb 2010 08:51:29 +1100
From:	Michael Neuling <mikey@...ling.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
cc:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Americo Wang <xiyou.wangcong@...il.com>,
	Anton Blanchard <anton@...ba.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Oleg Nesterov <oleg@...hat.com>,
	James Morris <jmorris@...ei.org>, Ingo Molnar <mingo@...e.hu>,
	linux-fsdevel@...r.kernel.org, stable@...nel.org,
	linux-kernel@...r.kernel.org, linuxppc-dev@...abs.org,
	Serge Hallyn <serue@...ibm.com>,
	Paul Mackerras <paulus@...ba.org>, benh@...nel.crashing.org,
	miltonm@....com, aeb@....nl, linux-parisc@...r.kernel.org
Subject: Re: [PATCH] Restrict initial stack space expansion to rlimit


> > > note: it's untested.
> > 
> > Works for me on ppc64 with 4k and 64k pages.  Thanks!
> > 
> > I'd still like someone with a CONFIG_STACK_GROWSUP arch to test/ACK it
> > as well.
> 
> There's only one CONFIG_GROWSUP arch - parisc.
> 
> Guys, here's the rolled-up patch.  

FYI the rolled up patch still works fine on PPC64.  Thanks.  

> Could someone please test it on parisc?
> 
> err, I'm not sure what one needs to do to test it, actually. 
> Presumably it involves setting an unusual `ulimit -s'.  Can someone
> please suggest a test plan?

How about doing:
  'ulimit -s 15; ls'
before and after the patch is applied.  Before it's applied, 'ls' should
be killed.  After the patch is applied, 'ls' should no longer be killed.

I'm suggesting a stack limit of 15KB since it's small enough to trigger
20*PAGE_SIZE.  Also 15KB not a multiple of PAGE_SIZE, which is a trickier
case to handle correctly with this code.

4K pages on parisc should be fine to test with.

Mikey

> 
> From: Michael Neuling <mikey@...ling.org>
> 
> When reserving stack space for a new process, make sure we're not
> attempting to expand the stack by more than rlimit allows.
> 
> This fixes a bug caused by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba ("mm:
> variable length argument support") and unmasked by
> fc63cf237078c86214abcb2ee9926d8ad289da9b ("exec: setup_arg_pages() fails
> to return errors").
> 
> This bug means that when limiting the stack to less the 20*PAGE_SIZE (eg. 
> 80K on 4K pages or 'ulimit -s 79') all processes will be killed before
> they start.  This is particularly bad with 64K pages, where a ulimit below
> 1280K will kill every process.
> 
> Signed-off-by: Michael Neuling <mikey@...ling.org>
> Cc: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> Cc: Americo Wang <xiyou.wangcong@...il.com>
> Cc: Anton Blanchard <anton@...ba.org>
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: James Morris <jmorris@...ei.org>
> Cc: Ingo Molnar <mingo@...e.hu>
> Cc: Serge Hallyn <serue@...ibm.com>
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> Cc: <stable@...nel.org>
> 
>  fs/exec.c |   21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff -puN fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit
 fs/exec.c
> --- a/fs/exec.c~fs-execc-restrict-initial-stack-space-expansion-to-rlimit
> +++ a/fs/exec.c
> @@ -571,6 +571,9 @@ int setup_arg_pages(struct linux_binprm 
>  	struct vm_area_struct *prev = NULL;
>  	unsigned long vm_flags;
>  	unsigned long stack_base;
> +	unsigned long stack_size;
> +	unsigned long stack_expand;
> +	unsigned long rlim_stack;
>  
>  #ifdef CONFIG_STACK_GROWSUP
>  	/* Limit stack size to 1GB */
> @@ -627,10 +630,24 @@ int setup_arg_pages(struct linux_binprm 
>  			goto out_unlock;
>  	}
>  
> +	stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> +	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;
> +	rlim_stack = min(rlim_stack, stack_size);
>  #ifdef CONFIG_STACK_GROWSUP
> -	stack_base = vma->vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> +	if (stack_size + stack_expand > rlim_stack)
> +		stack_base = vma->vm_start + rlim_stack;
> +	else
> +		stack_base = vma->vm_end + stack_expand;
>  #else
> -	stack_base = vma->vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> +	if (stack_size + stack_expand > rlim_stack)
> +		stack_base = vma->vm_end - rlim_stack;
> +	else
> +		stack_base = vma->vm_start - stack_expand;
>  #endif
>  	ret = expand_stack(vma, stack_base);
>  	if (ret)
> _
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/