Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1605790 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug Andy Lutomirski <luto@kernel.org> - 2017-03-21 17:40 +0100
    Re: [PATCH] selftests/x86/ldt_gdt_32: Work around a glibc sigaction  bug Ingo Molnar <mingo@kernel.org> - 2017-03-22 07:50 +0100
      Re: [PATCH] selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug Andy Lutomirski <luto@amacapital.net> - 2017-03-22 17:50 +0100

#1605790 — [PATCH] selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug

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
+
+/*
+ * 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

[toc] | [next] | [standalone]

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

* 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;
};


> +#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

[toc] | [prev] | [next] | [standalone]

#1606739

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web