Re: [PATCH] posix: Optimize Linux posix_spawn

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> On 08/05/2019 12:26, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>>> 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.
>> 
>> The problem is not the case where f2 actually hits the guard page.  You
>> are correct that this is detected as before.
>> 
>> What is not detected is a partial overflow that hits the activation
>> record of f1 *outside the array*, but stops short of extending to the
>> guard page.  It's impossible to tell what will happen in this case
>> because we do not know the frame layout that GCC has produced for f1.
>
> But would in this case protected as well with -fstack-protector, where
> f2 would overwrite f1 canary?

No, the canary is at the top of the activation record, and it would not
be checked upon return from f2 anyway.

>> The issue can only be avoided reliably by writing f1 in assembler, I
>> think.  INTERNAL_SYSCALL is probably not safe for vfork.
>
> In any case, I still think the specific scenario is quite an overkill 
> one to block this optimization. Specially because the possible adjustments
> is somewhat infeasible (rewrite the helper spawn function in assembly,
> or the impossibility to use INTERNAL_SYSCALL which may itself cause a
> stack allocation) or add a arch-specific vfork variant to handle this
> specific case.

I do not have a strong opinion regarding this matter.  I'd still prefer
using vfork here.  I really don't understand your reluctance in this
matter.

On the other hand, an argument could be made to keep the mmap allocation
and use MAP_SHARED, so that it's possible to communicate the correct
error for vfork-as-fork environments.

Thanks,
Florian