Re: [PATCH] posix: Optimize Linux posix_spawn

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

On 08/05/2019 15:03, Florian Weimer wrote:
> * 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.

But f2 canary would be checked anyway (if it is also built with
stack hardening). I give you that it still might be an issue for 
a vfork interface which might issue user provided callbacks, but I
am not sure the really net gain for this specific scenario.

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

Besides the optimization, I am not really convinced that the security gain
is really expressive in this case, the current scenario that invokes
mmap is overkill for specific usage, since there no guard page current
scenario is not really better security-wise, and the other scenarios
you are suggestion adds even more complexity.

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

If the idea is to remove CLONE_VM and use MAP_SHARED instead, it would
better to just use fork (which has it own scalability issues).