Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3

[Ben Hutchings]

On Fri, 2017-07-07 at 11:57 -0700, Kees Cook wrote:
> To avoid pathological stack usage or the need to special-case setuid
> execs, just limit all arg stack usage to at most _STK_LIM / 4 * 3 (6MB).
> 
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> Âfs/exec.c | 8 ++++----
> Â1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 904199086490..ddca2cf15f71 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -221,7 +221,6 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
> Â	if (write) {
> Â		unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
> Â		unsigned long ptr_size;
> -		struct rlimit *rlim;
> Â
> Â		/*
> Â		Â* Since the stack will hold pointers to the strings, we
> @@ -250,14 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
> Â			return page;
> Â
> Â		/*
> -		Â* Limit to 1/4-th the stack size for the argv+env strings.
> +		Â* Limit to 1/4 of the max stack size or 3/4 of _STK_LIM
> +		Â* (whichever is smaller) for the argv+env strings.
> Â		Â* This ensures that:
> Â		Â*ÂÂ- the remaining binfmt code will not run out of stack space,
> Â		Â*ÂÂ- the program will have a reasonable amount of stack left
> Â		Â*ÂÂÂÂto work from.
> Â		Â*/
> -		rlim = current->signal->rlim;
> -		if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4)
> +		if (size > min_t(unsigned long, rlimit(RLIMIT_STACK) / 4,
> +						_STK_LIM / 4 * 3))

You're dropping a READ_ONCE(), which I assume is there to guard against
races with prlimit().  That should probably be kept.

(When we exec a setuid program, is prlimit() by the real user already
blocked at this point?  If not then the stack limit could still be
reduced so that the stack is full of arguments.  But I don't see that
this is exploitable, at least not in the same way as very large
stacks.)

Ben.

> Â			goto fail;
> Â	}
> Â
> --Â
> 2.7.4
> 
> 
-- 
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer

Attachment: signature.asc
Description: This is a digitally signed message part