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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZUq81py010AdTE49@debug.ba.rivosinc.com>
Date:   Tue, 7 Nov 2023 14:40:22 -0800
From:   Deepak Gupta <debug@...osinc.com>
To:     Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc:     x86@...nel.org, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, luto@...nel.org,
        peterz@...radead.org, linux-kernel@...r.kernel.org,
        broonie@...nel.org, pengfei.xu@...el.com
Subject: Re: [PATCH] x86/shstk: Change SSP after user accesses

Acked-by: Deepak Gupta <debug@...osinc.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@...r.kernel.org
>Fixes: 05e36022c054 ("x86/shstk: Handle signals for shadow stack")
>Reported-by: Pengfei Xu <pengfei.xu@...el.com>
>Tested-by: Pengfei Xu <pengfei.xu@...el.com>
>Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.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
>