Re: [PATCH] selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug

[Andy Lutomirski <luto@amacapital.net>]

On Tue, Mar 21, 2017 at 11:48 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@kernel.org> wrote:
>
>> i386 glibc is buggy and calls the sigaction syscall incorrectly.
>> This is asymptomatic for normal programs, but it blows up on
>> programs that do evil things with segmentation.  ldt_gdt an example
>> of such an evil program.
>>
>> This doesn't appear to be a regression -- I think I just got lucky
>> with the uninitialized memory that glibc threw at the kernel when I
>> wrote the test.
>>
>> This hackish fix manually issues sigaction(2) syscalls to undo the
>> damage.  Without the fix, ldt_gdt_32 segfaults; with the fix, it
>> passes for me.
>>
>> See https://sourceware.org/bugzilla/show_bug.cgi?id=21269
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>
>> I'll see about factoring out sethandler(), etc into a separate file
>> soon.  In the mean time, this at least makes the test pass.
>>
>>  tools/testing/selftests/x86/ldt_gdt.c | 36 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/tools/testing/selftests/x86/ldt_gdt.c b/tools/testing/selftests/x86/ldt_gdt.c
>> index f6121612e769..18e6ae1f1bb6 100644
>> --- a/tools/testing/selftests/x86/ldt_gdt.c
>> +++ b/tools/testing/selftests/x86/ldt_gdt.c
>> @@ -409,6 +409,24 @@ static void *threadproc(void *ctx)
>>       }
>>  }
>>
>> +#ifdef __i386__
>> +
>> +#ifndef SA_RESTORE
>> +#define SA_RESTORER 0x04000000
>> +#endif
>
> This looks nicer IMHO:
>
> #ifndef SA_RESTORE
> # define SA_RESTORER 0x04000000
> #endif
>
>> +
>> +/*
>> + * The UAPI header calls this 'struct sigaction', which conflicts with
>> + * glibc.  Sigh.
>> + */
>> +struct fake_ksigaction {
>> +     void *handler;  /* the real type is nasty */
>> +        unsigned long sa_flags;
>> +        void (*sa_restorer)(void);
>> +     unsigned long sigset1, sigset2;
>> +};
>
> Please use tabs, not spaces. Also, don't merge types on the same line. I.e.
> something like:
>
> struct fake_ksigaction {
>         void *handler; /* the real type is nasty */
>         unsigned long sa_flags;
>         void (*sa_restorer)(void);
>         unsigned long sigset1;
>         unsigned long sigset2;
> };

Will improve.  Sorry about the spaces -- I cut-and-pasted some of
that, and apparently it got screwed up.

>
>
>> +#ifdef __i386__
>> +     struct fake_ksigaction ksa;
>
> Please either move this into a helper function or add a new block, we shouldn't
> declare new local variables C++ style. How come the compiler didn't warn about
> this? We should use the kernel build warnings.
>
>> +     if (syscall(SYS_rt_sigaction, sig, NULL, &ksa, 8) == 0) {
>> +             /*
>> +              * glibc has a nasty bug: it sometimes writes garbage to
>> +              * sa_restorer.  This interacts quite badly with anything
>> +              * that fiddles with SS because it can trigger legacy
>> +              * stack switching.  Patch it up.
>> +              */
>> +             printf("%d asdf %lx %p\n", sig, ksa.sa_flags, ksa.sa_restorer);
>> +             if (!(ksa.sa_flags & SA_RESTORER) && ksa.sa_restorer) {
>> +                     printf("asdffff\n");
>> +                     ksa.sa_restorer = NULL;
>> +                     if (syscall(SYS_rt_sigaction, sig, &ksa, NULL, 8) != 0)
>> +                             err(1, "rt_sigaction");
>
> What does the '8' stand for?

It's the one and only value of that parameter that's accepted.  I'll tidy it up.

--Andy