Re: [PATCH] posix: Optimize Linux posix_spawn

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 08/05/2019 10:12, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 07/05/2019 12:38, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> On 07/05/2019 05:33, Florian Weimer wrote:
>>>>> * Adhemerval Zanella:
>>>>>
>>>>>> The current internal posix_spawn symbol for Linux (__spawni) requires
>>>>>> to allocate a dynamic stack based on input arguments to handle the
>>>>>> SPAWN_XFLAGS_USE_PATH internal flag, which re-issue the input binary
>>>>>> as a shell script if execve call return ENOEXEC (to execute shell
>>>>>> scripts with an initial shebang).
>>>>>>
>>>>>> This is done only for compatibility mode and the generic case does not
>>>>>> require the extra calculation plus the potential large mmap/munmap
>>>>>> call.  For default case, a pre-defined buffer is sufficed to use on the
>>>>>> clone call instead.
>>>>>>
>>>>>> This patch optimizes Linux spawni by allocating a dynamic stack only
>>>>>> for compatibility symbol (SPAWN_XFLAGS_USE_PATH).  For generic case,
>>>>>> a stack allocated buffer is used instead.
>>>>>
>>>>> I'm still not convinced that stack switching is required here.
>>>>
>>>> - Performance-wise it would be just a stack increment (which tends to
>>>>   better than a mmap/unmap), 
>>>>
>>>> - It also should work with any stack hardening (--enable-stack-protector=all
>>>>   shows no issue) and the stack uses uses the calling process stack as well
>>>>   (which should trigger overflow due guard pages).
>>>
>>> No, since there is no guard page as part of the on-stack array, we do
>>> not detect any overflow into other parts of the stack frame if it
>>> happens to be stored below the frame.  So this still disables some of
>>> the hardening checks.
>>
>> Using a stack allocated array is essentially the same as using the same
>> stack of the caller: stack overflow in created helper thread will trigger
>> a fault based on the guard page of the calling thread (since stack is
>> allocated object will be based on caller thread). I give you this is sketchy
>> standard-wise.
> 
> The difference is this.  f1 is the outer function (that prepares the new
> stack, f2 is the function which runs with a switch stacked.
> 
> With a stack reuse in an assembler function, it looks like this:
> 
>    +-----------------------------
>    | Activation record for f1
>    |
>    |
>    +-----------------------------
>    | Activation record for f2
>    |
>    :
>    +=== Guard ===================
> 
> So it just looks like an ordinary function call.  Stack overflow in f2
> will hit the guard (if built with -fstack-clash-protection).
> 
> With an on-stack array for the stack, it looks like this:
> 
>    +-----------------------------
>    | Activation record for f1
>    |
>    | [ Array
>    | [ +-----------------------------
>    | [ | Activation record for f2
>    | [ |
>    | [ |
>    |
>    | (*)
>    |
>    :
>    +=== Guard ===================
> 
> 
> The problem is that technically, the activation record of f1 continues
> after the array, indicated by (*), and we won't know what the compiler
> will store there.  This means that even if f2 is built with
> -fstack-clash-protection, stack overflow detection is unreliable because
> the overflow into f1's activation record does not hit a guard page.

Thanks for explanation, but I still failing to see exactly why even with 
-fstack-clash-protection f2 probing will fail to hit the guard page.

The f2 probing will query page size based on stack address based on f1 stack
address, so the action record (which I assume you meant to be the prologue 
probe based on stack allocation added by compiler) a at f2 will essentially 
probe f1 stack memory.  Following your diagram, a potentially stack overflow 
on f2 would do:

   +-----------------------------
   | Activation record for f1
   |
   | [ Array
   | [ +-----------------------------
   | [ | Activation record for f2
   | [ |
   | [ |
   |
   | (f1act)
   |
   :
   +=== Guard ===================
   : (f2act)

My understanding is even -fstack-protector would work as well, since a
buffer overrun on f2 will still overwrite f1 canary. I am not seeing why
this specific *clone* usage is different than a function call and if it 
is indeed it might be troublesome since it s a potentially vector of 
attack.

> 
> Sorry if this is totally obvious.  If it is and you still maintain that
> both cases are equivalent, then perhaps I'm confused.

It is not a simple topic and you are probably more involved in recently
compiler hardening for stack-clash than me. Thanks for your patience though.

> 
> Thanks,
> Florian
>