Re: [PATCH] x86/shstk: Change SSP after user accesses

[Deepak Gupta <debug@rivosinc.com>]

Acked-by: Deepak Gupta <debug@rivosinc.com>

On Tue, Nov 07, 2023 at 10:22:51AM -0800, Rick Edgecombe wrote:
>When a signal is being delivered, the kernel needs to make accesses to
>userspace. These accesses could encounter an access error, in which case
>the signal delivery itself will trigger a segfault. Usually this would
>result in the kernel killing the process. But in the case of a SEGV signal
>handler being configured, the failure of the first signal delivery will
>result in *another* signal getting delivered. The second signal may
>succeed if another thread has resolved the issue that triggered the
>segfault (i.e. a well timed mprotect()/mmap()), or the second signal is
>being delivered to another stack (i.e. an alt stack).
>
>On x86, in the non-shadow stack case, all the accesses to userspace are
>done before changes to the registers (in pt_regs). The operation is
>aborted when an access error occurs, so although there may be writes done
>for the first signal, control flow changes for the signal (regs->ip,
>regs->sp, etc) are not committed until all the accesses have already
>completed successfully. This means that the second signal will be
>delivered as if it happened at the time of the first signal. It will
>effectively replace the first aborted signal, overwriting the half-written
>frame of the aborted signal. So on sigreturn from the second signal,
>control flow will resume happily from the point of control flow where the
>original signal was delivered.
>
>The problem is, when shadow stack is active, the shadow stack SSP
>register/MSR is updated *before* some of the userspace accesses. This
>means if the earlier accesses succeed and the later ones fail, the second
>signal will not be delivered at the same spot on the shadow stack as the
>first one. So on sigreturn from the second signal, the SSP will be
>pointing to the wrong location on the shadow stack (off by a frame).
>
>Pengfei privately reported that while using a shadow stack enabled glibc,
>the “signal06” test in the LTP test-suite hung. It turns out it is
>testing the above described double signal scenario. When this test was
>compiled with shadow stack, the first signal pushed a shadow stack
>sigframe, then the second pushed another. When the second signal was
>handled, the SSP was at the first shadow stack signal frame instead of
>the original location. The test then got stuck as the #CP from the twice
>incremented SSP was incorrect and generated segfaults in a loop.
>
>Fix this by adjusting the SSP register only after any userspace accesses,
>such that there can be no failures after the SSP is adjusted. Do this by
>moving the shadow stack sigframe push logic to happen after all other
>userspace accesses.
>
>Note, sigreturn (as supposed to the signal delivery dealt with in this
>patch) has ordering behavior that could lead to similar failures. The
>ordering issues there extend beyond shadow stack to include the alt stack
>restoration. Fixing that would require cross-arch changes, and the
>ordering today does not cause any known test or apps breakages. So leave
>it as is, for now.
>
>Cc: stable@vger.kernel.org
>Fixes: 05e36022c054 ("x86/shstk: Handle signals for shadow stack")
>Reported-by: Pengfei Xu <pengfei.xu@intel.com>
>Tested-by: Pengfei Xu <pengfei.xu@intel.com>
>Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>---
> arch/x86/kernel/signal_64.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
>index cacf2ede6217..23d8aaf8d9fd 100644
>--- a/arch/x86/kernel/signal_64.c
>+++ b/arch/x86/kernel/signal_64.c
>@@ -175,9 +175,6 @@ int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> 	frame = get_sigframe(ksig, regs, sizeof(struct rt_sigframe), &fp);
> 	uc_flags = frame_uc_flags(regs);
>
>-	if (setup_signal_shadow_stack(ksig))
>-		return -EFAULT;
>-
> 	if (!user_access_begin(frame, sizeof(*frame)))
> 		return -EFAULT;
>
>@@ -198,6 +195,9 @@ int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> 			return -EFAULT;
> 	}
>
>+	if (setup_signal_shadow_stack(ksig))
>+		return -EFAULT;
>+
> 	/* Set up registers for signal handler */
> 	regs->di = ksig->sig;
> 	/* In case the signal handler was declared without prototypes */
>-- 
>2.34.1
>