[PATCH] parisc: fix crash with signals and alloca

[PATCH] parisc: fix crash with signals and alloca

[Mikulas Patocka]

Hi

I was debugging some crashes on parisc and I found out that there is a 
crash possibility if a function using alloca is interrupted by a signal. 
The reason for the crash is that the gcc alloca implementation leaves 
garbage in the upper 32 bits of the sp register. This normally doesn't 
matter (the upper bits are ignored because the PSW W-bit is clear), 
however the signal delivery routine in the kernel uses full 64 bits of sp 
and it fails with -EFAULT if the upper 32 bits are not zero.

I created this program that demonstrates the problem:

#include <stdlib.h>
#include <unistd.h>
#include <signal.h>
#include <alloca.h>

static __attribute__((noinline,noclone)) void aa(int *size)
{
	void * volatile p = alloca(-*size);
	while (1) ;
}

static void handler(int sig)
{
	write(1, "signal delivered\n", 17);
	_exit(0);
}

int main(void)
{
	int size = -0x100;
	signal(SIGALRM, handler);
	alarm(1);
	aa(&size);
}

If you compile it with optimizations, it will crash.
The "aa" function has this disassembly:

000106a0 <aa>:
   106a0:       08 03 02 41     copy r3,r1
   106a4:       08 1e 02 43     copy sp,r3
   106a8:       6f c1 00 80     stw,ma r1,40(sp)
   106ac:       37 dc 3f c1     ldo -20(sp),ret0
   106b0:       0c 7c 12 90     stw ret0,8(r3)
   106b4:       0f 40 10 9c     ldw 0(r26),ret0		; ret0 = 0x00000000FFFFFF00
   106b8:       97 9c 00 7e     subi 3f,ret0,ret0	; ret0 = 0xFFFFFFFF0000013F
   106bc:       d7 80 1c 1a     depwi 0,31,6,ret0	; ret0 = 0xFFFFFFFF00000100
   106c0:       0b 9e 0a 1e     add,l sp,ret0,sp	;   sp = 0xFFFFFFFFxxxxxxxx
   106c4:       e8 1f 1f f7     b,l,n 106c4 <aa+0x24>,r0

This patch fixes the bug by truncating the "frame" variable to 32 bits.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 arch/parisc/kernel/signal.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-5.12/arch/parisc/kernel/signal.c
===================================================================
--- linux-5.12.orig/arch/parisc/kernel/signal.c	2021-08-29 19:06:33.000000000 +0200
+++ linux-5.12/arch/parisc/kernel/signal.c	2021-08-29 21:17:55.000000000 +0200
@@ -246,6 +246,11 @@ setup_rt_frame(struct ksignal *ksig, sig
 	
 #ifdef CONFIG_64BIT
 
+	if (is_compat_task()) {
+		/* The gcc alloca implementation leaves garbage in the upper 32 bits of sp.*/
+		frame = (struct rt_sigframe __user *)(unsigned long)ptr_to_compat(frame);
+	}
+
 	compat_frame = (struct compat_rt_sigframe __user *)frame;
 	
 	if (is_compat_task()) {

Re: [PATCH] parisc: fix crash with signals and alloca

[Helge Deller]

On 8/29/21 9:50 PM, Mikulas Patocka wrote:
> Hi
>
> I was debugging some crashes on parisc and I found out that there is a
> crash possibility if a function using alloca is interrupted by a signal.
> The reason for the crash is that the gcc alloca implementation leaves
> garbage in the upper 32 bits of the sp register. This normally doesn't
> matter (the upper bits are ignored because the PSW W-bit is clear),
> however the signal delivery routine in the kernel uses full 64 bits of sp
> and it fails with -EFAULT if the upper 32 bits are not zero.
>
> I created this program that demonstrates the problem:
>
> #include <stdlib.h>
> #include <unistd.h>
> #include <signal.h>
> #include <alloca.h>
>
> static __attribute__((noinline,noclone)) void aa(int *size)
> {
> 	void * volatile p = alloca(-*size);
> 	while (1) ;
> }
>
> static void handler(int sig)
> {
> 	write(1, "signal delivered\n", 17);
> 	_exit(0);
> }
>
> int main(void)
> {
> 	int size = -0x100;
> 	signal(SIGALRM, handler);
> 	alarm(1);
> 	aa(&size);
> }
>
> If you compile it with optimizations, it will crash.
> The "aa" function has this disassembly:
>
> 000106a0 <aa>:
>     106a0:       08 03 02 41     copy r3,r1
>     106a4:       08 1e 02 43     copy sp,r3
>     106a8:       6f c1 00 80     stw,ma r1,40(sp)
>     106ac:       37 dc 3f c1     ldo -20(sp),ret0
>     106b0:       0c 7c 12 90     stw ret0,8(r3)
>     106b4:       0f 40 10 9c     ldw 0(r26),ret0		; ret0 = 0x00000000FFFFFF00
>     106b8:       97 9c 00 7e     subi 3f,ret0,ret0	; ret0 = 0xFFFFFFFF0000013F
>     106bc:       d7 80 1c 1a     depwi 0,31,6,ret0	; ret0 = 0xFFFFFFFF00000100
>     106c0:       0b 9e 0a 1e     add,l sp,ret0,sp	;   sp = 0xFFFFFFFFxxxxxxxx
>     106c4:       e8 1f 1f f7     b,l,n 106c4 <aa+0x24>,r0
>
> This patch fixes the bug by truncating the "frame" variable to 32 bits.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
>
> ---
>   arch/parisc/kernel/signal.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> Index: linux-5.12/arch/parisc/kernel/signal.c
> ===================================================================
> --- linux-5.12.orig/arch/parisc/kernel/signal.c	2021-08-29 19:06:33.000000000 +0200
> +++ linux-5.12/arch/parisc/kernel/signal.c	2021-08-29 21:17:55.000000000 +0200
> @@ -246,6 +246,11 @@ setup_rt_frame(struct ksignal *ksig, sig
>
>   #ifdef CONFIG_64BIT
>
> +	if (is_compat_task()) {
> +		/* The gcc alloca implementation leaves garbage in the upper 32 bits of sp.*/
> +		frame = (struct rt_sigframe __user *)(unsigned long)ptr_to_compat(frame);
> +	}
> +


Very good catch!!!!
I'm just wondering if we miss to clip the sp somewhere earlier in the
kernel call chain (e.g. in the irq/entry handlers), or if the clipping
should be done somewhere else, e.g. some lines above here...

Helge




>   	compat_frame = (struct compat_rt_sigframe __user *)frame;
>
>   	if (is_compat_task()) {
>

Re: [PATCH] parisc: fix crash with signals and alloca

[John David Anglin]

On 2021-08-29 4:46 p.m., Helge Deller wrote:
> I'm just wondering if we miss to clip the sp somewhere earlier in the
> kernel call chain (e.g. in the irq/entry handlers), or if the clipping
> should be done somewhere else, e.g. some lines above here...
Maybe sp should be clipped in get_stack_use_cr30?

Dave

-- 
John David Anglin  dave.anglin@bell.net

[PATCH v2] parisc: fix crash with signals and alloca

[Mikulas Patocka]

On Sun, 29 Aug 2021, Helge Deller wrote:

> > Index: linux-5.12/arch/parisc/kernel/signal.c
> > ===================================================================
> > --- linux-5.12.orig/arch/parisc/kernel/signal.c	2021-08-29
> > 19:06:33.000000000 +0200
> > +++ linux-5.12/arch/parisc/kernel/signal.c	2021-08-29 21:17:55.000000000
> > +0200
> > @@ -246,6 +246,11 @@ setup_rt_frame(struct ksignal *ksig, sig
> > 
> >   #ifdef CONFIG_64BIT
> > 
> > +	if (is_compat_task()) {
> > +		/* The gcc alloca implementation leaves garbage in the upper
> > 32 bits of sp.*/
> > +		frame = (struct rt_sigframe __user *)(unsigned
> > long)ptr_to_compat(frame);
> > +	}
> > +
> 
> 
> Very good catch!!!!
> I'm just wondering if we miss to clip the sp somewhere earlier in the
> kernel call chain (e.g. in the irq/entry handlers), or if the clipping
> should be done somewhere else, e.g. some lines above here...
> 
> Helge

You are right - we should clip earlier. I've realized that my patch wasn't 
correct - when we go down this path:
	get_sigframe -> sas_ss_flags -> on_sig_stack -> __on_sig_stack
we already need clipped sp, so that __on_sig_stack can properly determine 
if we are on the signal stack or not.

Here I'm sending a second version of the patch that clips the sp earlier:





From: Mikulas Patocka <mpatocka@redhat.com>

I was debugging some crashes on parisc and I found out that there is a
crash possibility if a function using alloca is interrupted by a signal.
The reason for the crash is that the gcc alloca implementation leaves
garbage in the upper 32 bits of the sp register. This normally doesn't
matter (the upper bits are ignored because the PSW W-bit is clear),
however the signal delivery routine in the kernel uses full 64 bits of sp
and it fails with -EFAULT if the upper 32 bits are not zero.

I created this program that demonstrates the problem:

#include <stdlib.h>
#include <unistd.h>
#include <signal.h>
#include <alloca.h>

static __attribute__((noinline,noclone)) void aa(int *size)
{
	void * volatile p = alloca(-*size);
	while (1) ;
}

static void handler(int sig)
{
	write(1, "signal delivered\n", 17);
	_exit(0);
}

int main(void)
{
	int size = -0x100;
	signal(SIGALRM, handler);
	alarm(1);
	aa(&size);
}

If you compile it with optimizations, it will crash.
The "aa" function has this disassembly:

000106a0 <aa>:
   106a0:       08 03 02 41     copy r3,r1
   106a4:       08 1e 02 43     copy sp,r3
   106a8:       6f c1 00 80     stw,ma r1,40(sp)
   106ac:       37 dc 3f c1     ldo -20(sp),ret0
   106b0:       0c 7c 12 90     stw ret0,8(r3)
   106b4:       0f 40 10 9c     ldw 0(r26),ret0		; ret0 = 0x00000000FFFFFF00
   106b8:       97 9c 00 7e     subi 3f,ret0,ret0	; ret0 = 0xFFFFFFFF0000013F
   106bc:       d7 80 1c 1a     depwi 0,31,6,ret0	; ret0 = 0xFFFFFFFF00000100
   106c0:       0b 9e 0a 1e     add,l sp,ret0,sp	;   sp = 0xFFFFFFFFxxxxxxxx
   106c4:       e8 1f 1f f7     b,l,n 106c4 <aa+0x24>,r0

This patch fixes the bug by truncating the "usp" variable to 32 bits.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 arch/parisc/kernel/signal.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-5.12/arch/parisc/kernel/signal.c
===================================================================
--- linux-5.12.orig/arch/parisc/kernel/signal.c	2021-08-29 19:06:33.000000000 +0200
+++ linux-5.12/arch/parisc/kernel/signal.c	2021-08-30 11:21:12.000000000 +0200
@@ -237,6 +237,12 @@ setup_rt_frame(struct ksignal *ksig, sig
 #endif
 	
 	usp = (regs->gr[30] & ~(0x01UL));
+#ifdef CONFIG_64BIT
+	if (is_compat_task()) {
+		/* The gcc alloca implementation leaves garbage in the upper 32 bits of sp */
+		usp = (compat_uint_t)usp;
+	}
+#endif
 	/*FIXME: frame_size parameter is unused, remove it. */
 	frame = get_sigframe(&ksig->ka, usp, sizeof(*frame));