Re: --enable-stack-protector for glibc, v4, now with arm

[Nix <nix at esperi dot org dot uk>]

On 3 Mar 2016, nix@esperi.org.uk spake thusly:
> Looking at the prototypes of the functions in question, we see (dropping
> my additions):
>
> int
> internal_function attribute_hidden
> __pthread_mutex_unlock_usercnt (pthread_mutex_t *mutex, int decr)
>
> void
> internal_function
> __pthread_mutex_cond_lock_adjust (pthread_mutex_t *mutex)
>
> and looking at the assembler in
> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S, it is obvious
> enough even to someone with my extremely minimal x86 asm experience that
> this thing is not exactly feeling constrained by the normal way one
> calls functions

Well, no. That's because internal_function expands to -mregparm on x86,
which I quite forgot -- and the cause depends on this.

Look at line 313 of pthread_cond_wait.S. Here's the block around it:

        /* We need to go back to futex_wait.  If we're using requeue_pi, then
           release the mutex we had acquired and go back.  */
22:     movl    16(%esp), %edx
        test    %edx, %edx
        jz      8b

        /* Adjust the mutex values first and then unlock it.  The unlock
           should always succeed or else the kernel did not lock the mutex
           correctly.  */
        movl    dep_mutex(%ebx), %eax
        call    __pthread_mutex_cond_lock_adjust
        xorl    %edx, %edx
        call    __pthread_mutex_unlock_usercnt
        jmp     8b

Now that second call to __pthread_mutex_unlock_usercnt() is interesting,
because *no* effort is made to set up the first parameter, the mutex, in
%eax: it's assumed to persist from __pthread_mutex_cond_lock_adjust().
And indeed if you look at the generated x86 assembler for the
non-stack-protected version of __pthread_mutex_cond_lock_adjust(),
you'll see that %eax is consulted but is never modified at any point.
However, when stack-protecting, %eax is used in the epilogue to test the
stack canary, and is smashed in the process, wrecking the call to
__pthread_mutex_unlock_usercnt(). (The second argument, passed in %edx,
is zeroed right there, so no such dependency exists for it.)

pthread_cond_timedwait.S has identical code in label 15.

How should I put it, this is dodgy. This is really dodgy. It's depending
on the compiler's register allocation for regparm functions never
changing to clobber %eax, which, well, we've already seen how reliable
*that* is. It's caller-saved and the caller is not saving it!

Adding a single instruction to both files to reload %eax fixes it, at
minimal cost (this is already a fallback path), and lets us stack-
protect the entire C-side mutex unlocking code path, eliminating the
mystery. I'll do that in the next patch series.

I have also audited all the x86 code in glibc for signs of the same
malaise, and found one instance, in
sysdeps/mach/hurd/i386/____longjmp_chk.S: I fixed that by decorating
_hurd_self_sigstate with inhibit_stack_protector, both because I can't
test it easily and because it seemed far simpler than the converse (that
function is normally inlined in any case). That'll also be in the
next series.

(Testing now, on the usual combination of x86-32/64, sparc-32/64 and
ARM.)