[PATCH 1/2] powerpc/signal: Fix handling of SA_RESTORER sigaction flag

[PATCH 1/2] powerpc/signal: Fix handling of SA_RESTORER sigaction flag

[Christophe Leroy]

powerpc advertises support of SA_RESTORER sigaction flag.

Make it the truth.

Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/signal_32.c | 8 ++++++--
 arch/powerpc/kernel/signal_64.c | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 0608581967f0..cf3da1386595 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -769,7 +769,9 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	}
 
 	/* Save user registers on the stack */
-	if (tsk->mm->context.vdso) {
+	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
+		tramp = (unsigned long)ksig->ka.sa.sa_restorer;
+	} else if (tsk->mm->context.vdso) {
 		tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp_rt32);
 	} else {
 		tramp = (unsigned long)mctx->mc_pad;
@@ -865,7 +867,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	else
 		unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
 
-	if (tsk->mm->context.vdso) {
+	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
+		tramp = (unsigned long)ksig->ka.sa.sa_restorer;
+	} else if (tsk->mm->context.vdso) {
 		tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp32);
 	} else {
 		tramp = (unsigned long)mctx->mc_pad;
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 1831bba0582e..fb31a334aca6 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -910,7 +910,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	tsk->thread.fp_state.fpscr = 0;
 
 	/* Set up to return from userspace. */
-	if (tsk->mm->context.vdso) {
+	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
+		regs_set_return_ip(regs, (unsigned long)ksig->ka.sa.sa_restorer);
+	} else if (tsk->mm->context.vdso) {
 		regs_set_return_ip(regs, VDSO64_SYMBOL(tsk->mm->context.vdso, sigtramp_rt64));
 	} else {
 		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
-- 
2.25.0

[PATCH RFC 2/2] powerpc/signal: Retire signal trampoline on stack

[Christophe Leroy]

The signal trampoline is either:
- As specified by the caller via SA_RESTORER
- In VDSO if VDSO is properly mapped
- Fallback on user stack

However, nowadays user stack is mapped non executable by default
so the fallback will generate an exec fault.

All other architectures having VDSO except x86 and sh don't install
any fallback trampoline on stack.

Simplify the code by doing the same, remove signal trampoline
on stack. If VDSO is not mapped, a NULL like address will be set
and the user app will gently fault.

If a user application explicitly want's to unmap VDSO, it can
still provide an SA_RESTORER.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/signal_32.c  | 26 +++++++++-------------
 arch/powerpc/kernel/signal_64.c  | 38 ++++----------------------------
 arch/powerpc/perf/callchain_32.c |  5 -----
 arch/powerpc/perf/callchain_64.c |  2 --
 4 files changed, 14 insertions(+), 57 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index cf3da1386595..366d07cb42da 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -769,16 +769,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	}
 
 	/* Save user registers on the stack */
-	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
+	if (ksig->ka.sa.sa_flags & SA_RESTORER)
 		tramp = (unsigned long)ksig->ka.sa.sa_restorer;
-	} else if (tsk->mm->context.vdso) {
+	else if (tsk->mm->context.vdso)
 		tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp_rt32);
-	} else {
-		tramp = (unsigned long)mctx->mc_pad;
-		unsafe_put_user(PPC_RAW_LI(_R0, __NR_rt_sigreturn), &mctx->mc_pad[0], failed);
-		unsafe_put_user(PPC_RAW_SC(), &mctx->mc_pad[1], failed);
-		asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
-	}
+	else
+		tramp = 0;
+
 	unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
 
 	user_access_end();
@@ -867,16 +864,13 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	else
 		unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
 
-	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
+	if (ksig->ka.sa.sa_flags & SA_RESTORER)
 		tramp = (unsigned long)ksig->ka.sa.sa_restorer;
-	} else if (tsk->mm->context.vdso) {
+	else if (tsk->mm->context.vdso)
 		tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp32);
-	} else {
-		tramp = (unsigned long)mctx->mc_pad;
-		unsafe_put_user(PPC_RAW_LI(_R0, __NR_sigreturn), &mctx->mc_pad[0], failed);
-		unsafe_put_user(PPC_RAW_SC(), &mctx->mc_pad[1], failed);
-		asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
-	}
+	else
+		tramp = 0;
+
 	user_access_end();
 
 	regs->link = tramp;
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index fb31a334aca6..39522ebd1137 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -610,32 +610,6 @@ static long restore_tm_sigcontexts(struct task_struct *tsk, struct sigcontext __
 }
 #endif
 
-/*
- * Setup the trampoline code on the stack
- */
-static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
-{
-	int i;
-	long err = 0;
-
-	/* Call the handler and pop the dummy stackframe*/
-	err |= __put_user(PPC_RAW_BCTRL(), &tramp[0]);
-	err |= __put_user(PPC_RAW_ADDI(_R1, _R1, __SIGNAL_FRAMESIZE), &tramp[1]);
-
-	err |= __put_user(PPC_RAW_LI(_R0, syscall), &tramp[2]);
-	err |= __put_user(PPC_RAW_SC(), &tramp[3]);
-
-	/* Minimal traceback info */
-	for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++)
-		err |= __put_user(0, &tramp[i]);
-
-	if (!err)
-		flush_icache_range((unsigned long) &tramp[0],
-			   (unsigned long) &tramp[TRAMP_SIZE]);
-
-	return err;
-}
-
 /*
  * Userspace code may pass a ucontext which doesn't include VSX added
  * at the end.  We need to check for this case.
@@ -910,16 +884,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	tsk->thread.fp_state.fpscr = 0;
 
 	/* Set up to return from userspace. */
-	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
+	if (ksig->ka.sa.sa_flags & SA_RESTORER)
 		regs_set_return_ip(regs, (unsigned long)ksig->ka.sa.sa_restorer);
-	} else if (tsk->mm->context.vdso) {
+	else if (tsk->mm->context.vdso)
 		regs_set_return_ip(regs, VDSO64_SYMBOL(tsk->mm->context.vdso, sigtramp_rt64));
-	} else {
-		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
-		if (err)
-			goto badframe;
-		regs_set_return_ip(regs, (unsigned long) &frame->tramp[0]);
-	}
+	else
+		regs->nip = 0;
 
 	/* Allocate a dummy caller frame for the signal handler. */
 	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
diff --git a/arch/powerpc/perf/callchain_32.c b/arch/powerpc/perf/callchain_32.c
index b83c47b7947f..8ab4663cef50 100644
--- a/arch/powerpc/perf/callchain_32.c
+++ b/arch/powerpc/perf/callchain_32.c
@@ -57,8 +57,6 @@ struct rt_signal_frame_32 {
 
 static int is_sigreturn_32_address(unsigned int nip, unsigned int fp)
 {
-	if (nip == fp + offsetof(struct signal_frame_32, mctx.mc_pad))
-		return 1;
 	if (current->mm->context.vdso &&
 	    nip == VDSO32_SYMBOL(current->mm->context.vdso, sigtramp32))
 		return 1;
@@ -67,9 +65,6 @@ static int is_sigreturn_32_address(unsigned int nip, unsigned int fp)
 
 static int is_rt_sigreturn_32_address(unsigned int nip, unsigned int fp)
 {
-	if (nip == fp + offsetof(struct rt_signal_frame_32,
-				 uc.uc_mcontext.mc_pad))
-		return 1;
 	if (current->mm->context.vdso &&
 	    nip == VDSO32_SYMBOL(current->mm->context.vdso, sigtramp_rt32))
 		return 1;
diff --git a/arch/powerpc/perf/callchain_64.c b/arch/powerpc/perf/callchain_64.c
index 8d0df4226328..8c7078ff67c2 100644
--- a/arch/powerpc/perf/callchain_64.c
+++ b/arch/powerpc/perf/callchain_64.c
@@ -66,8 +66,6 @@ struct signal_frame_64 {
 
 static int is_sigreturn_64_address(unsigned long nip, unsigned long fp)
 {
-	if (nip == fp + offsetof(struct signal_frame_64, tramp))
-		return 1;
 	if (current->mm->context.vdso &&
 	    nip == VDSO64_SYMBOL(current->mm->context.vdso, sigtramp_rt64))
 		return 1;
-- 
2.25.0

Re: [PATCH 1/2] powerpc/signal: Fix handling of SA_RESTORER sigaction flag

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> powerpc advertises support of SA_RESTORER sigaction flag.
>
> Make it the truth.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/signal_32.c | 8 ++++++--
>  arch/powerpc/kernel/signal_64.c | 4 +++-
>  2 files changed, 9 insertions(+), 3 deletions(-)

Hi Christophe,

I dug into the history a bit on this.

The 32-bit port originally did not define SA_RESTORER in
include/asm-ppc/signal.h, but it was added in 2.1.79.

  https://github.com/mpe/linux-fullhistory/commit/4e7e9c0d54ff5725a73d2210a950f8bc0f225073

That commit added SA_RESTORER to the header, added code to get/set it in
sys_sigaction(), but didn't add any code to use it for signal delivery.


The 64-bit port was merged with SA_RESTORER already defined in
include/asm-ppc64/signal.h:

  https://github.com/mpe/linux-fullhistory/commit/c3aa9878533e724f639852c3d951e6a169e04081
  
Similarly there was code to set/get it in sys_sigaction(), but no code
to use it for signal delivery.


Later the two ports were merged, and the headers were moved and
disintegrated into uapi, so we end up today with SA_RESTORER defined in
arch/powerpc/include/uapi/asm/signal.h, but no code to use it.

So essentially we've had SA_RESTORER defined since ancient kernels, but
never actually supported using it for anything.


One problem with enabling it now is there's no way for userspace to
determine if it's on a fixed kernel or not. That makes it unusable for
userspace, unless it does version checks, or is happy to break on all
old kernels (not likely). We could solve that by defining
SA_RESTORER_FIXED or something, but that's slightly gross.

It's also described in the man page as "Not intended for application
use", ie. it's intended for use by libc. I'm not sure there's any value
in adding support for it to the kernel unless we know there's interest
from glibc/musl in using it.

So my inclination is that we should *not* add support for it, rather we
should leave it unimplemented and remove SA_RESTORER from the header.
There's precedent in riscv for not supporting it at all.

cheers



> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 0608581967f0..cf3da1386595 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -769,7 +769,9 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>  	}
>  
>  	/* Save user registers on the stack */
> -	if (tsk->mm->context.vdso) {
> +	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
> +		tramp = (unsigned long)ksig->ka.sa.sa_restorer;
> +	} else if (tsk->mm->context.vdso) {
>  		tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp_rt32);
>  	} else {
>  		tramp = (unsigned long)mctx->mc_pad;
> @@ -865,7 +867,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>  	else
>  		unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
>  
> -	if (tsk->mm->context.vdso) {
> +	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
> +		tramp = (unsigned long)ksig->ka.sa.sa_restorer;
> +	} else if (tsk->mm->context.vdso) {
>  		tramp = VDSO32_SYMBOL(tsk->mm->context.vdso, sigtramp32);
>  	} else {
>  		tramp = (unsigned long)mctx->mc_pad;
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 1831bba0582e..fb31a334aca6 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -910,7 +910,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	tsk->thread.fp_state.fpscr = 0;
>  
>  	/* Set up to return from userspace. */
> -	if (tsk->mm->context.vdso) {
> +	if (ksig->ka.sa.sa_flags & SA_RESTORER) {
> +		regs_set_return_ip(regs, (unsigned long)ksig->ka.sa.sa_restorer);
> +	} else if (tsk->mm->context.vdso) {
>  		regs_set_return_ip(regs, VDSO64_SYMBOL(tsk->mm->context.vdso, sigtramp_rt64));
>  	} else {
>  		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
> -- 
> 2.25.0

Re: [PATCH 1/2] powerpc/signal: Fix handling of SA_RESTORER sigaction flag

[Christophe Leroy]

Le 04/02/2022 à 11:22, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> powerpc advertises support of SA_RESTORER sigaction flag.
>>
>> Make it the truth.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/kernel/signal_32.c | 8 ++++++--
>>   arch/powerpc/kernel/signal_64.c | 4 +++-
>>   2 files changed, 9 insertions(+), 3 deletions(-)
> 
> Hi Christophe,
> 
> I dug into the history a bit on this.
> 
> The 32-bit port originally did not define SA_RESTORER in
> include/asm-ppc/signal.h, but it was added in 2.1.79.
> 
>    https://github.com/mpe/linux-fullhistory/commit/4e7e9c0d54ff5725a73d2210a950f8bc0f225073
> 
> That commit added SA_RESTORER to the header, added code to get/set it in
> sys_sigaction(), but didn't add any code to use it for signal delivery.
> 
> 
> The 64-bit port was merged with SA_RESTORER already defined in
> include/asm-ppc64/signal.h:
> 
>    https://github.com/mpe/linux-fullhistory/commit/c3aa9878533e724f639852c3d951e6a169e04081
>    
> Similarly there was code to set/get it in sys_sigaction(), but no code
> to use it for signal delivery.
> 
> 
> Later the two ports were merged, and the headers were moved and
> disintegrated into uapi, so we end up today with SA_RESTORER defined in
> arch/powerpc/include/uapi/asm/signal.h, but no code to use it.
> 
> So essentially we've had SA_RESTORER defined since ancient kernels, but
> never actually supported using it for anything.
> 
> 
> One problem with enabling it now is there's no way for userspace to
> determine if it's on a fixed kernel or not. That makes it unusable for
> userspace, unless it does version checks, or is happy to break on all
> old kernels (not likely). We could solve that by defining
> SA_RESTORER_FIXED or something, but that's slightly gross.
> 
> It's also described in the man page as "Not intended for application
> use", ie. it's intended for use by libc. I'm not sure there's any value
> in adding support for it to the kernel unless we know there's interest
> from glibc/musl in using it.
> 
> So my inclination is that we should *not* add support for it, rather we
> should leave it unimplemented and remove SA_RESTORER from the header.
> There's precedent in riscv for not supporting it at all.
> 

Nowadays, stacks are mapped noexec, so the fallback stack trampoline 
cannot work anymore. If a process doesn't want exec stack and doesn't 
want to map the VDSO, the SA_RESTORER is the only alternative to get 
signal working.

On several architectures including arm64 and s390 only VDSO and 
SA_RESTORER are supported, on stack signal trampoline is not supported 
anymore.

So my idea was to first implement SA_RESTORER and then as a second step 
to retire the on stack signal trampoline which has become useless with 
noexec stacks.

See 
https://elixir.bootlin.com/linux/v5.17-rc1/source/arch/arm64/kernel/signal.c#L761

or 
https://elixir.bootlin.com/linux/v5.17-rc1/source/arch/s390/kernel/signal.c#L337

Christophe