linux-kernel - Re: [PATCH] exec: Limit arg stack to at most _STK

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]

Date:   Mon, 10 Jul 2017 14:44:04 +0100
From:   Ben Hutchings <ben@...adent.org.uk>
To:     Kees Cook <keescook@...omium.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andy Lutomirski <luto@...nel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Michal Hocko <mhocko@...nel.org>,
        Hugh Dickins <hughd@...gle.com>,
        Oleg Nesterov <oleg@...hat.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3

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@...omium.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))

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


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