[RFC PATCH] powerpc/signal: sanitise PT_NIP and sa_handler low bits

From: Nicholas Piggin <npiggin@gmail.com>
Date: 2021-11-30 07:30:28

The bottom 2 bits of NIP are ignored when RFI returns with SRR0 = NIP,
so regs->nip does not correspond to the actual return address if either
of those bits are set. Further, these bits are reserved in SRR0 so they
should not be set. Sanitize PT_NIP from signal handlers to ensure they
can't be set by userspace, this also keeps the low 2 bit of TFHAR clear,
which are similarly reserved. 32-bit signal delivery returns directly to
the handler, so sa_handler is sanitised similarly there.

This can cause a bug when CONFIG_PPC_RFI_SRR_DEBUG=y on a processor that
does not implement the 2 low bits of SRR0 (always read back 0) because
SRR0 will not match regs->nip. This was caught by sigfuz, but a simple
reproducer follows.

  #include <stdlib.h>
  #include <signal.h>
  #include <ucontext.h>

  static void trap_signal_handler(int signo, siginfo_t *si, void *uc)
  {
      ucontext_t *ucp = uc;
      ucp->uc_mcontext.gp_regs[PT_NIP] |= 3;
  }

  int main(void)
  {
      struct sigaction trap_sa;
      trap_sa.sa_flags = SA_SIGINFO;
      trap_sa.sa_sigaction = trap_signal_handler;
      sigaction(SIGUSR1, &trap_sa, NULL);
      raise(SIGUSR1);
      exit(EXIT_SUCCESS);
  }

Reported-by: Sachin Sant <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
I'm not entirely sure about the 32-bit / compat part. Or the 64-bit for
that matter except that it does seem to fix the bug caused by the test
program.

Thanks,
Nick

 arch/powerpc/kernel/signal_32.c | 23 ++++++++++++++++-------
 arch/powerpc/kernel/signal_64.c | 17 ++++++++++++-----
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 3e053e2fd6b6..5379bece8072 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c

@@ -116,7 +116,7 @@ __unsafe_restore_general_regs(struct pt_regs *regs, struct mcontext __user *sr)
 	int i;
 
 	for (i = 0; i <= PT_RESULT; i++) {
-		if ((i == PT_MSR) || (i == PT_SOFTE))
+		if ((i == PT_NIP) || (i == PT_MSR) || (i == PT_SOFTE))
 			continue;
 		unsafe_get_user(gregs[i], &sr->mc_gregs[i], failed);
 	}

@@ -156,7 +156,7 @@ static __always_inline
 int __unsafe_restore_general_regs(struct pt_regs *regs, struct mcontext __user *sr)
 {
 	/* copy up to but not including MSR */
-	unsafe_copy_from_user(regs, &sr->mc_gregs, PT_MSR * sizeof(elf_greg_t), failed);
+	unsafe_copy_from_user(regs, &sr->mc_gregs, PT_NIP * sizeof(elf_greg_t), failed);
 
 	/* copy from orig_r3 (the word after the MSR) up to the end */
 	unsafe_copy_from_user(&regs->orig_gpr3, &sr->mc_gregs[PT_ORIG_R3],

@@ -458,7 +458,7 @@ static long restore_user_regs(struct pt_regs *regs,
 			      struct mcontext __user *sr, int sig)
 {
 	unsigned int save_r2 = 0;
-	unsigned long msr;
+	unsigned long nip, msr;
 #ifdef CONFIG_VSX
 	int i;
 #endif

@@ -473,6 +473,9 @@ static long restore_user_regs(struct pt_regs *regs,
 		save_r2 = (unsigned int)regs->gpr[2];
 	unsafe_restore_general_regs(regs, sr, failed);
 	set_trap_norestart(regs);
+	unsafe_get_user(nip, &sr->mc_gregs[PT_NIP], failed);
+	nip &= ~3UL;
+	regs_set_return_ip(regs, nip);
 	unsafe_get_user(msr, &sr->mc_gregs[PT_MSR], failed);
 	if (!sig)
 		regs->gpr[2] = (unsigned long) save_r2;

@@ -560,7 +563,7 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 				 struct mcontext __user *sr,
 				 struct mcontext __user *tm_sr)
 {
-	unsigned long msr, msr_hi;
+	unsigned long nip, msr, msr_hi;
 	int i;
 
 	if (tm_suspend_disabled)

@@ -576,7 +579,9 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 		return 1;
 
 	unsafe_restore_general_regs(&current->thread.ckpt_regs, sr, failed);
-	unsafe_get_user(current->thread.tm_tfhar, &sr->mc_gregs[PT_NIP], failed);
+	unsafe_get_user(nip, &sr->mc_gregs[PT_NIP], failed);
+	nip &= ~3UL;
+	current->thread.tm_tfhar = nip;
 	unsafe_get_user(msr, &sr->mc_gregs[PT_MSR], failed);
 
 	/* Restore the previous little-endian mode */

@@ -646,6 +651,10 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 		current->thread.used_vsr = true;
 	}
 
+	unsafe_get_user(nip, &tm_sr->mc_gregs[PT_NIP], failed);
+	nip &= ~3UL;
+	regs_set_return_ip(regs, nip);
+
 	/* Get the top half of the MSR from the user context */
 	unsafe_get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR], failed);
 	msr_hi <<= 32;

@@ -801,7 +810,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	regs->gpr[4] = (unsigned long)&frame->info;
 	regs->gpr[5] = (unsigned long)&frame->uc;
 	regs->gpr[6] = (unsigned long)frame;
-	regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler);
+	regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler & ~3UL);
 	/* enter the signal handler in native-endian mode */
 	regs_set_return_msr(regs, (regs->msr & ~MSR_LE) | (MSR_KERNEL & MSR_LE));

@@ -889,7 +898,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	regs->gpr[1] = newsp;
 	regs->gpr[3] = ksig->sig;
 	regs->gpr[4] = (unsigned long) sc;
-	regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler);
+	regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler & ~3UL);
 	/* enter the signal handler in native-endian mode */
 	regs_set_return_msr(regs, (regs->msr & ~MSR_LE) | (MSR_KERNEL & MSR_LE));

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index d1e1fc0acbea..5ef24adb9803 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c

@@ -336,7 +336,7 @@ static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_
 	elf_vrreg_t __user *v_regs;
 #endif
 	unsigned long save_r13 = 0;
-	unsigned long msr;
+	unsigned long nip, msr;
 	struct pt_regs *regs = tsk->thread.regs;
 #ifdef CONFIG_VSX
 	int i;

@@ -350,7 +350,9 @@ static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_
 
 	/* copy the GPRs */
 	unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr), efault_out);
-	unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
+	unsafe_get_user(nip, &sc->gp_regs[PT_NIP], efault_out);
+	nip &= ~3UL;
+	regs_set_return_ip(regs, nip);
 	/* get MSR separately, transfer the LE bit if doing signal return */
 	unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
 	if (sig)

@@ -434,7 +436,7 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 	elf_vrreg_t __user *v_regs, *tm_v_regs;
 #endif
 	unsigned long err = 0;
-	unsigned long msr;
+	unsigned long nip, msr;
 	struct pt_regs *regs = tsk->thread.regs;
 #ifdef CONFIG_VSX
 	int i;

@@ -458,8 +460,13 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 	 * For the case of getting a signal and simply returning from it,
 	 * we don't need to re-copy them here.
 	 */
-	err |= __get_user(regs->nip, &tm_sc->gp_regs[PT_NIP]);
-	err |= __get_user(tsk->thread.tm_tfhar, &sc->gp_regs[PT_NIP]);
+	err |= __get_user(nip, &tm_sc->gp_regs[PT_NIP]);
+	nip &= ~3UL;
+	regs_set_return_ip(regs, nip);
+
+	err |= __get_user(nip, &sc->gp_regs[PT_NIP]);
+	nip &= ~3UL;
+	tsk->thread.tm_tfhar = nip;
 
 	/* get MSR separately, transfer the LE bit if doing signal return */
 	err |= __get_user(msr, &sc->gp_regs[PT_MSR]);

-- 
2.23.0

`h`	back out one level
`j`	next message in thread / next series-diff position
`k`	previous message in thread / previous series-diff position
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help