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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170322064812.GA9848@gmail.com>
Date:   Wed, 22 Mar 2017 07:48:12 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Borislav Petkov <bp@...en8.de>, stable@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] selftests/x86/ldt_gdt_32: Work around a glibc sigaction
 bug


* Andy Lutomirski <luto@...nel.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@...r.kernel.org
> Signed-off-by: Andy Lutomirski <luto@...nel.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