On Tue, Mar 21, 2017 at 11:48 PM, Ingo Molnar <mingo@xxxxxxxxxx> wrote: > > * Andy Lutomirski <luto@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx >> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx> >> --- >> >> 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