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]

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

On Mon 10-07-17 08:39:43, Kees Cook wrote:
> On Mon, Jul 10, 2017 at 6:13 AM, Michal Hocko <mhocko@...nel.org> wrote:
> > 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...
> 
> Linus applied this one, yes.

Hmm, this is rather rushed...
 
> > 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 it would be best to only apply this to setuid processes, but
> Linus asked that this change be universal. After my secureexec
> refactoring, I think it should be possible to add a "how much stack
> has already been used?" check in setup_new_exec() and abort the
> privileged exec if it exceeds the secureexec stack limit.
> 
> > 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).
> 
> I think Linus's intention is to back off from the stack gap, but maybe
> I misunderstood.

We will always need some gap inforcement. 256 pages enforced currently
can be loosen after the stack probing is generally spread. But let's be
realistic there are people using other (non-distribution) compilers and it
would be good to have them covered as well, to some extent at least.
Also we might remove the expand_stack enforcement but we will still need
to keep a gap for new mmaps. With all that in place I am not really sure
what this patch actually prevents from.

> > 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?
> 
> With a variable stack limit, we'll continue to run risks of
> gap-jumping if the compiler isn't doing stack probing, so while we
> might be able to further improve the layout logic, I think we still
> need to impose limits on setuid programs.

So how exactly this patch helps if we really enforce the gap between the
stack and the mmap base?

-- 
Michal Hocko
SUSE Labs