linux-kernel - Re: [PATCH] Restrict stack space reservation 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: <32102.1265625951@neuling.org>
Date:	Mon, 08 Feb 2010 21:45:51 +1100
From:	Michael Neuling <mikey@...ling.org>
To:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
cc:	Anton Blanchard <anton@...ba.org>,
	Andrew Morton <akpm@...ux-foundation.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>,
	WANG Cong <xiyou.wangcong@...il.com>,
	Paul Mackerras <paulus@...ba.org>, benh@...nel.crashing.org,
	miltonm@....com, aeb@....nl
Subject: Re: [PATCH] Restrict stack space reservation to rlimit


In message <20100208145240.FB58.A69D9226@...fujitsu.com> you wrote:
> > > >  
> > > > Hi,
> > > > 
> > > > > Why do we need page size independent stack size? It seems to have
> > > > > compatibility breaking risk.
> > > > 
> > > > I don't think so. The current behaviour is clearly wrong, we dont need 
a
> > > > 16x larger stack just because you went from a 4kB to a 64kB base page
> > > > size. The user application stack usage is the same in both cases.
> > > 
> > > I didn't discuss which behavior is better. Michael said he want to apply
> > > his patch to 2.6.32 & 2.6.33. stable tree never accept the breaking
> > > compatibility patch.
> > > 
> > > Your answer doesn't explain why can't we wait it until next merge window.
> > > 
> > > btw, personally, I like page size indepent stack size. but I'm not sure
> > > why making stack size independency is related to bug fix.
> > 
> > I tend to agree.  
> > 
> > Below is just the bug fix to limit the reservation size based rlimit.
> > We still reserve different stack sizes based on the page size as
> > before (unless we hit rlimit of course).
> 
> Thanks.
> 
> I agree your patch in almost part. but I have very few requests.
> 
> 
> > Mikey
> > 
> > Restrict stack space reservation to rlimit
> > 
> > When reserving stack space for a new process, make sure we're not
> > attempting to allocate more than rlimit allows.
> > 
> > This fixes a bug cause by b6a2fea39318e43fee84fa7b0b90d68bed92d2ba 
> > "mm: variable length argument support" and unmasked by
> > fc63cf237078c86214abcb2ee9926d8ad289da9b 
> > "exec: setup_arg_pages() fails to return errors".
> 
> Your initial mail have following problem use-case. please append it
> into the patch description.
> 
> 	On recent ppc64 kernels, limiting the stack (using 'ulimit -s blah') is
> 	now more restrictive than it was before.  On 2.6.31 with 4k pages I
> 	could run 'ulimit -s 16; /usr/bin/test' without a problem.  Now with
> 	mainline, even 'ulimit -s 64; /usr/bin/test' gets killed.

Ok,  I'll add this info in.  

> 
> > 
> > Signed-off-by: Michael Neuling <mikey@...ling.org>
> > Cc: Anton Blanchard <anton@...ba.org>
> > Cc: stable@...nel.org
> > ---
> >  fs/exec.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > Index: linux-2.6-ozlabs/fs/exec.c
> > ===================================================================
> > --- linux-2.6-ozlabs.orig/fs/exec.c
> > +++ linux-2.6-ozlabs/fs/exec.c
> > @@ -627,10 +627,13 @@ int setup_arg_pages(struct linux_binprm 
> >  			goto out_unlock;
> >  	}
> >  
> > +	stack_base = min(EXTRA_STACK_VM_PAGES * PAGE_SIZE,
> > +			 current->signal->rlim[RLIMIT_STACK].rlim_cur -
> > +			   PAGE_SIZE);
> 
> This line is a bit unclear why "- PAGE_SIZE" is necessary.

This is because the stack is already 1 page in size.  I'm going to
change that code to make it clearer...  hopefully :-)

> personally, I like following likes explicit comments.
> 
> 	stack_expand = EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> 	stack_lim = ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur);
> 
> 	/* Initial stack must not cause stack overflow. */
> 	if (stack_expand + PAGE_SIZE > stack_lim)
> 		stack_expand = stack_lim - PAGE_SIZE;
> 
> note: accessing rlim_cur require ACCESS_ONCE.
> 
> 
> Thought?

Thanks, looks better/clearer to me too.  I'll change, new patch coming....

Mikey

> 
> 
> >  #ifdef CONFIG_STACK_GROWSUP
> > -	stack_base = vma->vm_end + EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> > +	stack_base = vma->vm_end + stack_base;
> >  #else
> > -	stack_base = vma->vm_start - EXTRA_STACK_VM_PAGES * PAGE_SIZE;
> > +	stack_base = vma->vm_start - stack_base;
> >  #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/