[PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Adhemerval Zanella]

On 12/03/2021 05:52, Siddhesh Poyarekar wrote:
> On 3/10/21 6:20 PM, Adhemerval Zanella via Libc-alpha wrote:
>>
>>
>> On 10/03/2021 07:13, Siddhesh Poyarekar via Libc-alpha wrote:
>>> From: David Hughes <davidhughes205@gmail.com>
>>>
>>> Enabling --enable-stack-protector=all causes the following tests to fail:
>>>
>>>      FAIL: elf/ifuncmain9picstatic
>>>      FAIL: elf/ifuncmain9static
>>>
>>> Nick Alcock (who committed the stack protector code) marked the IFUNC
>>> resolvers with inhibit_stack_protector when he done the original work and
>>> suggested doing so again @ BZ #25680. This patch adds
>>> inhibit_stack_protector to ifuncmain9.
>>>
>>> After patch is applied, --enable-stack-protector=all does not fail the
>>> above tests.
>>
>> The BZ#25680 report makes me wonder if would be better to just disable
>> --enable-stack-protector=all on architecture with IFUNC for now.
>> This fix the issue on glibc testsuite, but it might still trigger
>> by users if this same trick is not used (as noted by Sergei).
> 
> The general problem is that it is at best pointless to add stack protector prologue/epilogue to ifunc resolver functions in static binaries.  On x86_64 it is visible because it results in a crash due to TLS segment register %fs not being set up but elsewhere, the stack chk guard is zero, which makes the dereference safe, but useless.  On ppc64le, the TLS setup happens first, thus avoiding the crash and behaving like the other non-TLS stack_chk_guard architectures.
> 
> One way to make it not crash on x86_64 could be to delay ifunc resolution (which would then mean making sure that TLS doesn't use ifunc'd functions, e.g. memcpy) and bring it on par with the rest of the architectures.

Is it something preventing us to make x86 follows other architectures in this
regard? 

> 
> I'm also considering proposing that gcc skips over ifunc resolvers for stack protection, at least in the static case.  The dynamic case happens to work somehow, I am yet to conclude that it's by design.

Not sure if adding exceptions for specific cases is the best approach,
the users would expect that such mitigations to work regardless of
compilation/linking model (although some do require extra linker/runtime
support).  My take is if we could fix on glibc we should aim for it.

> 
>> Florian stated on comment #2 that “all” is very unlikely to add
>> additional protection and this basically adds *another* undocumented
>> ifunc restriction. And it seems likely that distributions like Gentoo
>> will just use 'strong' instead of 'all'.
> 
> AFAICT Gentoo decided to use 'strong' instead of 'all' because of Florian's comment.
> 
>> So, what would be the implications of limiting stack protection to
>> 'strong' instead of 'all' for ABIs with ifunc? Is this issue only
>> redistricted to some ABIs (the reported indicates that only x86_64
>> is affected)?
> 
> It's not the glibc build that's affected though, at least not to an extent that we ought to care.  What's broken is our support with binaries that were built with stack-protector-all *and* ifuncs.  This patch implies that we don't support it; all we need to do is document this somewhere and perhaps fix gcc to never emit the prologue/epilogue for ifunc resolvers.

Yeah, I understood that and my concern is if users do start to use
ifunc more profusely they will need to keep adding the 
inhibit_stack_protector on resolvers to work around this issue.

Fixing the ifunc resolution order also might simplify the code
and avoid adding the inhibit_stack_protector on multiples files.