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

[Kees Cook <keescook@chromium.org>]

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