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: <CAGXu5jJuVsCQb6DZdSS4vSxNzs7rhu-S+hhiRCsriZTp+Key_Q@mail.gmail.com>
Date:   Mon, 10 Jul 2017 08:34:41 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Ben Hutchings <ben@...adent.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        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>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3

On Mon, Jul 10, 2017 at 6:44 AM, Ben Hutchings <ben@...adent.org.uk> wrote:
> 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.

READ_ONCE() is in the rlimit() helper:

static inline unsigned long task_rlimit(const struct task_struct *tsk,
                unsigned int limit)
{
        return READ_ONCE(tsk->signal->rlim[limit].rlim_cur);
}

static inline unsigned long rlimit(unsigned int limit)
{
        return task_rlimit(current, limit);
}


> (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.)

Hm, prlimit64 lets you do remote tasks and has checks, but prlimit
against current has no checks (i.e. current can always set its own
rlimits). Additionally, I don't see anything that stops a race with
any of the rlimits. (I think Andy mentioned this too.)

In this particular patch, the race doesn't matter since we're bounded
by the _STK_LIM calculation, but everywhere else, it does seem to
matter...

I think a two threaded process could spin with prlimit() calls while
the other thread attempted to do execs()... :(

-Kees

-- 
Kees Cook
Pixel Security