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

[PATCH v1 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
---
 arch/x86/include/asm/sighandling.h | 20 ++++++++++++++++++++
 arch/x86/kernel/signal_32.c        |  4 ++++
 arch/x86/kernel/signal_64.c        |  4 ++++
 3 files changed, 28 insertions(+)

diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
index e770c4fc47f4..ecb0411fe88c 100644
--- a/arch/x86/include/asm/sighandling.h
+++ b/arch/x86/include/asm/sighandling.h
@@ -24,4 +24,24 @@ 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) && cpu_feature_enabled(X86_FEATURE_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 v1 1/1] x86/fred/signal: Prevent single-step upon ERETU completion

[H. Peter Anvin]

On 5/21/25 23:05, Xin Li (Intel) wrote:
>  
> +/*
> + * 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) && cpu_feature_enabled(X86_FEATURE_FRED) &&
> +	    !(regs->flags & X86_EFLAGS_TF))
> +		regs->fred_ss.swevent = 0;
> +}
> +

Minor nit (and I should have caught this when I saw your patch earlier):

cpu_feature_enabled(X86_FEATURE_FRED) is unnecessary here, because when
FRED is not enabled, regs->fred_ss.swevent will always be 0, and this
bit has no function, so there is no point in making that extra test.

The only reason for IS_ENABLED(CONFIG_X86_FRED) (which is implied in
cpu_feature_enabled() anyway via !CONFIG_X86_DISABLED_FEATURE_FRED) is
to eliminate the code entirely if FRED is not compiled in.

Although the way it goes, it sounds like CONFIG_X86_FRED might go away
soon anyway, since KVM wants to use some of the FRED code
unconditionally (and Xen might follow, too.)

	-hpa

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

[Xin Li]

On 5/22/2025 12:20 AM, H. Peter Anvin wrote:
> On 5/21/25 23:05, Xin Li (Intel) wrote:
>>   
>> +/*
>> + * 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) && cpu_feature_enabled(X86_FEATURE_FRED) &&
>> +	    !(regs->flags & X86_EFLAGS_TF))
>> +		regs->fred_ss.swevent = 0;
>> +}
>> +
> 
> Minor nit (and I should have caught this when I saw your patch earlier):
> 
> cpu_feature_enabled(X86_FEATURE_FRED) is unnecessary here, because when
> FRED is not enabled, regs->fred_ss.swevent will always be 0, and this
> bit has no function, so there is no point in making that extra test.

Yeah, less conditions, less complexity.  I will remove it.

Thanks!
     Xin

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

[kernel test robot]

Hi Xin,

kernel test robot noticed the following build errors:

[auto build test ERROR on 6a7c3c2606105a41dde81002c0037420bc1ddf00]

url:    https://github.com/intel-lab-lkp/linux/commits/Xin-Li-Intel/x86-fred-signal-Prevent-single-step-upon-ERETU-completion/20250522-140954
base:   6a7c3c2606105a41dde81002c0037420bc1ddf00
patch link:    https://lore.kernel.org/r/20250522060549.2882444-1-xin%40zytor.com
patch subject: [PATCH v1 1/1] x86/fred/signal: Prevent single-step upon ERETU completion
config: i386-buildonly-randconfig-003-20250522 (https://download.01.org/0day-ci/archive/20250523/202505230141.4YBHhrPI-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250523/202505230141.4YBHhrPI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505230141.4YBHhrPI-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/signal_32.c:32:
   arch/x86/include/asm/sighandling.h: In function 'prevent_single_step_upon_eretu':
>> arch/x86/include/asm/sighandling.h:44:21: error: 'struct pt_regs' has no member named 'fred_ss'
      44 |                 regs->fred_ss.swevent = 0;
         |                     ^~


vim +44 arch/x86/include/asm/sighandling.h

    26	
    27	/*
    28	 * To prevent infinite SIGTRAP handler loop if TF is used without an external
    29	 * debugger, clear the software event flag in the augmented SS, ensuring no
    30	 * single-step trap is pending upon ERETU completion.
    31	 *
    32	 * Note, this function should be called in sigreturn() before the original state
    33	 * is restored to make sure the TF is read from the entry frame.
    34	 */
    35	static __always_inline void prevent_single_step_upon_eretu(struct pt_regs *regs)
    36	{
    37		/*
    38		 * If the trap flag (TF) is set, i.e., the sigreturn() SYSCALL instruction
    39		 * is being single-stepped, do not clear the software event flag in the
    40		 * augmented SS, thus a debugger won't skip over the following instruction.
    41		 */
    42		if (IS_ENABLED(CONFIG_X86_FRED) && cpu_feature_enabled(X86_FEATURE_FRED) &&
    43		    !(regs->flags & X86_EFLAGS_TF))
  > 44			regs->fred_ss.swevent = 0;
    45	}
    46	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Xin Li]

On 5/22/2025 10:20 AM, kernel test robot wrote:
> Hi Xin,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 6a7c3c2606105a41dde81002c0037420bc1ddf00]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Xin-Li-Intel/x86-fred-signal-Prevent-single-step-upon-ERETU-completion/20250522-140954
> base:   6a7c3c2606105a41dde81002c0037420bc1ddf00
> patch link:    https://lore.kernel.org/r/20250522060549.2882444-1-xin%40zytor.com
> patch subject: [PATCH v1 1/1] x86/fred/signal: Prevent single-step upon ERETU completion
> config: i386-buildonly-randconfig-003-20250522 (https://download.01.org/0day-ci/archive/20250523/202505230141.4YBHhrPI-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250523/202505230141.4YBHhrPI-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202505230141.4YBHhrPI-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>     In file included from arch/x86/kernel/signal_32.c:32:
>     arch/x86/include/asm/sighandling.h: In function 'prevent_single_step_upon_eretu':
>>> arch/x86/include/asm/sighandling.h:44:21: error: 'struct pt_regs' has no member named 'fred_ss'
>        44 |                 regs->fred_ss.swevent = 0;
>           |                     ^~
> 

Hmm, this statement is under IS_ENABLED(CONFIG_X86_FRED), which should
be a compile time FALSE with i386.  Why it is still being compiled?

Thanks!
     Xin

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

[Andrew Cooper]

On 22/05/2025 6:33 pm, Xin Li wrote:
> On 5/22/2025 10:20 AM, kernel test robot wrote:
>> Hi Xin,
>>
>> kernel test robot noticed the following build errors:
>>
>> [auto build test ERROR on 6a7c3c2606105a41dde81002c0037420bc1ddf00]
>>
>> url:   
>> https://github.com/intel-lab-lkp/linux/commits/Xin-Li-Intel/x86-fred-signal-Prevent-single-step-upon-ERETU-completion/20250522-140954
>> base:   6a7c3c2606105a41dde81002c0037420bc1ddf00
>> patch link:   
>> https://lore.kernel.org/r/20250522060549.2882444-1-xin%40zytor.com
>> patch subject: [PATCH v1 1/1] x86/fred/signal: Prevent single-step
>> upon ERETU completion
>> config: i386-buildonly-randconfig-003-20250522
>> (https://download.01.org/0day-ci/archive/20250523/202505230141.4YBHhrPI-lkp@intel.com/config)
>> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
>> reproduce (this is a W=1 build):
>> (https://download.01.org/0day-ci/archive/20250523/202505230141.4YBHhrPI-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new
>> version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes:
>> https://lore.kernel.org/oe-kbuild-all/202505230141.4YBHhrPI-lkp@intel.com/
>>
>> All errors (new ones prefixed by >>):
>>
>>     In file included from arch/x86/kernel/signal_32.c:32:
>>     arch/x86/include/asm/sighandling.h: In function
>> 'prevent_single_step_upon_eretu':
>>>> arch/x86/include/asm/sighandling.h:44:21: error: 'struct pt_regs'
>>>> has no member named 'fred_ss'
>>        44 |                 regs->fred_ss.swevent = 0;
>>           |                     ^~
>>
>
> Hmm, this statement is under IS_ENABLED(CONFIG_X86_FRED), which should
> be a compile time FALSE with i386.  Why it is still being compiled?

Because what the compiler is seeing is:

if (0 && ...)
    regs->fred_ss.swevent = 0;

and the bad field name is found at parse time, while the whole
expression is only discarded during optimisation.

The one thing you can't IS_ENABLED() around is conditional fields.  That
needs to be full #ifdef.

~Andrew

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

[Dave Hansen]

On 5/22/25 10:33, Xin Li wrote:
>>>> arch/x86/include/asm/sighandling.h:44:21: error: 'struct pt_regs'
>>>> has no member named 'fred_ss'
>>        44 |                 regs->fred_ss.swevent = 0;
>>           |                     ^~
>>
> 
> Hmm, this statement is under IS_ENABLED(CONFIG_X86_FRED), which should

The compiler still _compiles_ unreachable statements:

	if (0)
		foo = bar.bar;

An #ifdef will keep the compiler out completely, of course. That's
probably the best thing in this case, especially since it'll be tucked
away in a header.

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

[Xin Li]

>>>      arch/x86/include/asm/sighandling.h: In function
>>> 'prevent_single_step_upon_eretu':
>>>>> arch/x86/include/asm/sighandling.h:44:21: error: 'struct pt_regs'
>>>>> has no member named 'fred_ss'
>>>         44 |                 regs->fred_ss.swevent = 0;
>>>            |                     ^~
>>>
>>
>> Hmm, this statement is under IS_ENABLED(CONFIG_X86_FRED), which should
>> be a compile time FALSE with i386.  Why it is still being compiled?
> 
> Because what the compiler is seeing is:
> 
> if (0 && ...)
>      regs->fred_ss.swevent = 0;
> 
> and the bad field name is found at parse time, while the whole
> expression is only discarded during optimisation.
> 
> The one thing you can't IS_ENABLED() around is conditional fields.  That
> needs to be full #ifdef.

Thanks a lot for the explanation.  Just sent out v3 using #ifdef.

     Xin

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

[Xin Li]

On 5/22/2025 10:54 AM, Dave Hansen wrote:
> On 5/22/25 10:33, Xin Li wrote:
>>>>> arch/x86/include/asm/sighandling.h:44:21: error: 'struct pt_regs'
>>>>> has no member named 'fred_ss'
>>>         44 |                 regs->fred_ss.swevent = 0;
>>>            |                     ^~
>>>
>>
>> Hmm, this statement is under IS_ENABLED(CONFIG_X86_FRED), which should
> 
> The compiler still _compiles_ unreachable statements:
> 
> 	if (0)
> 		foo = bar.bar;
> 
> An #ifdef will keep the compiler out completely, of course. That's
> probably the best thing in this case, especially since it'll be tucked
> away in a header.

Thanks a lot for the explanation.

     Xin