Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964903AbdCVQq3 (ORCPT ); Wed, 22 Mar 2017 12:46:29 -0400 Received: from mail-vk0-f42.google.com ([209.85.213.42]:35849 "EHLO mail-vk0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964873AbdCVQqW (ORCPT ); Wed, 22 Mar 2017 12:46:22 -0400 MIME-Version: 1.0 In-Reply-To: <20170322064812.GA9848@gmail.com> References: <371a5620248568efaf31dd9d897af3775725d9b8.1490114317.git.luto@kernel.org> <20170322064812.GA9848@gmail.com> From: Andy Lutomirski Date: Wed, 22 Mar 2017 09:37:13 -0700 Message-ID: Subject: Re: [PATCH] selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug To: Ingo Molnar Cc: Andy Lutomirski , X86 ML , "linux-kernel@vger.kernel.org" , Borislav Petkov , stable , Thomas Gleixner , "H. Peter Anvin" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3511 Lines: 103 On Tue, Mar 21, 2017 at 11:48 PM, Ingo Molnar wrote: > > * Andy Lutomirski 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 >> --- >> >> 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