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

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

[Rick Edgecombe]

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

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

[Deepak Gupta]

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
>

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Tue, Nov 07, 2023 at 10:22:51AM -0800, Rick Edgecombe wrote:

> 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.

Thanks for the heads up on this.  From inspection it looks like the
arm64 patches currently order things similarly to your new x86 code.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]