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: [email protected]
Signed-off-by: Andy Lutomirski <[email protected]>
---
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
+
+/*
+ * 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;
+};
+#endif
+
static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
int flags)
{
@@ -420,6 +438,24 @@ static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
if (sigaction(sig, &sa, 0))
err(1, "sigaction");
+#ifdef __i386__
+ struct fake_ksigaction ksa;
+ 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");
+ }
+ }
+#endif
}
static jmp_buf jmpbuf;
--
2.9.3
* Andy Lutomirski <[email protected]> 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: [email protected]
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
>
> 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;
};
> +#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?
Thanks,
Ingo
On Tue, Mar 21, 2017 at 11:48 PM, Ingo Molnar <[email protected]> wrote:
>
> * Andy Lutomirski <[email protected]> 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: [email protected]
>> Signed-off-by: Andy Lutomirski <[email protected]>
>> ---
>>
>> 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