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

[Michal Hocko <mhocko@kernel.org>]

I am not sure whether this is still actual because there are just too
many pathes flying around these days. I am still trying to catch up...

On Fri 07-07-17 11:57:29, 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).

I am worried that we've grown  users which rely on a large argument
lists and now we are pulling more magic constants into the game. This
just calls for another breakage.

I think we should simply step back and think about what we want to fix
here actually. If this is the pathological case when the attacker can
grow the stack too large and too close to a regular mappings then we
already have means to address that (stack gap).

If we are worried that mmaps can get way too close to the stack then
I would question why this is possible at all. Bottom-up layout will
require consuming mmap space and top-down layout seems just broken
because we do not try to offset the mmap_base relative to the stack and
rather calculate both from TASK_SIZE. Or at least this is my current
undestanding. Am I missing something? Aren't we just trying to fix a bug
at a wrong place?

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  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))
>  			goto fail;
>  	}
>  
> -- 
> 2.7.4
> 
> 
> -- 
> Kees Cook
> Pixel Security

-- 
Michal Hocko
SUSE Labs