Re: [RFC][PATCH] exec: Use init rlimits for setuid exec

[Andy Lutomirski]

On Thu, Jul 6, 2017 at 12:12 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Thu, Jul 6, 2017 at 10:52 AM, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>> On Thu, Jul 6, 2017 at 10:29 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>>>
>>>>  (a) minimal: just use our existing default stack (and stack _only_)
>>>> limit value for suid binaries that actually get extra permissions: {
>>>> _STK_LIM, RLIM_INFINITY }.
>>>
>>> This would look a lot like the existing patch; it'd just not copy the
>>> init process rlimits.
>>
>> Can't we just do the final rlimit setting so late in execve that we
>> don't need that whole "saved_rlimit" thing?
>
> The stack rlimit defines the mmap layout too:
>
> do_execveat_common() ->
> exec_binprm() ->
> search_binary_handler() ->
> fmt->load_binary (load_elf_binary()) ->
> setup_new_exec() ->
> arch_pick_mmap_layout() ->
> mmap_is_legacy() ->
> rlimit(RLIMIT_STACK) == RLIM_INFINITY
>
> exec_binprm() happens after the other stack setup (copy_strings()), so
> if we wanted to avoid saved_rlimit, we'd have to replumb how
> arch_pick_mmap_layout() works and how copy_strings() performs its
> calculations (neither looks too terrible).

How about a much simpler solution: don't read rlimit at all in
copy_strings(), let alone try to enforce it.  Instead, just before the
point of no return, check how much stack space is already used and, if
it's more than an appropriate threshold (e.g. 1/4 of the rlimit),
abort.  Sure, this adds overhead if we're going to abort, but does
that really matter?

I don't see why using rlimit for layout control makes any sense
whatsoever.  Is there some historical reason we need that?  As far as
I can see (on insufficient inspection) is that the kernel is trying to
guarantee that, if we have so much arg crap that our remaining stack
is less than 128k, then we don't exceed our limit by a little bit.