linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/7] powerpc/signal64: Copy siginfo before changing regs->nip
@ 2021-06-15  6:40 Christophe Leroy
  2021-06-15  6:40 ` [PATCH 2/7] powerpc/signal64: Don't read sigaction arguments back from user memory Christophe Leroy
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Christophe Leroy @ 2021-06-15  6:40 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
  Cc: linux-kernel, linuxppc-dev

From: Michael Ellerman <mpe@ellerman.id.au>

In commit 96d7a4e06fab ("powerpc/signal64: Rewrite handle_rt_signal64()
to minimise uaccess switches") the 64-bit signal code was rearranged to
use user_write_access_begin/end().

As part of that change the call to copy_siginfo_to_user() was moved
later in the function, so that it could be done after the
user_write_access_end().

In particular it was moved after we modify regs->nip to point to the
signal trampoline. That means if copy_siginfo_to_user() fails we exit
handle_rt_signal64() with an error but with regs->nip modified, whereas
previously we would not modify regs->nip until the copy succeeded.

Returning an error from signal delivery but with regs->nip updated
leaves the process in a sort of half-delivered state. We do immediately
force a SEGV in signal_setup_done(), called from do_signal(), so the
process should never run in the half-delivered state.

However that SEGV is not delivered until we've gone around to
do_notify_resume() again, so it's possible some tracing could observe
the half-delivered state.

There are other cases where we fail signal delivery with regs partly
updated, eg. the write to newsp and SA_SIGINFO, but the latter at least
is very unlikely to fail as it reads back from the frame we just wrote
to.

Looking at other arches they seem to be more careful about leaving regs
unchanged until the copy operations have succeeded, and in general that
seems like good hygenie.

So although the current behaviour is not cleary buggy, it's also not
clearly correct. So move the call to copy_siginfo_to_user() up prior to
the modification of regs->nip, which is closer to the old behaviour, and
easier to reason about.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/signal_64.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index dca66481d0c2..f9e1f5428b9e 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -902,6 +902,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
 	user_write_access_end();
 
+	/* Save the siginfo outside of the unsafe block. */
+	if (copy_siginfo_to_user(&frame->info, &ksig->info))
+		goto badframe;
+
 	/* Make sure signal handler doesn't get spurious FP exceptions */
 	tsk->thread.fp_state.fpscr = 0;
 
@@ -915,11 +919,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		regs->nip = (unsigned long) &frame->tramp[0];
 	}
 
-
-	/* Save the siginfo outside of the unsafe block. */
-	if (copy_siginfo_to_user(&frame->info, &ksig->info))
-		goto badframe;
-
 	/* Allocate a dummy caller frame for the signal handler. */
 	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
 	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-06-15  7:29 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15  6:40 [PATCH 1/7] powerpc/signal64: Copy siginfo before changing regs->nip Christophe Leroy
2021-06-15  6:40 ` [PATCH 2/7] powerpc/signal64: Don't read sigaction arguments back from user memory Christophe Leroy
2021-06-15  6:40 ` [PATCH 3/7] powerpc/signal64: Access function descriptor with user access block Christophe Leroy
2021-06-15  6:41 ` [PATCH 4/7] powerpc/signal: Include the new stack frame inside the " Christophe Leroy
2021-06-15  6:41 ` [PATCH 5/7] signal: Add unsafe_copy_siginfo_to_user() Christophe Leroy
2021-06-15  6:52   ` Christoph Hellwig
2021-06-15  7:03     ` Christophe Leroy
2021-06-15  7:21       ` Christoph Hellwig
2021-06-15  7:28         ` Christophe Leroy
2021-06-15  6:41 ` [PATCH 6/7] powerpc/uaccess: Add unsafe_clear_user() Christophe Leroy
2021-06-15  6:53   ` Christoph Hellwig
2021-06-15  7:10     ` Christophe Leroy
2021-06-15  6:41 ` [PATCH 7/7] powerpc/signal: Use unsafe_copy_siginfo_to_user() Christophe Leroy
2021-06-15  6:55   ` Christoph Hellwig
2021-06-15  7:00     ` Christophe Leroy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).