[PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[Xin Li (Intel)]

From: Xin Li <xin@zytor.com>

Clear the software event flag in the augmented SS to prevent infinite
SIGTRAP handler loop if TF is used without an external debugger.

Following is a typical single-stepping flow for a user process:

1) The user process is prepared for single-stepping by setting
   RFLAGS.TF = 1.
2) When any instruction in user space completes, a #DB is triggered.
3) The kernel handles the #DB and returns to user space, invoking the
   SIGTRAP handler with RFLAGS.TF = 0.
4) After the SIGTRAP handler finishes, the user process performs a
   sigreturn syscall, restoring the original state, including
   RFLAGS.TF = 1.
5) Goto step 2.

According to the FRED specification:

A) Bit 17 in the augmented SS is designated as the software event
   flag, which is set to 1 for FRED event delivery of SYSCALL,
   SYSENTER, or INT n.
B) If bit 17 of the augmented SS is 1 and ERETU would result in
   RFLAGS.TF = 1, a single-step trap will be pending upon completion
   of ERETU.

In step 4) above, the software event flag is set upon the sigreturn
syscall, and its corresponding ERETU would restore RFLAGS.TF = 1.
This combination causes a pending single-step trap upon completion of
ERETU.  Therefore, another #DB is triggered before any user space
instruction is executed, which leads to an infinite loop in which the
SIGTRAP handler keeps being invoked on the same user space IP.

Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
Cc: stable@vger.kernel.org
---

Change in v2:
*) Remove the check cpu_feature_enabled(X86_FEATURE_FRED), because
   regs->fred_ss.swevent will always be 0 otherwise (H. Peter Anvin).
---
 arch/x86/include/asm/sighandling.h | 19 +++++++++++++++++++
 arch/x86/kernel/signal_32.c        |  4 ++++
 arch/x86/kernel/signal_64.c        |  4 ++++
 3 files changed, 27 insertions(+)

diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
index e770c4fc47f4..637f7705f0b2 100644
--- a/arch/x86/include/asm/sighandling.h
+++ b/arch/x86/include/asm/sighandling.h
@@ -24,4 +24,23 @@ int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
 int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
 int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
 
+/*
+ * To prevent infinite SIGTRAP handler loop if TF is used without an external
+ * debugger, clear the software event flag in the augmented SS, ensuring no
+ * single-step trap is pending upon ERETU completion.
+ *
+ * Note, this function should be called in sigreturn() before the original state
+ * is restored to make sure the TF is read from the entry frame.
+ */
+static __always_inline void prevent_single_step_upon_eretu(struct pt_regs *regs)
+{
+	/*
+	 * If the trap flag (TF) is set, i.e., the sigreturn() SYSCALL instruction
+	 * is being single-stepped, do not clear the software event flag in the
+	 * augmented SS, thus a debugger won't skip over the following instruction.
+	 */
+	if (IS_ENABLED(CONFIG_X86_FRED) && !(regs->flags & X86_EFLAGS_TF))
+		regs->fred_ss.swevent = 0;
+}
+
 #endif /* _ASM_X86_SIGHANDLING_H */
diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index 98123ff10506..42bbc42bd350 100644
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -152,6 +152,8 @@ SYSCALL32_DEFINE0(sigreturn)
 	struct sigframe_ia32 __user *frame = (struct sigframe_ia32 __user *)(regs->sp-8);
 	sigset_t set;
 
+	prevent_single_step_upon_eretu(regs);
+
 	if (!access_ok(frame, sizeof(*frame)))
 		goto badframe;
 	if (__get_user(set.sig[0], &frame->sc.oldmask)
@@ -175,6 +177,8 @@ SYSCALL32_DEFINE0(rt_sigreturn)
 	struct rt_sigframe_ia32 __user *frame;
 	sigset_t set;
 
+	prevent_single_step_upon_eretu(regs);
+
 	frame = (struct rt_sigframe_ia32 __user *)(regs->sp - 4);
 
 	if (!access_ok(frame, sizeof(*frame)))
diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index ee9453891901..d483b585c6c6 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -250,6 +250,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	sigset_t set;
 	unsigned long uc_flags;
 
+	prevent_single_step_upon_eretu(regs);
+
 	frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
 	if (!access_ok(frame, sizeof(*frame)))
 		goto badframe;
@@ -366,6 +368,8 @@ COMPAT_SYSCALL_DEFINE0(x32_rt_sigreturn)
 	sigset_t set;
 	unsigned long uc_flags;
 
+	prevent_single_step_upon_eretu(regs);
+
 	frame = (struct rt_sigframe_x32 __user *)(regs->sp - 8);
 
 	if (!access_ok(frame, sizeof(*frame)))

base-commit: 6a7c3c2606105a41dde81002c0037420bc1ddf00
-- 
2.49.0

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[Dave Hansen]

On 5/22/25 10:17, Xin Li (Intel) wrote:
> Clear the software event flag in the augmented SS to prevent infinite
> SIGTRAP handler loop if TF is used without an external debugger.

Do you have a test case for this? It seems like the kind of thing we'd
want in selftests/.

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[Xin Li]

On 5/22/2025 10:22 AM, Dave Hansen wrote:
> On 5/22/25 10:17, Xin Li (Intel) wrote:
>> Clear the software event flag in the augmented SS to prevent infinite
>> SIGTRAP handler loop if TF is used without an external debugger.
> 
> Do you have a test case for this? It seems like the kind of thing we'd
> want in selftests/.

Ah, good point!

The issue was found during an internal LASS test, I will convert it to a
selftest.

Thanks!
     Xin

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[Andrew Cooper]

On 22/05/2025 6:22 pm, Dave Hansen wrote:
> On 5/22/25 10:17, Xin Li (Intel) wrote:
>> Clear the software event flag in the augmented SS to prevent infinite
>> SIGTRAP handler loop if TF is used without an external debugger.
> Do you have a test case for this? It seems like the kind of thing we'd
> want in selftests/.

Hmm.

This was a behaviour intentionally changed in FRED so traps wouldn't get
lost if an exception where to occur.

What precise case is triggering this?

~Andrew

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[H. Peter Anvin]

On May 22, 2025 10:53:16 AM PDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>On 22/05/2025 6:22 pm, Dave Hansen wrote:
>> On 5/22/25 10:17, Xin Li (Intel) wrote:
>>> Clear the software event flag in the augmented SS to prevent infinite
>>> SIGTRAP handler loop if TF is used without an external debugger.
>> Do you have a test case for this? It seems like the kind of thing we'd
>> want in selftests/.
>
>Hmm.
>
>This was a behaviour intentionally changed in FRED so traps wouldn't get
>lost if an exception where to occur.
>
>What precise case is triggering this?
>
>~Andrew

SIGTRAP → sigreturn. Basically, we have to uplevel the suppression behavior to the kernel (where it belongs) instead of doing it at the ISA level. 

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[Andrew Cooper]

On 22/05/2025 10:28 pm, H. Peter Anvin wrote:
> On May 22, 2025 10:53:16 AM PDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 22/05/2025 6:22 pm, Dave Hansen wrote:
>>> On 5/22/25 10:17, Xin Li (Intel) wrote:
>>>> Clear the software event flag in the augmented SS to prevent infinite
>>>> SIGTRAP handler loop if TF is used without an external debugger.
>>> Do you have a test case for this? It seems like the kind of thing we'd
>>> want in selftests/.
>> Hmm.
>>
>> This was a behaviour intentionally changed in FRED so traps wouldn't get
>> lost if an exception where to occur.
>>
>> What precise case is triggering this?
>>
>> ~Andrew
> SIGTRAP → sigreturn. Basically, we have to uplevel the suppression behavior to the kernel (where it belongs) instead of doing it at the ISA level. 

So the problem is specifically that we're in a SYSCALL context (from
FRED's point of view), and we rewrite state in the FRED FRAME to be
another context which happened to have eflags.TF set.

And the combination of these two triggers a new singlestep to be pending
immediately.

I have to admit that I didn't like the implication from the SYSCALL bit,
and argued to have it handled differently, but alas.  I think the real
bug here is trying to ERETU with a splice of two different contexts
worth of FRED state.

~Andrew

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[Xin Li]

On 5/22/2025 10:53 AM, Andrew Cooper wrote:
> This was a behaviour intentionally changed in FRED so traps wouldn't get
> lost if an exception where to occur.
> 
> What precise case is triggering this?

Following is the test code:

// SPDX-License-Identifier: GPL-2.0-or-later
/*
  *  Copyright (C) 2025 Intel Corporation
  */
#define _GNU_SOURCE

#include <err.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ucontext.h>

static void sethandler(int sig, void (*handler)(int, siginfo_t *, void 
*), int flags)
{
	struct sigaction sa;

	memset(&sa, 0, sizeof(sa));
	sa.sa_sigaction = handler;
	sa.sa_flags = SA_SIGINFO | flags;
	sigemptyset(&sa.sa_mask);

	if (sigaction(sig, &sa, 0))
		err(1, "sigaction");

	return;
}

static void sigtrap(int sig, siginfo_t *info, void *ctx_void)
{
	ucontext_t *ctx = (ucontext_t *)ctx_void;
	static unsigned long last_trap_ip;
	static unsigned int loop_count_on_same_ip;

	if (last_trap_ip == ctx->uc_mcontext.gregs[REG_RIP]) {
		printf("trapped on %016lx\n", last_trap_ip);

		if (++loop_count_on_same_ip > 10) {
			printf("trap loop detected, test failed\n");
			exit(2);
		}

		return;
	}

	loop_count_on_same_ip = 0;
	last_trap_ip = ctx->uc_mcontext.gregs[REG_RIP];
	printf("trapped on %016lx\n", last_trap_ip);
}

int main(int argc, char *argv[])
{
	sethandler(SIGTRAP, sigtrap, 0);

	asm volatile("push $0x302\n\t"
		     "popf\n\t"
		     "nop\n\t"
		     "nop\n\t"
		     "push $0x202\n\t"
		     "popf\n\t");

	printf("test passed\n");
}


W/o the fix when FRED enabled, I get:
xin@fred-ubt:~$ ./lass_test
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trapped on 00000000004012fe
trap loop detected, test failed


W/ the fix when FRED enabled:
[xin@dev ~]$ ./lass_test
trapped on 00000000004012fe
trapped on 00000000004012ff
trapped on 0000000000401304
trapped on 0000000000401305
test passed

Obviously the test passes on IDT.

As Dave asked, I will integrate this test into selftests.

Thanks!
     Xin

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[H. Peter Anvin]

On 5/22/25 17:10, Andrew Cooper wrote:
>>>
>>> ~Andrew
>> SIGTRAP → sigreturn. Basically, we have to uplevel the suppression behavior to the kernel (where it belongs) instead of doing it at the ISA level. 
> 
> So the problem is specifically that we're in a SYSCALL context (from
> FRED's point of view), and we rewrite state in the FRED FRAME to be
> another context which happened to have eflags.TF set.
> 
> And the combination of these two triggers a new singlestep to be pending
> immediately.
> 
> I have to admit that I didn't like the implication from the SYSCALL bit,
> and argued to have it handled differently, but alas.  I think the real
> bug here is trying to ERETU with a splice of two different contexts
> worth of FRED state.
> 

To some degree it is, yes. And it is sigreturn that does that splicing.

But it is desirable to be able to single-step across sigreturn if one is
debugging from inside the signal handler, hence we should not clearing
TF if it is set on sigreturn entry.

This is in fact exactly analogous to ERETU ignoring the syscall bit if
TF is set before ERETU is executed, just one abstraction level higher up
in the stack.

	-hpa

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[H. Peter Anvin]

On May 22, 2025 5:57:31 PM PDT, Xin Li <xin@zytor.com> wrote:
>On 5/22/2025 10:53 AM, Andrew Cooper wrote:
>> This was a behaviour intentionally changed in FRED so traps wouldn't get
>> lost if an exception where to occur.
>> 
>> What precise case is triggering this?
>
>Following is the test code:
>
>// SPDX-License-Identifier: GPL-2.0-or-later
>/*
> *  Copyright (C) 2025 Intel Corporation
> */
>#define _GNU_SOURCE
>
>#include <err.h>
>#include <signal.h>
>#include <stdio.h>
>#include <stdlib.h>
>#include <string.h>
>#include <sys/ucontext.h>
>
>static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *), int flags)
>{
>	struct sigaction sa;
>
>	memset(&sa, 0, sizeof(sa));
>	sa.sa_sigaction = handler;
>	sa.sa_flags = SA_SIGINFO | flags;
>	sigemptyset(&sa.sa_mask);
>
>	if (sigaction(sig, &sa, 0))
>		err(1, "sigaction");
>
>	return;
>}
>
>static void sigtrap(int sig, siginfo_t *info, void *ctx_void)
>{
>	ucontext_t *ctx = (ucontext_t *)ctx_void;
>	static unsigned long last_trap_ip;
>	static unsigned int loop_count_on_same_ip;
>
>	if (last_trap_ip == ctx->uc_mcontext.gregs[REG_RIP]) {
>		printf("trapped on %016lx\n", last_trap_ip);
>
>		if (++loop_count_on_same_ip > 10) {
>			printf("trap loop detected, test failed\n");
>			exit(2);
>		}
>
>		return;
>	}
>
>	loop_count_on_same_ip = 0;
>	last_trap_ip = ctx->uc_mcontext.gregs[REG_RIP];
>	printf("trapped on %016lx\n", last_trap_ip);
>}
>
>int main(int argc, char *argv[])
>{
>	sethandler(SIGTRAP, sigtrap, 0);
>
>	asm volatile("push $0x302\n\t"
>		     "popf\n\t"
>		     "nop\n\t"
>		     "nop\n\t"
>		     "push $0x202\n\t"
>		     "popf\n\t");
>
>	printf("test passed\n");
>}
>
>
>W/o the fix when FRED enabled, I get:
>xin@fred-ubt:~$ ./lass_test
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trapped on 00000000004012fe
>trap loop detected, test failed
>
>
>W/ the fix when FRED enabled:
>[xin@dev ~]$ ./lass_test
>trapped on 00000000004012fe
>trapped on 00000000004012ff
>trapped on 0000000000401304
>trapped on 0000000000401305
>test passed
>
>Obviously the test passes on IDT.
>
>As Dave asked, I will integrate this test into selftests.
>
>Thanks!
>    Xin

Btw, make the test work on 32 bits as well (just a matter of using a different ucontext.)

Re: [PATCH v2 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[Xin Li]

On 5/23/2025 12:42 AM, H. Peter Anvin wrote:
> Btw, make the test work on 32 bits as well (just a matter of using a different ucontext.)

Yeah, that is for sure.