[RFC PATCH v1 0/2] selftests/x86: sysret_rip update for FRED system

[RFC PATCH v1 0/2] selftests/x86: sysret_rip update for FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

On Mon, 23 Jan 2023 15:58:12 -0800, "H. Peter Anvin" wrote:
> On 1/23/23 15:43, Ammar Faizi wrote:
> > 
> > Align them to spot differences:
> > 
> >      0x200893 = 0b1000000000100010010011
> >      0x200a93 = 0b1000000000101010010011
> >                               ^
> > 
> > Or just xor them to find the differences:
> > 
> >      (gdb) p/x 0x200893 ^ 0x200a93
> >      $3 = 0x200
> > 
> > ** Checks my Intel SDM cheat sheets. **
> > 
> > Then, I was like "Oh, that's (1 << 9) a.k.a. IF. Of course we can't
> > change rflags[IF] from userspace!!!".
> > 
> > In short, we can't use 0x200893 as the rflags_sentinel value because it
> > clears the interrupt flag.
> > 
> 
> Right, my mistake.

I changed it to 0x200a93. The test passed on my machine. But I don't
have a FRED system to test the special case.

Didn't manage to apply the feedback from Andrew about the way to handle
redzone properly, though.

Something like this...

----------

This is just an RFC patchset.

Xin Li reported sysret_rip test fails at:

        assert(ctx->uc_mcontext.gregs[REG_EFL] ==
               ctx->uc_mcontext.gregs[REG_R11]);

in a FRED system. Handle the FRED system scenario too. There are two
patches in this series. Comments welcome...

Note: Only tested for 'syscall' sets %rcx=%rip and %r11=%rflags case.
I don't have a FRED system to test it.

How to test this:

  $ make -C tools/testing/selftests/x86
  $ tools/testing/selftests/x86/sysret_rip_64

Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  selftests/x86: sysret_rip: Handle syscall in a FRED system
  selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

 tools/testing/selftests/x86/sysret_rip.c | 105 ++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 1 deletion(-)


base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
-- 
Ammar Faizi

[RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

The current selftest asserts %r11 == %rflags after the 'syscall'
returns to user. Such an assertion doesn't apply to a FRED system
because in a FRED system the 'syscall' instruction does not set
%r11=%rflags and %rcx=%rip.

Handle the FRED case. Now, test that:

  - "syscall" in a FRED system doesn't clobber %rcx and %r11.
  - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

The 'raise()' function from libc can't be used to control those
registers. Therefore, create a syscall wrapper in inline Assembly to
fully control them.

Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Missing a Signed-off-by tag from HPA. 

@hpa send your sign off if you like this patch.

 tools/testing/selftests/x86/sysret_rip.c | 96 +++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d90207ab..9056f2e2674d2bc5 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -39,6 +39,95 @@ asm (
 extern const char test_page[];
 static void const *current_test_page_addr = test_page;
 
+/* Arbitrary values */
+static const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
+static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
+
+/* An arbitrary *valid* RFLAGS value */
+static const unsigned long rflags_sentinel = 0x200a93;
+
+enum regs_ok {
+	REGS_ERROR  = -1,	/* Invalid register contents */
+	REGS_SAVED  =  0,	/* Registers properly preserved */
+	REGS_SYSRET =  1	/* Registers match syscall/sysret */
+};
+
+/*
+ * Returns:
+ *  0 = %rcx and %r11 preserved.
+ *  1 = %rcx and %r11 set to %rflags and %rip.
+ * -1 = %rcx and/or %r11 set to any other values.
+ *
+ * Note that check_regs_syscall() sets %rbx to the syscall return %rip.
+ */
+static enum regs_ok check_regs_result(unsigned long r11, unsigned long rcx,
+				      unsigned long rbx)
+{
+	if (r11 == r11_sentinel && rcx == rcx_sentinel) {
+		return REGS_SAVED;
+	} else if (r11 == rflags_sentinel && rcx == rbx) {
+		return REGS_SYSRET;
+	} else {
+		printf("[FAIL] check_regs_result\n");
+		printf("        r11_sentinel = %#lx; %%r11 = %#lx;\n", r11_sentinel, r11);
+		printf("        rcx_sentinel = %#lx; %%rcx = %#lx;\n", rcx_sentinel, rcx);
+		printf("        rflags_sentinel = %#lx\n", rflags_sentinel);
+		return REGS_ERROR;
+	}
+}
+
+static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
+		       unsigned long arg3, unsigned long arg4,
+		       unsigned long arg5, unsigned long arg6)
+{
+	register unsigned long r11 asm("%r11");
+	register unsigned long r10 asm("%r10");
+	register unsigned long r8 asm("%r8");
+	register unsigned long r9 asm("%r9");
+	unsigned long rcx, rbx;
+
+	r11 = r11_sentinel;
+	rcx = rcx_sentinel;
+	r10 = arg4;
+	r8 = arg5;
+	r9 = arg6;
+
+	asm volatile (
+		"movq	-8(%%rsp), %%r12\n\t"	/* Don't clobber redzone. */
+		"pushq	%[rflags_sentinel]\n\t"
+		"popf\n\t"
+		"movq	%%r12, -8(%%rsp)\n\t"
+		"leaq	1f(%%rip), %[rbx]\n\t"
+		"syscall\n"
+		"1:"
+
+		: "+a" (nr_syscall),
+		  "+r" (r11),
+		  "+c" (rcx),
+		  [rbx] "=b" (rbx)
+
+		: [rflags_sentinel] "g" (rflags_sentinel),
+		  "D" (arg1),	/* %rdi */
+		  "S" (arg2),	/* %rsi */
+		  "d" (arg3),	/* %rdx */
+		  "r" (r10),
+		  "r" (r8),
+		  "r" (r9)
+
+		: "r12", "memory"
+	);
+
+	/*
+	 * Test that:
+	 *
+	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
+	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
+	 *
+	 */
+	assert(check_regs_result(r11, rcx, rbx) != REGS_ERROR);
+	return nr_syscall;
+}
+
 static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
 		       int flags)
 {
@@ -101,11 +190,16 @@ static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 	return;
 }
 
+static void __raise(int sig)
+{
+	do_syscall(__NR_kill, getpid(), sig, 0, 0, 0, 0);
+}
+
 static void test_sigreturn_to(unsigned long ip)
 {
 	rip = ip;
 	printf("[RUN]\tsigreturn to 0x%lx\n", ip);
-	raise(SIGUSR1);
+	__raise(SIGUSR1);
 }
 
 static jmp_buf jmpbuf;
-- 
Ammar Faizi

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[H. Peter Anvin]

On 1/23/23 16:26, Ammar Faizi wrote:
> +
> +static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
> +		       unsigned long arg3, unsigned long arg4,
> +		       unsigned long arg5, unsigned long arg6)
> +{
> +	register unsigned long r11 asm("%r11");
> +	register unsigned long r10 asm("%r10");
> +	register unsigned long r8 asm("%r8");
> +	register unsigned long r9 asm("%r9");
> +	unsigned long rcx, rbx;
> +
> +	r11 = r11_sentinel;
> +	rcx = rcx_sentinel;
> +	r10 = arg4;
> +	r8 = arg5;
> +	r9 = arg6;
> +
> +	asm volatile (
> +		"movq	-8(%%rsp), %%r12\n\t"	/* Don't clobber redzone. */
> +		"pushq	%[rflags_sentinel]\n\t"
> +		"popf\n\t"
> +		"movq	%%r12, -8(%%rsp)\n\t"
> +		"leaq	1f(%%rip), %[rbx]\n\t"
> +		"syscall\n"
> +		"1:"
> +
> +		: "+a" (nr_syscall),
> +		  "+r" (r11),
> +		  "+c" (rcx),
> +		  [rbx] "=b" (rbx)
> +
> +		: [rflags_sentinel] "g" (rflags_sentinel),
> +		  "D" (arg1),	/* %rdi */
> +		  "S" (arg2),	/* %rsi */
> +		  "d" (arg3),	/* %rdx */
> +		  "r" (r10),
> +		  "r" (r8),
> +		  "r" (r9)
> +
> +		: "r12", "memory"
> +	);
> +
> +	/*
> +	 * Test that:
> +	 *
> +	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
> +	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> +	 *
> +	 */
> +	assert(check_regs_result(r11, rcx, rbx) != REGS_ERROR);
> +	return nr_syscall;
> +}
> +

So as per Andrew's comment, add:

register void * rsp asm("%rsp");

...

"+r" (rsp)	/* clobber the redzone */

... as the right way to avoid redzone problems.

	-hpa

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Mon, Jan 23, 2023 at 05:40:23PM -0800, H. Peter Anvin wrote:
> On 1/23/23 16:26, Ammar Faizi wrote:
> > +
> > +static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
> > +		       unsigned long arg3, unsigned long arg4,
> > +		       unsigned long arg5, unsigned long arg6)
> > +{
> > +	register unsigned long r11 asm("%r11");
> > +	register unsigned long r10 asm("%r10");
> > +	register unsigned long r8 asm("%r8");
> > +	register unsigned long r9 asm("%r9");
> > +	unsigned long rcx, rbx;
> > +
> > +	r11 = r11_sentinel;
> > +	rcx = rcx_sentinel;
> > +	r10 = arg4;
> > +	r8 = arg5;
> > +	r9 = arg6;
> > +
> > +	asm volatile (
> > +		"movq	-8(%%rsp), %%r12\n\t"	/* Don't clobber redzone. */
> > +		"pushq	%[rflags_sentinel]\n\t"
> > +		"popf\n\t"
> > +		"movq	%%r12, -8(%%rsp)\n\t"
> > +		"leaq	1f(%%rip), %[rbx]\n\t"
> > +		"syscall\n"
> > +		"1:"
> > +
> > +		: "+a" (nr_syscall),
> > +		  "+r" (r11),
> > +		  "+c" (rcx),
> > +		  [rbx] "=b" (rbx)
> > +
> > +		: [rflags_sentinel] "g" (rflags_sentinel),
> > +		  "D" (arg1),	/* %rdi */
> > +		  "S" (arg2),	/* %rsi */
> > +		  "d" (arg3),	/* %rdx */
> > +		  "r" (r10),
> > +		  "r" (r8),
> > +		  "r" (r9)
> > +
> > +		: "r12", "memory"
> > +	);
> > +
> > +	/*
> > +	 * Test that:
> > +	 *
> > +	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
> > +	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> > +	 *
> > +	 */
> > +	assert(check_regs_result(r11, rcx, rbx) != REGS_ERROR);
> > +	return nr_syscall;
> > +}
> > +
> 
> So as per Andrew's comment, add:
> 
> register void * rsp asm("%rsp");
> 
> ...
> 
> "+r" (rsp)	/* clobber the redzone */
> 
> ... as the right way to avoid redzone problems.

Fixed in v2.

-- 
Ammar Faizi

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Mon, Jan 23, 2023 at 05:40:23PM -0800, H. Peter Anvin wrote:
> So as per Andrew's comment, add:
> 
> register void * rsp asm("%rsp");
> 
> ...
> 
> "+r" (rsp)	/* clobber the redzone */
> 
> ... as the right way to avoid redzone problems.

I played with this more. I found something wrong with this. This doesn't
work for me. The compiler still uses red zone despite I use "+r" (rsp).

What did I do wrong?

-----

ammarfaizi2@integral2:/tmp$ gcc -fno-stack-protector -O2 -Wall -Wextra test.c -o test
ammarfaizi2@integral2:/tmp$ objdump --no-show-raw-insn -d test | grep "a_leaf_func_with_red_zone>:" -A8
0000000000001180 <a_leaf_func_with_red_zone>:
    1180:  endbr64 
    1184:  mov    $0x1,%eax
    1189:  mov    %rax,-0x8(%rsp)   ## BUG!!!
    118e:  pushf  
    118f:  pop    %rax
    1190:  mov    -0x8(%rsp),%rax   ## BUG!!!
    1195:  ret


ammarfaizi2@integral2:/tmp$ clang -O2 -Wall -Wextra test.c -o test
ammarfaizi2@integral2:/tmp$ objdump --no-show-raw-insn -d test | grep "a_leaf_func_with_red_zone>:" -A6
0000000000001140 <a_leaf_func_with_red_zone>:
    1140:  mov    $0x1,%eax
    1145:  mov    %rax,-0x8(%rsp)   ## BUG!!!
    114a:  pushf  
    114b:  pop    %rax
    114c:  mov    -0x8(%rsp),%rax   ## BUG!!!
    1151:  ret


-----
ammarfaizi2@integral2:~$ gcc --version
gcc (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

ammarfaizi2@integral2:~$ clang --version
Ubuntu clang version 16.0.0 (++20230124031324+d63e492562f2-1~exp1~20230124151444.705)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin


-----
test.c:

#include <stdio.h>
static inline void clobber_redzone(void)
{
        register void *rsp __asm__("%rsp");
        unsigned long rflags;

        __asm__ volatile ("pushf; popq %1"
                          : "+r" (rsp), "=r" (rflags));
}

static inline void set_red_zone(long *mem, long val)
{
        __asm__ volatile ("movq %[val], %[mem]"
                           : [mem] "=m" (*mem)
                           : [val] "r" (val));
}

static inline long get_red_zone(long *mem)
{
        long ret;

        __asm__ volatile ("movq %[in], %[out]"
                           : [out] "=r" (ret)
                           : [in] "m" (*mem));
        return ret;
}

__attribute__((__noinline__))
long a_leaf_func_with_red_zone(void)
{
        long x;

        set_red_zone(&x, 1);
        clobber_redzone();
        /* The correct retval is 1 */
        return get_red_zone(&x);
}

int main(void)
{
        printf("ret = %ld\n", a_leaf_func_with_red_zone());
        return 0;
}

-- 
Ammar Faizi

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Andrew Cooper]

On 26/01/2023 8:08 pm, Ammar Faizi wrote:
> On Mon, Jan 23, 2023 at 05:40:23PM -0800, H. Peter Anvin wrote:
>> So as per Andrew's comment, add:
>>
>> register void * rsp asm("%rsp");
>>
>> ...
>>
>> "+r" (rsp)	/* clobber the redzone */
>>
>> ... as the right way to avoid redzone problems.
> I played with this more. I found something wrong with this. This doesn't
> work for me. The compiler still uses red zone despite I use "+r" (rsp).
>
> What did I do wrong?

Well this is a fine mess...

https://godbolt.org/z/MaPM7s8qr does the right thing, but is now
contrary to the prior discussion regarding calls in asm, which concluded
that the "+r"(rsp) was the way to go.

Furthermore GCC regressed in 9.0 and emits:

  warning: listing the stack pointer register 'rsp' in a clobber list is
deprecated [-Wdeprecated]

which might be the intention of the developers, but is wrong seeing as
this is the only way to say "I modify the redzone" to the compiler...

~Andrew

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Andrew Cooper]

On 15/02/2023 9:17 am, Andrew Cooper wrote:
> On 26/01/2023 8:08 pm, Ammar Faizi wrote:
>> On Mon, Jan 23, 2023 at 05:40:23PM -0800, H. Peter Anvin wrote:
>>> So as per Andrew's comment, add:
>>>
>>> register void * rsp asm("%rsp");
>>>
>>> ...
>>>
>>> "+r" (rsp)	/* clobber the redzone */
>>>
>>> ... as the right way to avoid redzone problems.
>> I played with this more. I found something wrong with this. This doesn't
>> work for me. The compiler still uses red zone despite I use "+r" (rsp).
>>
>> What did I do wrong?
> Well this is a fine mess...
>
> https://godbolt.org/z/MaPM7s8qr does the right thing, but is now
> contrary to the prior discussion regarding calls in asm, which concluded
> that the "+r"(rsp) was the way to go.
>
> Furthermore GCC regressed in 9.0 and emits:
>
>   warning: listing the stack pointer register 'rsp' in a clobber list is
> deprecated [-Wdeprecated]
>
> which might be the intention of the developers, but is wrong seeing as
> this is the only way to say "I modify the redzone" to the compiler...

I've opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108799

~Andrew

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Wed, Feb 15, 2023 at 10:29:37AM +0000, Andrew Cooper wrote:
> I've opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108799

Added myself to the CC list. Thanks for opening.

-- 
Ammar Faizi

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Wed, Feb 15, 2023 at 09:17:23AM +0000, Andrew Cooper wrote:
> On 26/01/2023 8:08 pm, Ammar Faizi wrote:
> > What did I do wrong?
> 
> Well this is a fine mess...
> 
> https://godbolt.org/z/MaPM7s8qr does the right thing, but is now
> contrary to the prior discussion regarding calls in asm, which concluded
> that the "+r"(rsp) was the way to go.

Does that also mean the ASM_CALL_CONSTRAINT macro in
arch/x86/include/asm/asm.h macro is wrong?

That macro adds a "+r"(rsp) constraint, and we assume it's safe to
execute the "call" instruction with that constraint in an inline
Assembly.

I am not sure what "+r" (rsp) actually does. And if we are now
complaining, "+r" (rsp) doesn't work. Since when it works? Or at least,
where is that rule written?  I couldn't find any GCC or Clang version
that does it right with the "+r" (rsp) constraint (from a quick playing
with that godbolt link).

> Furthermore GCC regressed in 9.0 and emits:
> 
>   warning: listing the stack pointer register 'rsp' in a clobber list is
> deprecated [-Wdeprecated]
> 
> which might be the intention of the developers, but is wrong seeing as
> this is the only way to say "I modify the redzone" to the compiler...

Yeah, adding "rsp" to the clobber list works. But sadly, it's deprecated
in GCC. Not sure what the reason is.

I think the most straightforward and safest way, for now, is: "Don't
clobber the red zone from the inline asm.".

I will use the previous approach to avoid red-zone clobbering in the
next revision. That's by adding "r12" to the clobber list and preserving
the red zone content in "%r12". 

-- 
Ammar Faizi

Re: [RFC PATCH v1 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

[ Resending, missed Xin Li from the Cc list... ]

On Mon, Jan 23, 2023 at 05:40:23PM -0800, H. Peter Anvin wrote:
> So as per Andrew's comment, add:
> 
> register void * rsp asm("%rsp");
> 
> ...
> 
> "+r" (rsp)	/* clobber the redzone */
> 
> ... as the right way to avoid redzone problems.

I played with this more. I found something wrong with this. This doesn't
work for me. The compiler still uses red zone despite I use "+r" (rsp).

What did I do wrong?

-----

ammarfaizi2@integral2:/tmp$ gcc -fno-stack-protector -O2 -Wall -Wextra test.c -o test
ammarfaizi2@integral2:/tmp$ objdump --no-show-raw-insn -d test | grep "a_leaf_func_with_red_zone>:" -A8
0000000000001180 <a_leaf_func_with_red_zone>:
    1180:  endbr64 
    1184:  mov    $0x1,%eax
    1189:  mov    %rax,-0x8(%rsp)   ## BUG!!!
    118e:  pushf  
    118f:  pop    %rax
    1190:  mov    -0x8(%rsp),%rax   ## BUG!!!
    1195:  ret


ammarfaizi2@integral2:/tmp$ clang -O2 -Wall -Wextra test.c -o test
ammarfaizi2@integral2:/tmp$ objdump --no-show-raw-insn -d test | grep "a_leaf_func_with_red_zone>:" -A6
0000000000001140 <a_leaf_func_with_red_zone>:
    1140:  mov    $0x1,%eax
    1145:  mov    %rax,-0x8(%rsp)   ## BUG!!!
    114a:  pushf  
    114b:  pop    %rax
    114c:  mov    -0x8(%rsp),%rax   ## BUG!!!
    1151:  ret


-----
ammarfaizi2@integral2:~$ gcc --version
gcc (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

ammarfaizi2@integral2:~$ clang --version
Ubuntu clang version 16.0.0 (++20230124031324+d63e492562f2-1~exp1~20230124151444.705)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin


-----
test.c:

#include <stdio.h>
static inline void clobber_redzone(void)
{
        register void *rsp __asm__("%rsp");
        unsigned long rflags;

        __asm__ volatile ("pushf; popq %1"
                          : "+r" (rsp), "=r" (rflags));
}

static inline void set_red_zone(long *mem, long val)
{
        __asm__ volatile ("movq %[val], %[mem]"
                           : [mem] "=m" (*mem)
                           : [val] "r" (val));
}

static inline long get_red_zone(long *mem)
{
        long ret;

        __asm__ volatile ("movq %[in], %[out]"
                           : [out] "=r" (ret)
                           : [in] "m" (*mem));
        return ret;
}

__attribute__((__noinline__))
long a_leaf_func_with_red_zone(void)
{
        long x;

        set_red_zone(&x, 1);
        clobber_redzone();
        /* The correct retval is 1 */
        return get_red_zone(&x);
}

int main(void)
{
        printf("ret = %ld\n", a_leaf_func_with_red_zone());
        return 0;
}

-- 
Ammar Faizi

[RFC PATCH v1 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Test that:

 - "syscall" in a FRED system doesn't clobber %rcx and %r11.
 - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

Test them out with a trivial system call like __NR_getppid and friends
which are extremely likely to return with SYSRET on an IDT system; check
that it returns a nonnegative value and then save the result.

Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Co-developed-by: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Missing a Signed-off-by tag from HPA. 

@hpa send your sign off if you like this patch.

 tools/testing/selftests/x86/sysret_rip.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 9056f2e2674d2bc5..c55f6d04f0ae1f2d 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -252,8 +252,17 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 	printf("[OK]\tWe survived\n");
 }
 
+static void test_syscall_rcx_r11(void)
+{
+	do_syscall(__NR_getpid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_gettid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
+}
+
 int main()
 {
+	test_syscall_rcx_r11();
+
 	/*
 	 * When the kernel returns from a slow-path syscall, it will
 	 * detect whether SYSRET is appropriate.  If it incorrectly
-- 
Ammar Faizi

[RFC PATCH v2 0/2] selftests/x86: sysret_rip update for FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

This is an RFC patchset v2.

Xin Li reported sysret_rip test fails at:

        assert(ctx->uc_mcontext.gregs[REG_EFL] ==
               ctx->uc_mcontext.gregs[REG_R11]);

in a FRED system. Handle the FRED system scenario too. There are two
patches in this series. Comments welcome...

Note: This patchset is only tested for 'syscall' sets %rcx=%rip and
%r11=%rflags case. I don't have a FRED system to test it.

How to test this:

  $ make -C tools/testing/selftests/x86
  $ tools/testing/selftests/x86/sysret_rip_64

Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

## Changelog v2:

   - Use "+r"(rsp) as the right way to avoid redzone problems
     per Andrew's comment (hpa).
     (Ref: https://lore.kernel.org/lkml/8f5c24df-514d-5d89-f58f-ec8c3eb1e049@zytor.com )

---

Ammar Faizi (2):
  selftests/x86: sysret_rip: Handle syscall in a FRED system
  selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

 tools/testing/selftests/x86/sysret_rip.c | 105 ++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 1 deletion(-)


base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
-- 
Ammar Faizi

[RFC PATCH v2 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

The current selftest asserts %r11 == %rflags after the 'syscall'
returns to user. Such an assertion doesn't apply to a FRED system
because in a FRED system the 'syscall' instruction does not set
%r11=%rflags and %rcx=%rip.

Handle the FRED case. Now, test that:

  - "syscall" in a FRED system doesn't clobber %rcx and %r11.
  - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

The 'raise()' function from libc can't be used to control those
registers. Therefore, create a syscall wrapper in inline Assembly to
fully control them.

Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Need hpa's sign off.

## Changelog v2:

   - Use "+r"(rsp) as the right way to avoid redzone problems
     per Andrew's comment (hpa).
     (Ref: https://lore.kernel.org/lkml/8f5c24df-514d-5d89-f58f-ec8c3eb1e049@zytor.com )

 tools/testing/selftests/x86/sysret_rip.c | 96 +++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d90207ab..550bc4e69ac46997 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -39,6 +39,95 @@ asm (
 extern const char test_page[];
 static void const *current_test_page_addr = test_page;
 
+/* Arbitrary values */
+static const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
+static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
+
+/* An arbitrary *valid* RFLAGS value */
+static const unsigned long rflags_sentinel = 0x200a93;
+
+enum regs_ok {
+	REGS_ERROR  = -1,	/* Invalid register contents */
+	REGS_SAVED  =  0,	/* Registers properly preserved */
+	REGS_SYSRET =  1	/* Registers match syscall/sysret */
+};
+
+/*
+ * Returns:
+ *  0 = %rcx and %r11 preserved.
+ *  1 = %rcx and %r11 set to %rflags and %rip.
+ * -1 = %rcx and/or %r11 set to any other values.
+ *
+ * Note that check_regs_syscall() sets %rbx to the syscall return %rip.
+ */
+static enum regs_ok check_regs_result(unsigned long r11, unsigned long rcx,
+				      unsigned long rbx)
+{
+	if (r11 == r11_sentinel && rcx == rcx_sentinel) {
+		return REGS_SAVED;
+	} else if (r11 == rflags_sentinel && rcx == rbx) {
+		return REGS_SYSRET;
+	} else {
+		printf("[FAIL] check_regs_result\n");
+		printf("        r11_sentinel = %#lx; %%r11 = %#lx;\n", r11_sentinel, r11);
+		printf("        rcx_sentinel = %#lx; %%rcx = %#lx;\n", rcx_sentinel, rcx);
+		printf("        rflags_sentinel = %#lx\n", rflags_sentinel);
+		return REGS_ERROR;
+	}
+}
+
+static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
+		       unsigned long arg3, unsigned long arg4,
+		       unsigned long arg5, unsigned long arg6)
+{
+	register unsigned long r11 asm("%r11");
+	register unsigned long r10 asm("%r10");
+	register unsigned long r8 asm("%r8");
+	register unsigned long r9 asm("%r9");
+	register void *rsp asm("%rsp");
+	unsigned long rcx, rbx;
+
+	r11 = r11_sentinel;
+	rcx = rcx_sentinel;
+	r10 = arg4;
+	r8 = arg5;
+	r9 = arg6;
+
+	asm volatile (
+		"pushq	%[rflags_sentinel]\n\t"
+		"popf\n\t"
+		"leaq	1f(%%rip), %[rbx]\n\t"
+		"syscall\n"
+		"1:"
+
+		: "+a" (nr_syscall),
+		  "+r" (r11),
+		  "+c" (rcx),
+		  [rbx] "=b" (rbx),
+		  "+r" (rsp)	/* Clobber the redzone */
+
+		: [rflags_sentinel] "g" (rflags_sentinel),
+		  "D" (arg1),	/* %rdi */
+		  "S" (arg2),	/* %rsi */
+		  "d" (arg3),	/* %rdx */
+		  "r" (r10),
+		  "r" (r8),
+		  "r" (r9)
+
+		: "memory"
+	);
+
+	/*
+	 * Test that:
+	 *
+	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
+	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
+	 *
+	 */
+	assert(check_regs_result(r11, rcx, rbx) != REGS_ERROR);
+	return nr_syscall;
+}
+
 static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
 		       int flags)
 {
@@ -101,11 +190,16 @@ static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 	return;
 }
 
+static void __raise(int sig)
+{
+	do_syscall(__NR_kill, getpid(), sig, 0, 0, 0, 0);
+}
+
 static void test_sigreturn_to(unsigned long ip)
 {
 	rip = ip;
 	printf("[RUN]\tsigreturn to 0x%lx\n", ip);
-	raise(SIGUSR1);
+	__raise(SIGUSR1);
 }
 
 static jmp_buf jmpbuf;
-- 
Ammar Faizi

Re: [RFC PATCH v2 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[H. Peter Anvin]

On 1/23/23 18:27, Ammar Faizi wrote:
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> The current selftest asserts %r11 == %rflags after the 'syscall'
> returns to user. Such an assertion doesn't apply to a FRED system
> because in a FRED system the 'syscall' instruction does not set
> %r11=%rflags and %rcx=%rip.
> 
> Handle the FRED case. Now, test that:
> 
>    - "syscall" in a FRED system doesn't clobber %rcx and %r11.
>    - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> 
> The 'raise()' function from libc can't be used to control those
> registers. Therefore, create a syscall wrapper in inline Assembly to
> fully control them.
> 
> Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
> Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
> Reported-by: Xin Li <xin3.li@intel.com>
> Co-developed-by: "H. Peter Anvin" <hpa@zytor.com>
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
> 
> Need hpa's sign off.
> 

For both patches:

Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>

[RFC PATCH v2 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Test that:

 - "syscall" in a FRED system doesn't clobber %rcx and %r11.
 - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

Test them out with a trivial system call like __NR_getppid and friends
which are extremely likely to return with SYSRET on an IDT system; check
that it returns a nonnegative value and then save the result.

Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Co-developed-by: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Need hpa's sign off.

 tools/testing/selftests/x86/sysret_rip.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 550bc4e69ac46997..75c72d34dbc5840c 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -252,8 +252,17 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 	printf("[OK]\tWe survived\n");
 }
 
+static void test_syscall_rcx_r11(void)
+{
+	do_syscall(__NR_getpid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_gettid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
+}
+
 int main()
 {
+	test_syscall_rcx_r11();
+
 	/*
 	 * When the kernel returns from a slow-path syscall, it will
 	 * detect whether SYSRET is appropriate.  If it incorrectly
-- 
Ammar Faizi

Re: [RFC PATCH v2 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[H. Peter Anvin]

On 1/23/23 18:27, Ammar Faizi wrote:
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Test that:
> 
>   - "syscall" in a FRED system doesn't clobber %rcx and %r11.
>   - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> 
> Test them out with a trivial system call like __NR_getppid and friends
> which are extremely likely to return with SYSRET on an IDT system; check
> that it returns a nonnegative value and then save the result.
> 

"Nonnegative" here should be "valid"; it is an implementation detail 
that the error value is -1.

However, you are not checking that you don't get a mix of REGS_SAVED and 
REGS_SYSRET, which is a major part of the point.

	-hpa

Re: [RFC PATCH v2 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

On Mon, Jan 23, 2023 at 10:16:01PM -0800, H. Peter Anvin wrote:
> On 1/23/23 18:27, Ammar Faizi wrote:
> > Test that:
> > 
> >   - "syscall" in a FRED system doesn't clobber %rcx and %r11.
> >   - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> > 
> > Test them out with a trivial system call like __NR_getppid and friends
> > which are extremely likely to return with SYSRET on an IDT system; check
> > that it returns a nonnegative value and then save the result.
> > 
> 
> "Nonnegative" here should be "valid"; it is an implementation detail that
> the error value is -1.

Copy-paste error, will fix that!

> However, you are not checking that you don't get a mix of REGS_SAVED and
> REGS_SYSRET, which is a major part of the point.

Good point!

What do you think of adding this on top of patch #1?

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 75c72d34dbc5840c..3da827713831acbc 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -47,11 +47,14 @@ static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
 static const unsigned long rflags_sentinel = 0x200a93;
 
 enum regs_ok {
-	REGS_ERROR  = -1,	/* Invalid register contents */
-	REGS_SAVED  =  0,	/* Registers properly preserved */
-	REGS_SYSRET =  1	/* Registers match syscall/sysret */
+	REGS_INIT_VAL	= -2,	/* For init value checker, never returned */
+	REGS_ERROR 	= -1,	/* Invalid register contents */
+	REGS_SAVED 	=  0,	/* Registers properly preserved */
+	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
 };
 
+static enum regs_ok regs_ok_state = REGS_INIT_VAL;
+
 /*
  * Returns:
  *  0 = %rcx and %r11 preserved.
@@ -86,6 +89,7 @@ static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
 	register unsigned long r9 asm("%r9");
 	register void *rsp asm("%rsp");
 	unsigned long rcx, rbx;
+	enum regs_ok ret;
 
 	r11 = r11_sentinel;
 	rcx = rcx_sentinel;
@@ -124,7 +128,14 @@ static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
 	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
 	 *
 	 */
-	assert(check_regs_result(r11, rcx, rbx) != REGS_ERROR);
+	ret = check_regs_result(r11, rcx, rbx);
+	assert(ret != REGS_ERROR);
+
+	if (regs_ok_state == REGS_INIT_VAL)
+		regs_ok_state = ret;
+	else
+		assert(ret == regs_ok_state);
+
 	return nr_syscall;
 }
 
-- 
Ammar Faizi

Re: [RFC PATCH v2 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

On Tue, Jan 24, 2023 at 01:41:30PM +0700, Ammar Faizi wrote:
> On Mon, Jan 23, 2023 at 10:16:01PM -0800, H. Peter Anvin wrote:
> > However, you are not checking that you don't get a mix of REGS_SAVED and
> > REGS_SYSRET, which is a major part of the point.
> 
> Good point!
> 
> What do you think of adding this on top of patch #1?
> 
> diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
> index 75c72d34dbc5840c..3da827713831acbc 100644
> --- a/tools/testing/selftests/x86/sysret_rip.c
> +++ b/tools/testing/selftests/x86/sysret_rip.c
> @@ -47,11 +47,14 @@ static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
>  static const unsigned long rflags_sentinel = 0x200a93;
>  
>  enum regs_ok {
> -	REGS_ERROR  = -1,	/* Invalid register contents */
> -	REGS_SAVED  =  0,	/* Registers properly preserved */
> -	REGS_SYSRET =  1	/* Registers match syscall/sysret */
> +	REGS_INIT_VAL	= -2,	/* For init value checker, never returned */
> +	REGS_ERROR 	= -1,	/* Invalid register contents */
> +	REGS_SAVED 	=  0,	/* Registers properly preserved */
> +	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
>  };
>  
> +static enum regs_ok regs_ok_state = REGS_INIT_VAL;
> +
>  /*
>   * Returns:
>   *  0 = %rcx and %r11 preserved.
> @@ -86,6 +89,7 @@ static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
>  	register unsigned long r9 asm("%r9");
>  	register void *rsp asm("%rsp");
>  	unsigned long rcx, rbx;
> +	enum regs_ok ret;
>  
>  	r11 = r11_sentinel;
>  	rcx = rcx_sentinel;
> @@ -124,7 +128,14 @@ static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
>  	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
>  	 *
>  	 */
> -	assert(check_regs_result(r11, rcx, rbx) != REGS_ERROR);
> +	ret = check_regs_result(r11, rcx, rbx);
> +	assert(ret != REGS_ERROR);
> +
> +	if (regs_ok_state == REGS_INIT_VAL)
> +		regs_ok_state = ret;
> +	else
> +		assert(ret == regs_ok_state);
> +
>  	return nr_syscall;
>  }
>  

And oh, on top of that. Add a comment, just to make it clear...

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 3da827713831acbc..3d783f5baee1b66a 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -131,6 +131,10 @@ static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
        ret = check_regs_result(r11, rcx, rbx);
        assert(ret != REGS_ERROR);
 
+       /*
+        * Check that we don't get a mix of REGS_SAVED and REGS_SYSRET.
+        * Need at least 2 times 'syscall' invoked from this function.
+        */
        if (regs_ok_state == REGS_INIT_VAL)
                regs_ok_state = ret;
        else

-- 
Ammar Faizi

Re: [RFC PATCH v2 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[H. Peter Anvin]

On January 23, 2023 10:41:20 PM PST, Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
>On Mon, Jan 23, 2023 at 10:16:01PM -0800, H. Peter Anvin wrote:
>> On 1/23/23 18:27, Ammar Faizi wrote:
>> > Test that:
>> > 
>> >   - "syscall" in a FRED system doesn't clobber %rcx and %r11.
>> >   - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
>> > 
>> > Test them out with a trivial system call like __NR_getppid and friends
>> > which are extremely likely to return with SYSRET on an IDT system; check
>> > that it returns a nonnegative value and then save the result.
>> > 
>> 
>> "Nonnegative" here should be "valid"; it is an implementation detail that
>> the error value is -1.
>
>Copy-paste error, will fix that!
>
>> However, you are not checking that you don't get a mix of REGS_SAVED and
>> REGS_SYSRET, which is a major part of the point.
>
>Good point!
>
>What do you think of adding this on top of patch #1?
>
>diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
>index 75c72d34dbc5840c..3da827713831acbc 100644
>--- a/tools/testing/selftests/x86/sysret_rip.c
>+++ b/tools/testing/selftests/x86/sysret_rip.c
>@@ -47,11 +47,14 @@ static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
> static const unsigned long rflags_sentinel = 0x200a93;
> 
> enum regs_ok {
>-	REGS_ERROR  = -1,	/* Invalid register contents */
>-	REGS_SAVED  =  0,	/* Registers properly preserved */
>-	REGS_SYSRET =  1	/* Registers match syscall/sysret */
>+	REGS_INIT_VAL	= -2,	/* For init value checker, never returned */
>+	REGS_ERROR 	= -1,	/* Invalid register contents */
>+	REGS_SAVED 	=  0,	/* Registers properly preserved */
>+	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
> };
> 
>+static enum regs_ok regs_ok_state = REGS_INIT_VAL;
>+
> /*
>  * Returns:
>  *  0 = %rcx and %r11 preserved.
>@@ -86,6 +89,7 @@ static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
> 	register unsigned long r9 asm("%r9");
> 	register void *rsp asm("%rsp");
> 	unsigned long rcx, rbx;
>+	enum regs_ok ret;
> 
> 	r11 = r11_sentinel;
> 	rcx = rcx_sentinel;
>@@ -124,7 +128,14 @@ static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
> 	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> 	 *
> 	 */
>-	assert(check_regs_result(r11, rcx, rbx) != REGS_ERROR);
>+	ret = check_regs_result(r11, rcx, rbx);
>+	assert(ret != REGS_ERROR);
>+
>+	if (regs_ok_state == REGS_INIT_VAL)
>+		regs_ok_state = ret;
>+	else
>+		assert(ret == regs_ok_state);
>+
> 	return nr_syscall;
> }
> 

Works for me, although I would call it REGS_UNDEFINED myself.

Re: [RFC PATCH v2 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

On Tue, Jan 24, 2023 at 01:07:38AM -0800, H. Peter Anvin wrote:
> Works for me, although I would call it REGS_UNDEFINED myself.

I'll call it REGS_UNDEFINED too, in v3.

-- 
Ammar Faizi

[RFC PATCH v3 0/2] selftests/x86: sysret_rip update for FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

This is an RFC patchset v3:
sysret_rip test update for Intel FRED architecture. 

Xin Li reported sysret_rip test fails at:

        assert(ctx->uc_mcontext.gregs[REG_EFL] ==
               ctx->uc_mcontext.gregs[REG_R11]);

in a FRED system. Let's handle the FRED system scenario too. The
'syscall' instruction in a FRED system doesn't set %r11=%rflags.

There are two patches in this series.

How to test this:

  $ make -C tools/testing/selftests/x86
  $ tools/testing/selftests/x86/sysret_rip_64

Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com
Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

## Changelog v3:

   - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
     which is a major part of the point (hpa).

## Changelog v2:

   - Use "+r"(rsp) as the right way to avoid redzone problems
     per Andrew's comment (hpa).
     (Ref: https://lore.kernel.org/lkml/8f5c24df-514d-5d89-f58f-ec8c3eb1e049@zytor.com )

---

Ammar Faizi (2):
  selftests/x86: sysret_rip: Handle syscall in a FRED system
  selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

 tools/testing/selftests/x86/sysret_rip.c | 120 ++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 1 deletion(-)


base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
-- 
Ammar Faizi

[RFC PATCH v3 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

The current selftest asserts %r11 == %rflags after the 'syscall'
returns to user. Such an assertion doesn't apply to a FRED system
because in a FRED system the 'syscall' instruction does not set
%r11=%rflags and %rcx=%rip.

Handle the FRED case. Now, test that:

  - "syscall" in a FRED system doesn't clobber %rcx and %r11.
  - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

The 'raise()' function from libc can't be used to control those
registers. Therefore, create a syscall wrapper in inline Assembly to
fully control them.

Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 111 ++++++++++++++++++++++-
 1 file changed, 110 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d90207ab..b0d271c19ddd7834 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -39,6 +39,110 @@ asm (
 extern const char test_page[];
 static void const *current_test_page_addr = test_page;
 
+/* Arbitrary values */
+static const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
+static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
+
+/* An arbitrary *valid* RFLAGS value */
+static const unsigned long rflags_sentinel = 0x200a93;
+
+enum regs_ok {
+	REGS_UNDEFINED	= -2,	/* For init value checker, never returned */
+	REGS_ERROR	= -1,	/* Invalid register contents */
+	REGS_SAVED	=  0,	/* Registers properly preserved */
+	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
+};
+
+static enum regs_ok regs_ok_state = REGS_UNDEFINED;
+
+/*
+ * Returns:
+ *  0 = %rcx and %r11 preserved.
+ *  1 = %rcx and %r11 set to %rflags and %rip.
+ * -1 = %rcx and/or %r11 set to any other values.
+ *
+ * Note that check_regs_syscall() sets %rbx to the syscall return %rip.
+ */
+static enum regs_ok check_regs_result(unsigned long r11, unsigned long rcx,
+				      unsigned long rbx)
+{
+	if (r11 == r11_sentinel && rcx == rcx_sentinel) {
+		return REGS_SAVED;
+	} else if (r11 == rflags_sentinel && rcx == rbx) {
+		return REGS_SYSRET;
+	} else {
+		printf("[FAIL] check_regs_result\n");
+		printf("        r11_sentinel = %#lx; %%r11 = %#lx;\n", r11_sentinel, r11);
+		printf("        rcx_sentinel = %#lx; %%rcx = %#lx;\n", rcx_sentinel, rcx);
+		printf("        rflags_sentinel = %#lx\n", rflags_sentinel);
+		return REGS_ERROR;
+	}
+}
+
+static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
+		       unsigned long arg3, unsigned long arg4,
+		       unsigned long arg5, unsigned long arg6)
+{
+	register unsigned long r11 asm("%r11");
+	register unsigned long r10 asm("%r10");
+	register unsigned long r8 asm("%r8");
+	register unsigned long r9 asm("%r9");
+	register void *rsp asm("%rsp");
+	unsigned long rcx, rbx;
+	enum regs_ok ret;
+
+	r11 = r11_sentinel;
+	rcx = rcx_sentinel;
+	r10 = arg4;
+	r8 = arg5;
+	r9 = arg6;
+
+	asm volatile (
+		"pushq	%[rflags_sentinel]\n\t"
+		"popf\n\t"
+		"leaq	1f(%%rip), %[rbx]\n\t"
+		"syscall\n"
+		"1:"
+
+		: "+a" (nr_syscall),
+		  "+r" (r11),
+		  "+c" (rcx),
+		  [rbx] "=b" (rbx),
+		  "+r" (rsp)	/* Clobber the redzone */
+
+		: [rflags_sentinel] "g" (rflags_sentinel),
+		  "D" (arg1),	/* %rdi */
+		  "S" (arg2),	/* %rsi */
+		  "d" (arg3),	/* %rdx */
+		  "r" (r10),
+		  "r" (r8),
+		  "r" (r9)
+
+		: "memory"
+	);
+
+	/*
+	 * Test that:
+	 *
+	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
+	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
+	 *
+	 */
+	ret = check_regs_result(r11, rcx, rbx);
+	assert(ret != REGS_ERROR);
+
+	/*
+	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
+	 * Need at least 2 times 'syscall' invoked from this function.
+	 */
+	if (regs_ok_state == REGS_UNDEFINED)
+		regs_ok_state = ret;
+	else
+		assert(ret == regs_ok_state);
+
+	return nr_syscall;
+}
+
 static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
 		       int flags)
 {
@@ -101,11 +205,16 @@ static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 	return;
 }
 
+static void __raise(int sig)
+{
+	do_syscall(__NR_kill, getpid(), sig, 0, 0, 0, 0);
+}
+
 static void test_sigreturn_to(unsigned long ip)
 {
 	rip = ip;
 	printf("[RUN]\tsigreturn to 0x%lx\n", ip);
-	raise(SIGUSR1);
+	__raise(SIGUSR1);
 }
 
 static jmp_buf jmpbuf;
-- 
Ammar Faizi

[RFC PATCH v3 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Test that:

 - "syscall" in a FRED system doesn't clobber %rcx and %r11.
 - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

Test them out with a trivial system call like __NR_getppid and friends
which are extremely likely to return with SYSRET on an IDT system.

Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index b0d271c19ddd7834..bf90fac95a264e2d 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -267,8 +267,17 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 	printf("[OK]\tWe survived\n");
 }
 
+static void test_syscall_rcx_r11(void)
+{
+	do_syscall(__NR_getpid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_gettid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
+}
+
 int main()
 {
+	test_syscall_rcx_r11();
+
 	/*
 	 * When the kernel returns from a slow-path syscall, it will
 	 * detect whether SYSRET is appropriate.  If it incorrectly
-- 
Ammar Faizi

Re: [RFC PATCH v3 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[H. Peter Anvin]

On 1/24/23 02:09, Ammar Faizi wrote:
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Test that:
> 
>   - "syscall" in a FRED system doesn't clobber %rcx and %r11.
>   - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> 
> Test them out with a trivial system call like __NR_getppid and friends
> which are extremely likely to return with SYSRET on an IDT system.
> 
> Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
> Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Add to the description that the purpose of this is to ensure that 
various system calls are *consistent*, as per the comment immediately 
below your code.

	-hpa

Re: [RFC PATCH v3 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

On Tue, Jan 24, 2023 at 12:59:23PM -0800, H. Peter Anvin wrote:
> 
> 
> On 1/24/23 02:09, Ammar Faizi wrote:
> > From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> > 
> > Test that:
> > 
> >   - "syscall" in a FRED system doesn't clobber %rcx and %r11.
> >   - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> > 
> > Test them out with a trivial system call like __NR_getppid and friends
> > which are extremely likely to return with SYSRET on an IDT system.
> > 
> > Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
> > Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> > Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> > Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> > Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Add to the description that the purpose of this is to ensure that various
> system calls are *consistent*, as per the comment immediately below your
> code.

Added in v4.

-- 
Ammar Faizi

RE: [RFC PATCH v3 0/2] selftests/x86: sysret_rip update for FRED system

[Li, Xin3]

> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> This is an RFC patchset v3:
> sysret_rip test update for Intel FRED architecture.
> 
> Xin Li reported sysret_rip test fails at:
> 
>         assert(ctx->uc_mcontext.gregs[REG_EFL] ==
>                ctx->uc_mcontext.gregs[REG_R11]);

On FRED systems, flags is 0x200a93 and r11 is 0xfeedfacedeadbeef here.

We need to remove or change this assertion, maybe:
  assert(ctx->uc_mcontext.gregs[REG_EFL] == ctx->uc_mcontext.gregs[REG_R11] ||
         r11_sentinel == ctx->uc_mcontext.gregs[REG_R11]);

> 
> in a FRED system. Let's handle the FRED system scenario too. The 'syscall'
> instruction in a FRED system doesn't set %r11=%rflags.
> 
> There are two patches in this series.
> 
> How to test this:
> 
>   $ make -C tools/testing/selftests/x86
>   $ tools/testing/selftests/x86/sysret_rip_64
> 
> Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-
> 9c001c2343af@intel.com
> Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical
> addresses")
> Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-
> 786b55054126@zytor.com
> Reported-by: Xin Li <xin3.li@intel.com>
> Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
> 
> ## Changelog v3:
> 
>    - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
>      which is a major part of the point (hpa).
> 
> ## Changelog v2:
> 
>    - Use "+r"(rsp) as the right way to avoid redzone problems
>      per Andrew's comment (hpa).
>      (Ref: https://lore.kernel.org/lkml/8f5c24df-514d-5d89-f58f-
> ec8c3eb1e049@zytor.com )
> 
> ---
> 
> Ammar Faizi (2):
>   selftests/x86: sysret_rip: Handle syscall in a FRED system
>   selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`
> 
>  tools/testing/selftests/x86/sysret_rip.c | 120 ++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 1 deletion(-)
> 
> 
> base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
> --
> Ammar Faizi

RE: [RFC PATCH v3 0/2] selftests/x86: sysret_rip update for FRED system

[H. Peter Anvin]

On January 24, 2023 1:32:14 PM PST, "Li, Xin3" <xin3.li@intel.com> wrote:
>> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
>> 
>> This is an RFC patchset v3:
>> sysret_rip test update for Intel FRED architecture.
>> 
>> Xin Li reported sysret_rip test fails at:
>> 
>>         assert(ctx->uc_mcontext.gregs[REG_EFL] ==
>>                ctx->uc_mcontext.gregs[REG_R11]);
>
>On FRED systems, flags is 0x200a93 and r11 is 0xfeedfacedeadbeef here.
>
>We need to remove or change this assertion, maybe:
>  assert(ctx->uc_mcontext.gregs[REG_EFL] == ctx->uc_mcontext.gregs[REG_R11] ||
>         r11_sentinel == ctx->uc_mcontext.gregs[REG_R11]);
>
>> 
>> in a FRED system. Let's handle the FRED system scenario too. The 'syscall'
>> instruction in a FRED system doesn't set %r11=%rflags.
>> 
>> There are two patches in this series.
>> 
>> How to test this:
>> 
>>   $ make -C tools/testing/selftests/x86
>>   $ tools/testing/selftests/x86/sysret_rip_64
>> 
>> Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-
>> 9c001c2343af@intel.com
>> Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical
>> addresses")
>> Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-
>> 786b55054126@zytor.com
>> Reported-by: Xin Li <xin3.li@intel.com>
>> Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>> Acked-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
>> ---
>> 
>> ## Changelog v3:
>> 
>>    - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
>>      which is a major part of the point (hpa).
>> 
>> ## Changelog v2:
>> 
>>    - Use "+r"(rsp) as the right way to avoid redzone problems
>>      per Andrew's comment (hpa).
>>      (Ref: https://lore.kernel.org/lkml/8f5c24df-514d-5d89-f58f-
>> ec8c3eb1e049@zytor.com )
>> 
>> ---
>> 
>> Ammar Faizi (2):
>>   selftests/x86: sysret_rip: Handle syscall in a FRED system
>>   selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`
>> 
>>  tools/testing/selftests/x86/sysret_rip.c | 120 ++++++++++++++++++++++-
>>  1 file changed, 119 insertions(+), 1 deletion(-)
>> 
>> 
>> base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
>> --
>> Ammar Faizi
>
>

This should use check_regs_result() – which is exactly the reason I made that a separate function.

RE: [RFC PATCH v3 0/2] selftests/x86: sysret_rip update for FRED system

[Li, Xin3]

> >>
> >> Xin Li reported sysret_rip test fails at:
> >>
> >>         assert(ctx->uc_mcontext.gregs[REG_EFL] ==
> >>                ctx->uc_mcontext.gregs[REG_R11]);
> >
> >On FRED systems, flags is 0x200a93 and r11 is 0xfeedfacedeadbeef here.
> >
> >We need to remove or change this assertion, maybe:
> >  assert(ctx->uc_mcontext.gregs[REG_EFL] == ctx->uc_mcontext.gregs[REG_R11] ||
> >         r11_sentinel == ctx->uc_mcontext.gregs[REG_R11]);
> >
> >>
> >> in a FRED system. Let's handle the FRED system scenario too. The 'syscall'
> >> instruction in a FRED system doesn't set %r11=%rflags.
> >>
> 
> This should use check_regs_result() – which is exactly the reason I made that a
> separate function.

Exactly.

Re: [RFC PATCH v3 0/2] selftests/x86: sysret_rip update for FRED system

[Ammar Faizi]

On Tue, Jan 24, 2023 at 01:37:43PM -0800, H. Peter Anvin wrote:
> On January 24, 2023 1:32:14 PM PST, "Li, Xin3" <xin3.li@intel.com> wrote:
> >> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> >> 
> >> This is an RFC patchset v3:
> >> sysret_rip test update for Intel FRED architecture.
> >> 
> >> Xin Li reported sysret_rip test fails at:
> >> 
> >>         assert(ctx->uc_mcontext.gregs[REG_EFL] ==
> >>                ctx->uc_mcontext.gregs[REG_R11]);
> >
> >On FRED systems, flags is 0x200a93 and r11 is 0xfeedfacedeadbeef here.
> >
> >We need to remove or change this assertion, maybe:
> >  assert(ctx->uc_mcontext.gregs[REG_EFL] == ctx->uc_mcontext.gregs[REG_R11] ||
> >         r11_sentinel == ctx->uc_mcontext.gregs[REG_R11]);
> >
>
> This should use check_regs_result() – which is exactly the reason I made that a separate function.

Fixed in v4.

-- 
Ammar Faizi

Re: [RFC PATCH v3 0/2] selftests/x86: sysret_rip update for FRED system

[Andrew Cooper]

On 24/01/2023 9:32 pm, Li, Xin3 wrote:
>> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
>>
>> This is an RFC patchset v3:
>> sysret_rip test update for Intel FRED architecture.
>>
>> Xin Li reported sysret_rip test fails at:
>>
>>         assert(ctx->uc_mcontext.gregs[REG_EFL] ==
>>                ctx->uc_mcontext.gregs[REG_R11]);
> On FRED systems, flags is 0x200a93 and r11 is 0xfeedfacedeadbeef here.

Is this under SIMICS, or elsewhere?  It's doubly weird that flags/%r11
match, but %rip/%rcx don't.

~Andrew

RE: [RFC PATCH v3 0/2] selftests/x86: sysret_rip update for FRED system

[Li, Xin3]

> >>         assert(ctx->uc_mcontext.gregs[REG_EFL] ==
> >>                ctx->uc_mcontext.gregs[REG_R11]);
> > On FRED systems, flags is 0x200a93 and r11 is 0xfeedfacedeadbeef here.
> 
> Is this under SIMICS, or elsewhere?  It's doubly weird that flags/%r11 match, but
> %rip/%rcx don't.

This is on Intel Simics, with FRED enabled.

Flags and %r11 don’t match on FRED, and %rip and %rcx also don't match.

I think it's expected.

Xin

[RFC PATCH v4 0/2] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

This is an RFC patchset v4. There are two patches in this series.

Xin Li reported that the sysret_rip test fails at:

        assert(ctx->uc_mcontext.gregs[REG_EFL] ==
               ctx->uc_mcontext.gregs[REG_R11]);

on the Intel FRED architecture. Let's handle the FRED system scenario
too. The 'syscall' instruction in a FRED system doesn't set %rcx=%rip
and %r11=%rflags.

Syscall and sysenter in a FRED system are treated equivalently to
software interrupts, e.g. INT 0x80. They do not modify any registers.

Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com

#### Changelog v4:

   - Fix the assertion condition inside the SIGUSR1 handler (Xin Li).

   - Explain the purpose of patch #2 in the commit message (HPA).

   - Update commit message (Ammar).

   - Repeat test_syscall_rcx_r11_consistent() 32 times to be more sure
     that the result is really consistent (Ammar).

#### Changelog v3:

   - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
     which is a major part of the point (HPA).

#### Changelog v2:

   - Use "+r"(rsp) as the right way to avoid redzone problems
     per Andrew's comment (HPA).


Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  selftests/x86: sysret_rip: Handle syscall in a FRED system
  selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

 tools/testing/selftests/x86/sysret_rip.c | 147 +++++++++++++++++++++--
 1 file changed, 138 insertions(+), 9 deletions(-)


base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
-- 
Ammar Faizi

[RFC PATCH v4 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

The current selftest asserts (%r11 == %rflags) after the 'syscall'
returns to user. Such an assertion doesn't apply to the FRED system
because in that system the 'syscall' instruction does not set
%r11=%rflags and %rcx=%rip.

Handle the FRED case. Now, test that:

  - "syscall" in a FRED system doesn't clobber %rcx and %r11.
  - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

The 'raise()' function from libc can't be used to control those
registers. Therefore, create a syscall wrapper in inline Assembly to
fully control them.

Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 127 +++++++++++++++++++++--
 1 file changed, 120 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d90207ab..86a31bbac9a85a88 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -39,6 +39,113 @@ asm (
 extern const char test_page[];
 static void const *current_test_page_addr = test_page;
 
+/* Arbitrary values */
+static const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
+static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
+
+/* An arbitrary *valid* RFLAGS value */
+static const unsigned long rflags_sentinel = 0x200a93;
+
+enum regs_ok {
+	REGS_UNDEFINED	= -2,	/* For consistency checker init, never returned */
+	REGS_ERROR	= -1,	/* Invalid register contents */
+	REGS_SAVED	=  0,	/* Registers properly preserved */
+	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
+};
+
+/*
+ * Returns:
+ *  0 = %rcx and %r11 preserved.
+ *  1 = %rcx and %r11 set to %rflags and %rip.
+ * -1 = %rcx and/or %r11 set to any other values.
+ *
+ * @rbx should be set to the syscall return %rip.
+ */
+static enum regs_ok check_regs_result(unsigned long r11, unsigned long rcx,
+				      unsigned long rbx)
+{
+	if (r11 == r11_sentinel && rcx == rcx_sentinel)
+		return REGS_SAVED;
+
+	if (r11 == rflags_sentinel && rcx == rbx)
+		return REGS_SYSRET;
+
+	printf("[FAIL] check_regs_result\n");
+	printf("        r11_sentinel = %#lx; %%r11 = %#lx;\n", r11_sentinel, r11);
+	printf("        rcx_sentinel = %#lx; %%rcx = %#lx;\n", rcx_sentinel, rcx);
+	printf("        rflags_sentinel = %#lx\n", rflags_sentinel);
+	return REGS_ERROR;
+}
+
+static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
+		       unsigned long arg3, unsigned long arg4,
+		       unsigned long arg5, unsigned long arg6)
+{
+	static enum regs_ok regs_ok_state = REGS_UNDEFINED;
+	register unsigned long r11 asm("%r11");
+	register unsigned long r10 asm("%r10");
+	register unsigned long r8 asm("%r8");
+	register unsigned long r9 asm("%r9");
+	register void *rsp asm("%rsp");
+	unsigned long rcx, rbx;
+	enum regs_ok ret;
+
+	r11 = r11_sentinel;
+	rcx = rcx_sentinel;
+	r10 = arg4;
+	r8 = arg5;
+	r9 = arg6;
+
+	asm volatile (
+		"pushq	%[rflags_sentinel]\n\t"
+		"popf\n\t"
+		"leaq	1f(%%rip), %[rbx]\n\t"
+		"syscall\n"
+		"1:"
+
+		: "+a" (nr_syscall),
+		  "+r" (r11),
+		  "+c" (rcx),
+		  [rbx] "=b" (rbx),
+		  "+r" (rsp)	/* Clobber the redzone */
+
+		: [rflags_sentinel] "g" (rflags_sentinel),
+		  "D" (arg1),	/* %rdi */
+		  "S" (arg2),	/* %rsi */
+		  "d" (arg3),	/* %rdx */
+		  "r" (r10),
+		  "r" (r8),
+		  "r" (r9)
+
+		: "memory"
+	);
+
+	/*
+	 * Test that:
+	 *
+	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
+	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
+	 */
+	ret = check_regs_result(r11, rcx, rbx);
+	assert(ret != REGS_ERROR);
+
+	/*
+	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
+	 * It needs at least calling do_syscall() twice to assert.
+	 */
+	if (regs_ok_state == REGS_UNDEFINED) {
+		/*
+		 * First time calling do_syscall().
+		 */
+		regs_ok_state = ret;
+		return ret;
+	} else {
+		assert(regs_ok_state == ret);
+	}
+
+	return nr_syscall;
+}
+
 static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
 		       int flags)
 {
@@ -85,27 +192,33 @@ static void sigsegv_for_sigreturn_test(int sig, siginfo_t *info, void *ctx_void)
 static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 {
 	ucontext_t *ctx = (ucontext_t*)ctx_void;
+	enum regs_ok ret;
 
 	memcpy(&initial_regs, &ctx->uc_mcontext.gregs, sizeof(gregset_t));
 
+	ret = check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
+				ctx->uc_mcontext.gregs[REG_RCX],
+				ctx->uc_mcontext.gregs[REG_RBX]);
+
+	assert(ret != REGS_ERROR);
+
 	/* Set IP and CX to match so that SYSRET can happen. */
 	ctx->uc_mcontext.gregs[REG_RIP] = rip;
 	ctx->uc_mcontext.gregs[REG_RCX] = rip;
-
-	/* R11 and EFLAGS should already match. */
-	assert(ctx->uc_mcontext.gregs[REG_EFL] ==
-	       ctx->uc_mcontext.gregs[REG_R11]);
-
 	sethandler(SIGSEGV, sigsegv_for_sigreturn_test, SA_RESETHAND);
 
-	return;
+}
+
+static void __raise(int sig)
+{
+	do_syscall(__NR_kill, getpid(), sig, 0, 0, 0, 0);
 }
 
 static void test_sigreturn_to(unsigned long ip)
 {
 	rip = ip;
 	printf("[RUN]\tsigreturn to 0x%lx\n", ip);
-	raise(SIGUSR1);
+	__raise(SIGUSR1);
 }
 
 static jmp_buf jmpbuf;
-- 
Ammar Faizi

Re: [RFC PATCH v4 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Wed, Jan 25, 2023 at 10:22:39AM +0700, Ammar Faizi wrote:
> +	/*
> +	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
> +	 * It needs at least calling do_syscall() twice to assert.
> +	 */
> +	if (regs_ok_state == REGS_UNDEFINED) {
> +		/*
> +		 * First time calling do_syscall().
> +		 */
> +		regs_ok_state = ret;
> +		return ret;
> +	} else {

Oops, this should not be returning ret here. Ignore this version.
I'll send v5.

-- 
Ammar Faizi

Re: [RFC PATCH v4 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Wed, Jan 25, 2023 at 10:37:17AM +0700, Ammar Faizi wrote:
> On Wed, Jan 25, 2023 at 10:22:39AM +0700, Ammar Faizi wrote:
> > +	/*
> > +	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
> > +	 * It needs at least calling do_syscall() twice to assert.
> > +	 */
> > +	if (regs_ok_state == REGS_UNDEFINED) {
> > +		/*
> > +		 * First time calling do_syscall().
> > +		 */
> > +		regs_ok_state = ret;
> > +		return ret;
> > +	} else {
> 
> Oops, this should not be returning ret here. Ignore this version.
> I'll send v5.

Side note: This mistake doesn't affect the test correctness because
we currenly don't use the return value of do_syscall().

But still worth fixing...

-- 
Ammar Faizi

[RFC PATCH v4 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Test that:

  - REGS_SAVED: "syscall" in a FRED system doesn't clobber %rcx and
    %r11.

  - REGS_SYSRET: "syscall" in a non-FRED system sets %rcx=%rip and
    %r11=%rflags.

Test them out with trivial system calls like __NR_getppid and friends
which are extremely likely to return with SYSRET on an IDT system.

Goals of this test:

  - Ensure that the syscall behavior is consistent. It should be either
    always REGS_SAVED or always REGS_SYSRET. Not a mix of them.

  - Ensure that the kernel doesn't leak its internal data when
    returning to userspace.

Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 86a31bbac9a85a88..5b8576147c92dbbd 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -271,8 +271,24 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 	printf("[OK]\tWe survived\n");
 }
 
+/*
+ * Ensure that various system calls are consistent.
+ * We should not get a mix of REGS_SAVED and REGS_SYSRET.
+ */
+static void test_syscall_rcx_r11_consistent(void)
+{
+	do_syscall(__NR_getpid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_gettid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
+}
+
 int main()
 {
+	int i;
+
+	for (i = 0; i < 32; i++)
+		test_syscall_rcx_r11_consistent();
+
 	/*
 	 * When the kernel returns from a slow-path syscall, it will
 	 * detect whether SYSRET is appropriate.  If it incorrectly
@@ -280,7 +296,7 @@ int main()
 	 * it'll crash on Intel CPUs.
 	 */
 	sethandler(SIGUSR1, sigusr1, 0);
-	for (int i = 47; i < 64; i++)
+	for (i = 47; i < 64; i++)
 		test_sigreturn_to(1UL<<i);
 
 	clearhandler(SIGUSR1);
@@ -291,7 +307,7 @@ int main()
 	test_syscall_fallthrough_to((1UL << 47) - 2*PAGE_SIZE);
 
 	/* These are the interesting cases. */
-	for (int i = 47; i < 64; i++) {
+	for (i = 47; i < 64; i++) {
 		test_syscall_fallthrough_to((1UL<<i) - PAGE_SIZE);
 		test_syscall_fallthrough_to(1UL<<i);
 	}
-- 
Ammar Faizi

[RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

This is an RFC patchset v5. There are two patches in this series.

Xin Li reported that the sysret_rip test fails at:

        assert(ctx->uc_mcontext.gregs[REG_EFL] ==
               ctx->uc_mcontext.gregs[REG_R11]);

on the Intel FRED architecture. Let's handle the FRED system scenario
too. The 'syscall' instruction in a FRED system doesn't set %rcx=%rip
and %r11=%rflags.

Syscall and sysenter in a FRED system are treated equivalently to
software interrupts, e.g. INT 0x80. They do not modify any registers.

Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com

#### Changelog v5:

   - Fix do_syscall() return value (Ammar).

#### Changelog v4:

   - Fix the assertion condition inside the SIGUSR1 handler (Xin Li).

   - Explain the purpose of patch #2 in the commit message (HPA).

   - Update commit message (Ammar).

   - Repeat test_syscall_rcx_r11_consistent() 32 times to be more sure
     that the result is really consistent (Ammar).

#### Changelog v3:

   - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
     which is a major part of the point (HPA).

#### Changelog v2:

   - Use "+r"(rsp) as the right way to avoid redzone problems
     per Andrew's comment (HPA).


Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  selftests/x86: sysret_rip: Handle syscall in a FRED system
  selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

 tools/testing/selftests/x86/sysret_rip.c | 146 +++++++++++++++++++++--
 1 file changed, 137 insertions(+), 9 deletions(-)


base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
-- 
Ammar Faizi

[RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

The current selftest asserts (%r11 == %rflags) after the 'syscall'
returns to user. Such an assertion doesn't apply to the FRED system
because in that system the 'syscall' instruction does not set
%r11=%rflags and %rcx=%rip.

Handle the FRED case. Now, test that:

  - "syscall" in a FRED system doesn't clobber %rcx and %r11.
  - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

The 'raise()' function from libc can't be used to control those
registers. Therefore, create a syscall wrapper in inline Assembly to
fully control them.

Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 126 +++++++++++++++++++++--
 1 file changed, 119 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d90207ab..d3dc5ec496854179 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -39,6 +39,112 @@ asm (
 extern const char test_page[];
 static void const *current_test_page_addr = test_page;
 
+/* Arbitrary values */
+static const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
+static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
+
+/* An arbitrary *valid* RFLAGS value */
+static const unsigned long rflags_sentinel = 0x200a93;
+
+enum regs_ok {
+	REGS_UNDEFINED	= -2,	/* For consistency checker init, never returned */
+	REGS_ERROR	= -1,	/* Invalid register contents */
+	REGS_SAVED	=  0,	/* Registers properly preserved */
+	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
+};
+
+/*
+ * Returns:
+ *  0 = %rcx and %r11 preserved.
+ *  1 = %rcx and %r11 set to %rflags and %rip.
+ * -1 = %rcx and/or %r11 set to any other values.
+ *
+ * @rbx should be set to the syscall return %rip.
+ */
+static enum regs_ok check_regs_result(unsigned long r11, unsigned long rcx,
+				      unsigned long rbx)
+{
+	if (r11 == r11_sentinel && rcx == rcx_sentinel)
+		return REGS_SAVED;
+
+	if (r11 == rflags_sentinel && rcx == rbx)
+		return REGS_SYSRET;
+
+	printf("[FAIL] check_regs_result\n");
+	printf("        r11_sentinel = %#lx; %%r11 = %#lx;\n", r11_sentinel, r11);
+	printf("        rcx_sentinel = %#lx; %%rcx = %#lx;\n", rcx_sentinel, rcx);
+	printf("        rflags_sentinel = %#lx\n", rflags_sentinel);
+	return REGS_ERROR;
+}
+
+static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
+		       unsigned long arg3, unsigned long arg4,
+		       unsigned long arg5, unsigned long arg6)
+{
+	static enum regs_ok regs_ok_state = REGS_UNDEFINED;
+	register unsigned long r11 asm("%r11");
+	register unsigned long r10 asm("%r10");
+	register unsigned long r8 asm("%r8");
+	register unsigned long r9 asm("%r9");
+	register void *rsp asm("%rsp");
+	unsigned long rcx, rbx;
+	enum regs_ok ret;
+
+	r11 = r11_sentinel;
+	rcx = rcx_sentinel;
+	r10 = arg4;
+	r8 = arg5;
+	r9 = arg6;
+
+	asm volatile (
+		"pushq	%[rflags_sentinel]\n\t"
+		"popf\n\t"
+		"leaq	1f(%%rip), %[rbx]\n\t"
+		"syscall\n"
+		"1:"
+
+		: "+a" (nr_syscall),
+		  "+r" (r11),
+		  "+c" (rcx),
+		  [rbx] "=b" (rbx),
+		  "+r" (rsp)	/* Clobber the redzone */
+
+		: [rflags_sentinel] "g" (rflags_sentinel),
+		  "D" (arg1),	/* %rdi */
+		  "S" (arg2),	/* %rsi */
+		  "d" (arg3),	/* %rdx */
+		  "r" (r10),
+		  "r" (r8),
+		  "r" (r9)
+
+		: "memory"
+	);
+
+	/*
+	 * Test that:
+	 *
+	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
+	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
+	 */
+	ret = check_regs_result(r11, rcx, rbx);
+	assert(ret != REGS_ERROR);
+
+	/*
+	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
+	 * It needs at least calling do_syscall() twice to assert.
+	 */
+	if (regs_ok_state == REGS_UNDEFINED) {
+		/*
+		 * First time calling do_syscall().
+		 */
+		regs_ok_state = ret;
+	} else {
+		assert(regs_ok_state == ret);
+	}
+
+	return nr_syscall;
+}
+
 static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
 		       int flags)
 {
@@ -85,27 +191,33 @@ static void sigsegv_for_sigreturn_test(int sig, siginfo_t *info, void *ctx_void)
 static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 {
 	ucontext_t *ctx = (ucontext_t*)ctx_void;
+	enum regs_ok ret;
 
 	memcpy(&initial_regs, &ctx->uc_mcontext.gregs, sizeof(gregset_t));
 
+	ret = check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
+				ctx->uc_mcontext.gregs[REG_RCX],
+				ctx->uc_mcontext.gregs[REG_RBX]);
+
+	assert(ret != REGS_ERROR);
+
 	/* Set IP and CX to match so that SYSRET can happen. */
 	ctx->uc_mcontext.gregs[REG_RIP] = rip;
 	ctx->uc_mcontext.gregs[REG_RCX] = rip;
-
-	/* R11 and EFLAGS should already match. */
-	assert(ctx->uc_mcontext.gregs[REG_EFL] ==
-	       ctx->uc_mcontext.gregs[REG_R11]);
-
 	sethandler(SIGSEGV, sigsegv_for_sigreturn_test, SA_RESETHAND);
 
-	return;
+}
+
+static void __raise(int sig)
+{
+	do_syscall(__NR_kill, getpid(), sig, 0, 0, 0, 0);
 }
 
 static void test_sigreturn_to(unsigned long ip)
 {
 	rip = ip;
 	printf("[RUN]\tsigreturn to 0x%lx\n", ip);
-	raise(SIGUSR1);
+	__raise(SIGUSR1);
 }
 
 static jmp_buf jmpbuf;
-- 
Ammar Faizi

Re: [RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[H. Peter Anvin]

On 1/24/23 19:49, Ammar Faizi wrote:
> +
> +	/*
> +	 * Test that:
> +	 *
> +	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
> +	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
> +	 */
> +	ret = check_regs_result(r11, rcx, rbx);
> +	assert(ret != REGS_ERROR);
> +
> +	/*
> +	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
> +	 * It needs at least calling do_syscall() twice to assert.
> +	 */
> +	if (regs_ok_state == REGS_UNDEFINED) {
> +		/*
> +		 * First time calling do_syscall().
> +		 */
> +		regs_ok_state = ret;
> +	} else {
> +		assert(regs_ok_state == ret);
> +	}
> +

[...]

> +	ret = check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
> +				ctx->uc_mcontext.gregs[REG_RCX],
> +				ctx->uc_mcontext.gregs[REG_RBX]);
> +
> +	assert(ret != REGS_ERROR);
> +

This instance, too, needs to be checked against regs_ok_result. It would 
make most sense to move that handling, and the assert() into 
check_regs_result() or into a separate function around it.

>   	/* Set IP and CX to match so that SYSRET can happen. */
>   	ctx->uc_mcontext.gregs[REG_RIP] = rip;
>   	ctx->uc_mcontext.gregs[REG_RCX] = rip;

It would be interesting to have the syscall handler try both with and 
without this (so it would end up doing both IRET and SYSCALL on legacy.) 
Perhaps SIGUSR1 versus SIGUSR2...

	-hpa

Re: [RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Wed, Jan 25, 2023 at 12:39:26AM -0800, H. Peter Anvin wrote:
> > +	ret = check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
> > +				ctx->uc_mcontext.gregs[REG_RCX],
> > +				ctx->uc_mcontext.gregs[REG_RBX]);
> > +
> > +	assert(ret != REGS_ERROR);
> > +
> 
> This instance, too, needs to be checked against regs_ok_result. It would
> make most sense to move that handling, and the assert() into
> check_regs_result() or into a separate function around it.

OK. Sounds better.
 
> >   	/* Set IP and CX to match so that SYSRET can happen. */
> >   	ctx->uc_mcontext.gregs[REG_RIP] = rip;
> >   	ctx->uc_mcontext.gregs[REG_RCX] = rip;
> 
> It would be interesting to have the syscall handler try both with and
> without this (so it would end up doing both IRET and SYSCALL on legacy.)
> Perhaps SIGUSR1 versus SIGUSR2...

We will have a new separate patch for that.

-- 
Ammar Faizi

Re: [RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Wed, Jan 25, 2023 at 12:39:26AM -0800, H. Peter Anvin wrote:
> >   	/* Set IP and CX to match so that SYSRET can happen. */
> >   	ctx->uc_mcontext.gregs[REG_RIP] = rip;
> >   	ctx->uc_mcontext.gregs[REG_RCX] = rip;
> 
> It would be interesting to have the syscall handler try both with and
> without this (so it would end up doing both IRET and SYSCALL on legacy.)
> Perhaps SIGUSR1 versus SIGUSR2...

Just to clarify this more so I am sure I understand it correctly.

Did you mean to have the same signal handler without modifiying
'REG_RCX' but still change 'REG_RIP'?

IOW, we want to only *remove*:

   ctx->uc_mcontext.gregs[REG_RCX] = rip;

and *keep*:

   ctx->uc_mcontext.gregs[REG_RIP] = rip;

for the SIGUSR2 handler. Thus, inside the entry64 we will jump to the
iret path because %rcx != %r11 upon rt_sigreturn()?

-- 
Ammar Faizi

Re: [RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On 1/25/23 4:57 PM, Ammar Faizi wrote:
> On Wed, Jan 25, 2023 at 12:39:26AM -0800, H. Peter Anvin wrote:
>>>    	/* Set IP and CX to match so that SYSRET can happen. */
>>>    	ctx->uc_mcontext.gregs[REG_RIP] = rip;
>>>    	ctx->uc_mcontext.gregs[REG_RCX] = rip;
>>
>> It would be interesting to have the syscall handler try both with and
>> without this (so it would end up doing both IRET and SYSCALL on legacy.)
>> Perhaps SIGUSR1 versus SIGUSR2...
> 
> Just to clarify this more so I am sure I understand it correctly.
> 
> Did you mean to have the same signal handler without modifiying
> 'REG_RCX' but still change 'REG_RIP'?
> 
> IOW, we want to only *remove*:
> 
>     ctx->uc_mcontext.gregs[REG_RCX] = rip;
> 
> and *keep*:
> 
>     ctx->uc_mcontext.gregs[REG_RIP] = rip;
> 
> for the SIGUSR2 handler. Thus, inside the entry64 we will jump to the
> iret path because %rcx != %r11 upon rt_sigreturn()?

s/%rcx != %r11/%rcx != %rip/

-- 
Ammar Faizi

Re: [RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[H. Peter Anvin]

On January 25, 2023 1:57:15 AM PST, Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
>On Wed, Jan 25, 2023 at 12:39:26AM -0800, H. Peter Anvin wrote:
>> >   	/* Set IP and CX to match so that SYSRET can happen. */
>> >   	ctx->uc_mcontext.gregs[REG_RIP] = rip;
>> >   	ctx->uc_mcontext.gregs[REG_RCX] = rip;
>> 
>> It would be interesting to have the syscall handler try both with and
>> without this (so it would end up doing both IRET and SYSCALL on legacy.)
>> Perhaps SIGUSR1 versus SIGUSR2...
>
>Just to clarify this more so I am sure I understand it correctly.
>
>Did you mean to have the same signal handler without modifiying
>'REG_RCX' but still change 'REG_RIP'?
>
>IOW, we want to only *remove*:
>
>   ctx->uc_mcontext.gregs[REG_RCX] = rip;
>
>and *keep*:
>
>   ctx->uc_mcontext.gregs[REG_RIP] = rip;
>
>for the SIGUSR2 handler. Thus, inside the entry64 we will jump to the
>iret path because %rcx != %r11 upon rt_sigreturn()?
>

I guess it would depend on what they "normally" are. My #1 impulse would be to leave them both unchanged.

Re: [RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Wed, Jan 25, 2023 at 02:17:41AM -0800, H. Peter Anvin wrote:
> I guess it would depend on what they "normally" are. My #1 impulse would be to leave them both unchanged.

Ah okay... I think I understand now. My confusion came from a comment
in that code.

The current SIGUSR1 handler has a comment:

    /* Set IP and CX to match so that SYSRET can happen. */
    ctx->uc_mcontext.gregs[REG_RIP] = rip;
    ctx->uc_mcontext.gregs[REG_RCX] = rip;

So I thought if we leave them both unchanged, then SYSRET can happen
too, because IP and CX match. My initial confusion about that was:

    Where do we actually exercise IRET if the SIGUSR2 handler
    exercises SYSRET then?

I realized my assumption was wrong. The current SIGUSR1 handler
actually forces the kernel to use IRET, not SYSRET. Because the %rip
is set to a non-canonical address. So that's the place where it
exercises IRET.

IOW, my understanding now:

The current SIGUSR1 handler exercises the SYSRET-appropriate condition
detector in the kernel. It doesn't actually go to the SYSRET path
despite the comment saying "SYSRET can happen". That detector must take
us to the IRET path or we will #GP in kernel space on Intel CPUs.

In short, the SIGUSR1 handler asserts that "SYSRET must *not* happen".

The expected SIGUSR2 handler addition exercises the SYSRET path by
leaving REG_IP and REG_CX unchanged.

Am I correct?

-- 
Ammar Faizi

Re: [RFC PATCH v5 1/2] selftests/x86: sysret_rip: Handle syscall in a FRED system

[H. Peter Anvin]

On January 25, 2023 3:37:23 AM PST, Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
>On Wed, Jan 25, 2023 at 02:17:41AM -0800, H. Peter Anvin wrote:
>> I guess it would depend on what they "normally" are. My #1 impulse would be to leave them both unchanged.
>
>Ah okay... I think I understand now. My confusion came from a comment
>in that code.
>
>The current SIGUSR1 handler has a comment:
>
>    /* Set IP and CX to match so that SYSRET can happen. */
>    ctx->uc_mcontext.gregs[REG_RIP] = rip;
>    ctx->uc_mcontext.gregs[REG_RCX] = rip;
>
>So I thought if we leave them both unchanged, then SYSRET can happen
>too, because IP and CX match. My initial confusion about that was:
>
>    Where do we actually exercise IRET if the SIGUSR2 handler
>    exercises SYSRET then?
>
>I realized my assumption was wrong. The current SIGUSR1 handler
>actually forces the kernel to use IRET, not SYSRET. Because the %rip
>is set to a non-canonical address. So that's the place where it
>exercises IRET.
>
>IOW, my understanding now:
>
>The current SIGUSR1 handler exercises the SYSRET-appropriate condition
>detector in the kernel. It doesn't actually go to the SYSRET path
>despite the comment saying "SYSRET can happen". That detector must take
>us to the IRET path or we will #GP in kernel space on Intel CPUs.
>
>In short, the SIGUSR1 handler asserts that "SYSRET must *not* happen".
>
>The expected SIGUSR2 handler addition exercises the SYSRET path by
>leaving REG_IP and REG_CX unchanged.
>
>Am I correct?
>

That's the idea.

[RFC PATCH v5 2/2] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Test that:

  - REGS_SAVED: "syscall" in a FRED system doesn't clobber %rcx and
    %r11.

  - REGS_SYSRET: "syscall" in a non-FRED system sets %rcx=%rip and
    %r11=%rflags.

Test them out with trivial system calls like __NR_getppid and friends
which are extremely likely to return with SYSRET on an IDT system.

Goals of this test:

  - Ensure that the syscall behavior is consistent. It should be either
    always REGS_SAVED or always REGS_SYSRET. Not a mix of them.

  - The kernel doesn't leak its internal data when returning to
    userspace.

Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index d3dc5ec496854179..7ebfb603bf45fa0a 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -270,8 +270,24 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 	printf("[OK]\tWe survived\n");
 }
 
+/*
+ * Ensure that various system calls are consistent.
+ * We should not get a mix of REGS_SAVED and REGS_SYSRET.
+ */
+static void test_syscall_rcx_r11_consistent(void)
+{
+	do_syscall(__NR_getpid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_gettid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
+}
+
 int main()
 {
+	int i;
+
+	for (i = 0; i < 32; i++)
+		test_syscall_rcx_r11_consistent();
+
 	/*
 	 * When the kernel returns from a slow-path syscall, it will
 	 * detect whether SYSRET is appropriate.  If it incorrectly
@@ -279,7 +295,7 @@ int main()
 	 * it'll crash on Intel CPUs.
 	 */
 	sethandler(SIGUSR1, sigusr1, 0);
-	for (int i = 47; i < 64; i++)
+	for (i = 47; i < 64; i++)
 		test_sigreturn_to(1UL<<i);
 
 	clearhandler(SIGUSR1);
@@ -290,7 +306,7 @@ int main()
 	test_syscall_fallthrough_to((1UL << 47) - 2*PAGE_SIZE);
 
 	/* These are the interesting cases. */
-	for (int i = 47; i < 64; i++) {
+	for (i = 47; i < 64; i++) {
 		test_syscall_fallthrough_to((1UL<<i) - PAGE_SIZE);
 		test_syscall_fallthrough_to(1UL<<i);
 	}
-- 
Ammar Faizi

RE: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Li, Xin3]

This version passes on FRED, thanks a lot for quickly fixing it.

  Xin

> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> This is an RFC patchset v5. There are two patches in this series.
> 
> Xin Li reported that the sysret_rip test fails at:
> 
>         assert(ctx->uc_mcontext.gregs[REG_EFL] ==
>                ctx->uc_mcontext.gregs[REG_R11]);
> 
> on the Intel FRED architecture. Let's handle the FRED system scenario too. The
> 'syscall' instruction in a FRED system doesn't set %rcx=%rip and %r11=%rflags.
> 
> Syscall and sysenter in a FRED system are treated equivalently to software
> interrupts, e.g. INT 0x80. They do not modify any registers.
> 
> Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-
> 9c001c2343af@intel.com
> 
> #### Changelog v5:
> 
>    - Fix do_syscall() return value (Ammar).
> 
> #### Changelog v4:
> 
>    - Fix the assertion condition inside the SIGUSR1 handler (Xin Li).
> 
>    - Explain the purpose of patch #2 in the commit message (HPA).
> 
>    - Update commit message (Ammar).
> 
>    - Repeat test_syscall_rcx_r11_consistent() 32 times to be more sure
>      that the result is really consistent (Ammar).
> 
> #### Changelog v3:
> 
>    - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
>      which is a major part of the point (HPA).
> 
> #### Changelog v2:
> 
>    - Use "+r"(rsp) as the right way to avoid redzone problems
>      per Andrew's comment (HPA).
> 
> 
> Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
> 
> Ammar Faizi (2):
>   selftests/x86: sysret_rip: Handle syscall in a FRED system
>   selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`
> 
>  tools/testing/selftests/x86/sysret_rip.c | 146 +++++++++++++++++++++--
>  1 file changed, 137 insertions(+), 9 deletions(-)
> 
> 
> base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
> --
> Ammar Faizi

Re: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

On Wed, Jan 25, 2023 at 08:22:48AM +0000, Li, Xin3 wrote:
> This version passes on FRED, thanks a lot for quickly fixing it.

Great!

Can you pick these two patches and include it in the next version of
"x86: enable FRED for x86-64" RFC patchset?

-- 
Ammar Faizi

RE: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Li, Xin3]

> > This version passes on FRED, thanks a lot for quickly fixing it.
> 
> Great!
> 
> Can you pick these two patches and include it in the next version of
> "x86: enable FRED for x86-64" RFC patchset?

Would it be better to get this patch set merged first?

Otherwise surely I will include it in the FRED patch set.

  Xin

 

RE: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[H. Peter Anvin]

On January 25, 2023 9:07:18 AM PST, "Li, Xin3" <xin3.li@intel.com> wrote:
>> > This version passes on FRED, thanks a lot for quickly fixing it.
>> 
>> Great!
>> 
>> Can you pick these two patches and include it in the next version of
>> "x86: enable FRED for x86-64" RFC patchset?
>
>Would it be better to get this patch set merged first?
>
>Otherwise surely I will include it in the FRED patch set.
>
>  Xin
>
> 
>

If the maintainers are ok with it, it would be better to merge it sooner: once we have agreed on the semantics, which I believe we have, we should be testing those semantics and nothing else.

Re: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

On Wed, Jan 25, 2023 at 09:24:40AM -0800, H. Peter Anvin wrote:
> On January 25, 2023 9:07:18 AM PST, "Li, Xin3" <xin3.li@intel.com> wrote:
> > Would it be better to get this patch set merged first?
> > 
> > Otherwise surely I will include it in the FRED patch set.
> 
> If the maintainers are ok with it, it would be better to merge it
> sooner: once we have agreed on the semantics, which I believe we
> have, we should be testing those semantics and nothing else.

OK, let's keep this patchset separated from the FRED support
patchset.

In the meantime, let me address the recent HPA's comments.

-- 
Ammar Faizi

RE: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Li, Xin3]

> > > Would it be better to get this patch set merged first?
> > >
> > > Otherwise surely I will include it in the FRED patch set.
> >
> > If the maintainers are ok with it, it would be better to merge it
> > sooner: once we have agreed on the semantics, which I believe we have,
> > we should be testing those semantics and nothing else.
> 
> OK, let's keep this patchset separated from the FRED support patchset.

Thanks!

This patch set first makes the R11/RCX semantics clearer, and it BTW fixes
FRED tests.

To me it's more of an improvement to the existing code.

RE: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Li, Xin3]

> > > > Would it be better to get this patch set merged first?
> > > >
> > > > Otherwise surely I will include it in the FRED patch set.
> > >
> > > If the maintainers are ok with it, it would be better to merge it
> > > sooner: once we have agreed on the semantics, which I believe we
> > > have, we should be testing those semantics and nothing else.
> >
> > OK, let's keep this patchset separated from the FRED support patchset.
> 
> Thanks!
> 
> This patch set first makes the R11/RCX semantics clearer, and it BTW fixes FRED
> tests.
> 
> To me it's more of an improvement to the existing code.

Hi Faizi,

Any update on this patch set?

Thanks!
Xin

Re: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

On Wed, Feb 15, 2023 at 07:42:47AM +0000, Li, Xin3 wrote:
> Hi Faizi,
> 
> Any update on this patch set?

Xin,

Before I send the next version, I need an answer for this one:
https://lore.kernel.org/lkml/Y9LfmQ%2Fr1%2FpEP+uv@biznet-home.integral.gnuweeb.org/

I don't think the redzone problem is handled correctly here. Using
"+r" (rsp) constraint doesn't solve the redzone problem.

HPA, Andrew, anybody?

-- 
Ammar Faizi

Re: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

On Wed, Feb 15, 2023 at 07:42:47AM +0000, Li, Xin3 wrote:
> Hi Faizi,
> 
> Any update on this patch set?

No comment from HPA. But after the recent discussion with Andrew, I
think at least it's now clear that we are not going to use "+r"(rsp) to
avoid the red zone problem.

I am on leave today. Will send revision v8 on Monday.

Thanks,

-- 
Ammar Faizi

Re: [RFC PATCH v5 0/2] sysret_rip update for the Intel FRED architecture

[H. Peter Anvin]

On February 17, 2023 8:27:40 PM PST, Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
>On Wed, Feb 15, 2023 at 07:42:47AM +0000, Li, Xin3 wrote:
>> Hi Faizi,
>> 
>> Any update on this patch set?
>
>No comment from HPA. But after the recent discussion with Andrew, I
>think at least it's now clear that we are not going to use "+r"(rsp) to
>avoid the red zone problem.
>
>I am on leave today. Will send revision v8 on Monday.
>
>Thanks,
>

My apologies, I missed your latest response in the torrent of email. The redzone issue is weird; it ought to be breaking all over the place, not just this.

Let me take a quick look at it...

[RFC PATCH v6 0/3] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Hi,

This is an RFC patchset v6. There are three patches in this series.

Xin Li reported that the sysret_rip test fails at:

        assert(ctx->uc_mcontext.gregs[REG_EFL] ==
               ctx->uc_mcontext.gregs[REG_R11]);

on the Intel FRED architecture. Let's handle the FRED system scenario
too. The 'syscall' instruction in a FRED system doesn't set %rcx=%rip
and %r11=%rflags.

Syscall and sysenter in a FRED system are treated equivalently to
software interrupts, e.g. INT 0x80. They do not modify any registers.

Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com

## Changelog v6:

   - Move the check-regs assertion in sigusr1() to check_regs_result()
     (HPA).

   - Add a new test just like sigusr1(), but don't modify REG_RCX and
     REG_R11. This is used to test SYSRET behavior consistency (HPA).

## Changelog v5:

   - Fix do_syscall() return value (Ammar).

## Changelog v4:

   - Fix the assertion condition inside the SIGUSR1 handler (Xin Li).

   - Explain the purpose of patch #2 in the commit message (HPA).

   - Update commit message (Ammar).

   - Repeat test_syscall_rcx_r11_consistent() 32 times to be more sure
     that the result is really consistent (Ammar).

## Changelog v3:

   - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
     which is a major part of the point (HPA).

## Changelog v2:

   - Use "+r"(rsp) as the right way to avoid redzone problems
     per Andrew's comment (HPA).


Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (3):
  selftests/x86: sysret_rip: Handle syscall in a FRED system
  selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`
  selftests/x86: sysret_rip: Test opportunistic SYSRET

 tools/testing/selftests/x86/sysret_rip.c | 177 +++++++++++++++++++++--
 1 file changed, 168 insertions(+), 9 deletions(-)


base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
-- 
Ammar Faizi

[RFC PATCH v6 1/3] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

The current selftest asserts (%r11 == %rflags) after the 'syscall'
returns to user. Such an assertion doesn't apply to the FRED system
because in that system the 'syscall' instruction does not set
%r11=%rflags and %rcx=%rip.

Handle the FRED case. Now, test that:

  - "syscall" in a FRED system doesn't clobber %rcx and %r11.
  - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

The 'raise()' function from libc can't be used to control those
registers. Therefore, create a syscall wrapper in inline Assembly to
fully control them.

Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 120 +++++++++++++++++++++--
 1 file changed, 113 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d90207ab..100f55981d77a29b 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -39,6 +39,110 @@ asm (
 extern const char test_page[];
 static void const *current_test_page_addr = test_page;
 
+/* Arbitrary values */
+static const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
+static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
+
+/* An arbitrary *valid* RFLAGS value */
+static const unsigned long rflags_sentinel = 0x200a93;
+
+enum regs_ok {
+	REGS_UNDEFINED	= -1,	/* For consistency checker init, never returned */
+	REGS_SAVED	=  0,	/* Registers properly preserved */
+	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
+};
+
+/*
+ * REGS_SAVED  = %rcx and %r11 preserved.
+ * REGS_SYSRET = %rcx and %r11 set to %rflags and %rip.
+ * REGS_ERROR  = %rcx and/or %r11 set to any other values.
+ *
+ * @rbx should be set to the syscall return %rip.
+ */
+static void check_regs_result(unsigned long r11, unsigned long rcx,
+			      unsigned long rbx)
+{
+	static enum regs_ok regs_ok_state = REGS_UNDEFINED;
+	enum regs_ok ret;
+
+	/*
+	 * Test that:
+	 *
+	 * - "syscall" in a FRED system doesn't clobber %rcx and %r11.
+	 * - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.
+	 */
+	if (r11 == r11_sentinel && rcx == rcx_sentinel) {
+		ret = REGS_SAVED;
+	} else if (r11 == rflags_sentinel && rcx == rbx) {
+		ret = REGS_SYSRET;
+	} else {
+		printf("[FAIL] check_regs_result\n");
+		printf("        r11_sentinel = %#lx; %%r11 = %#lx;\n", r11_sentinel, r11);
+		printf("        rcx_sentinel = %#lx; %%rcx = %#lx;\n", rcx_sentinel, rcx);
+		printf("        rflags_sentinel = %#lx\n", rflags_sentinel);
+		exit(1);
+	}
+
+
+	/*
+	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
+	 * It needs at least calling check_regs_result() twice to assert.
+	 */
+	if (regs_ok_state == REGS_UNDEFINED) {
+		/*
+		 * First time calling check_regs_result().
+		 */
+		regs_ok_state = ret;
+	} else {
+		assert(regs_ok_state == ret);
+	}
+}
+
+static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
+		       unsigned long arg3, unsigned long arg4,
+		       unsigned long arg5, unsigned long arg6)
+{
+	register unsigned long r11 asm("%r11");
+	register unsigned long r10 asm("%r10");
+	register unsigned long r8 asm("%r8");
+	register unsigned long r9 asm("%r9");
+	register void *rsp asm("%rsp");
+	unsigned long rcx, rbx;
+
+	r11 = r11_sentinel;
+	rcx = rcx_sentinel;
+	r10 = arg4;
+	r8 = arg5;
+	r9 = arg6;
+
+	asm volatile (
+		"pushq	%[rflags_sentinel]\n\t"
+		"popf\n\t"
+		"leaq	1f(%%rip), %[rbx]\n\t"
+		"syscall\n"
+		"1:"
+
+		: "+a" (nr_syscall),
+		  "+r" (r11),
+		  "+c" (rcx),
+		  [rbx] "=b" (rbx),
+		  "+r" (rsp)	/* Clobber the redzone */
+
+		: [rflags_sentinel] "g" (rflags_sentinel),
+		  "D" (arg1),	/* %rdi */
+		  "S" (arg2),	/* %rsi */
+		  "d" (arg3),	/* %rdx */
+		  "r" (r10),
+		  "r" (r8),
+		  "r" (r9)
+
+		: "memory"
+	);
+
+	check_regs_result(r11, rcx, rbx);
+	return nr_syscall;
+}
+
 static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
 		       int flags)
 {
@@ -88,24 +192,26 @@ static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 
 	memcpy(&initial_regs, &ctx->uc_mcontext.gregs, sizeof(gregset_t));
 
+	check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
+			  ctx->uc_mcontext.gregs[REG_RCX],
+			  ctx->uc_mcontext.gregs[REG_RBX]);
+
 	/* Set IP and CX to match so that SYSRET can happen. */
 	ctx->uc_mcontext.gregs[REG_RIP] = rip;
 	ctx->uc_mcontext.gregs[REG_RCX] = rip;
-
-	/* R11 and EFLAGS should already match. */
-	assert(ctx->uc_mcontext.gregs[REG_EFL] ==
-	       ctx->uc_mcontext.gregs[REG_R11]);
-
 	sethandler(SIGSEGV, sigsegv_for_sigreturn_test, SA_RESETHAND);
+}
 
-	return;
+static void __raise(int sig)
+{
+	do_syscall(__NR_kill, getpid(), sig, 0, 0, 0, 0);
 }
 
 static void test_sigreturn_to(unsigned long ip)
 {
 	rip = ip;
 	printf("[RUN]\tsigreturn to 0x%lx\n", ip);
-	raise(SIGUSR1);
+	__raise(SIGUSR1);
 }
 
 static jmp_buf jmpbuf;
-- 
Ammar Faizi

Re: [RFC PATCH v6 1/3] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

On Thu, Jan 26, 2023 at 04:17:12AM +0700, Ammar Faizi wrote:
> +/*
> + * REGS_SAVED  = %rcx and %r11 preserved.
> + * REGS_SYSRET = %rcx and %r11 set to %rflags and %rip.
> + * REGS_ERROR  = %rcx and/or %r11 set to any other values.

Since we moved the assertion into check_regs_result(), we no longer
need REGS_ERROR. check_regs_result() is now a void function.

Will remove that comment...

-- 
Ammar Faizi

[RFC PATCH v6 2/3] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

Test that:

  - REGS_SAVED: "syscall" in a FRED system doesn't clobber %rcx and
    %r11.

  - REGS_SYSRET: "syscall" in a non-FRED system sets %rcx=%rip and
    %r11=%rflags.

Test them out with trivial system calls like __NR_getppid and friends
which are extremely likely to return with SYSRET on an IDT system.

Goals of this test:

  - Ensure that the syscall behavior is consistent. It should be either
    always REGS_SAVED or always REGS_SYSRET. Not a mix of them.

  - The kernel doesn't leak its internal data when returning to
    userspace.

Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 100f55981d77a29b..d45b7f0147cd25ad 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -264,8 +264,24 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 	printf("[OK]\tWe survived\n");
 }
 
+/*
+ * Ensure that various system calls are consistent.
+ * We should not get a mix of REGS_SAVED and REGS_SYSRET.
+ */
+static void test_syscall_rcx_r11_consistent(void)
+{
+	do_syscall(__NR_getpid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_gettid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
+}
+
 int main()
 {
+	int i;
+
+	for (i = 0; i < 32; i++)
+		test_syscall_rcx_r11_consistent();
+
 	/*
 	 * When the kernel returns from a slow-path syscall, it will
 	 * detect whether SYSRET is appropriate.  If it incorrectly
@@ -273,7 +289,7 @@ int main()
 	 * it'll crash on Intel CPUs.
 	 */
 	sethandler(SIGUSR1, sigusr1, 0);
-	for (int i = 47; i < 64; i++)
+	for (i = 47; i < 64; i++)
 		test_sigreturn_to(1UL<<i);
 
 	clearhandler(SIGUSR1);
@@ -284,7 +300,7 @@ int main()
 	test_syscall_fallthrough_to((1UL << 47) - 2*PAGE_SIZE);
 
 	/* These are the interesting cases. */
-	for (int i = 47; i < 64; i++) {
+	for (i = 47; i < 64; i++) {
 		test_syscall_fallthrough_to((1UL<<i) - PAGE_SIZE);
 		test_syscall_fallthrough_to(1UL<<i);
 	}
-- 
Ammar Faizi

[RFC PATCH v6 3/3] selftests/x86: sysret_rip: Test opportunistic SYSRET

[Ammar Faizi]

When run on a non-FRED system, test the opportunistic SYSRET fast-path.
Make sure the %rcx/%r11 clobbering behavior is consistent.

When run on a FRED system, test that %rcx/%r11 are preserved when
invoking syscall.

This is similar to what test_syscall_rcx_r11_consistent() is doing, but
with addition it's done via the SIGUSR2 signal handler.

Link: https://lore.kernel.org/lkml/8770815f-0f23-d0c5-e56a-d401827842c9@zytor.com
Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

On Wed, 25 Jan 2023 00:39:26 -0800, "H. Peter Anvin" wrote:
> >    /* Set IP and CX to match so that SYSRET can happen. */
> >     ctx->uc_mcontext.gregs[REG_RIP] = rip;
> >     ctx->uc_mcontext.gregs[REG_RCX] = rip;
> 
> It would be interesting to have the syscall handler try both with and 
> without this (so it would end up doing both IRET and SYSCALL on legacy.) 
> Perhaps SIGUSR1 versus SIGUSR2...


 tools/testing/selftests/x86/sysret_rip.c | 37 ++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index d45b7f0147cd25ad..a1e5ec6f08bcd523 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -275,6 +275,28 @@ static void test_syscall_rcx_r11_consistent(void)
 	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
 }
 
+static unsigned long usr2_rcx;
+static unsigned long usr2_r11;
+
+static void sigusr2(int sig, siginfo_t *info, void *ctx_void)
+{
+	ucontext_t *ctx = (ucontext_t*)ctx_void;
+
+	usr2_r11 = ctx->uc_mcontext.gregs[REG_R11];
+	usr2_rcx = ctx->uc_mcontext.gregs[REG_RCX];
+
+	check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
+			  ctx->uc_mcontext.gregs[REG_RCX],
+			  ctx->uc_mcontext.gregs[REG_RBX]);
+}
+
+static void test_sysret_consistent(void)
+{
+	printf("[RUN]\ttest_sysret_consistent\n");
+	__raise(SIGUSR2);
+	printf("[OK]\tRCX = %#lx;  R11 = %#lx\n", usr2_rcx, usr2_r11);
+}
+
 int main()
 {
 	int i;
@@ -292,6 +314,21 @@ int main()
 	for (i = 47; i < 64; i++)
 		test_sigreturn_to(1UL<<i);
 
+	/*
+	 *
+	 * When run on a non-FRED system, test the SYSRET path. Make
+	 * sure the %rcx/%r11 clobbering behavior is consistent.
+	 *
+	 * When run on a FRED system, test that %rcx/%r11 are preserved
+	 * when invoking syscall.
+	 *
+	 * This is similar to test_syscall_rcx_r11_consistent(), but via
+	 * a signal handler.
+	 */
+	sethandler(SIGUSR2, sigusr2, 0);
+	for (i = 0; i < 32; i++)
+		test_sysret_consistent();
+
 	clearhandler(SIGUSR1);
 
 	sethandler(SIGSEGV, sigsegv_for_fallthrough, 0);
-- 
Ammar Faizi

[RFC PATCH v7 0/3] sysret_rip update for the Intel FRED architecture

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Hi,

This is an RFC patchset v7. There are three patches in this series.

Xin Li reported that the sysret_rip test fails at:

        assert(ctx->uc_mcontext.gregs[REG_EFL] ==
               ctx->uc_mcontext.gregs[REG_R11]);

on the Intel FRED architecture. Let's handle the FRED system scenario
too. The 'syscall' instruction in a FRED system doesn't set %rcx=%rip
and %r11=%rflags.

Syscall and sysenter in a FRED system are treated equivalently to
software interrupts, e.g. INT 0x80. They do not modify any registers.

Link: https://lore.kernel.org/lkml/5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com

## Changelog v7:

   - Fix comment, REGS_ERROR no longer exists in the enum (Ammar).

   - Update commit message (Ammar).

## Changelog v6:

   - Move the check-regs assertion in sigusr1() to check_regs_result()
     (HPA).

   - Add a new test just like sigusr1(), but don't modify REG_RCX and
     REG_R11. This is used to test SYSRET behavior consistency (HPA).

## Changelog v5:

   - Fix do_syscall() return value (Ammar).

## Changelog v4:

   - Fix the assertion condition inside the SIGUSR1 handler (Xin Li).

   - Explain the purpose of patch #2 in the commit message (HPA).

   - Update commit message (Ammar).

   - Repeat test_syscall_rcx_r11_consistent() 32 times to be more sure
     that the result is really consistent (Ammar).

## Changelog v3:

   - Test that we don't get a mix of REGS_SAVED and REGS_SYSRET,
     which is a major part of the point (HPA).

## Changelog v2:

   - Use "+r"(rsp) as the right way to avoid redzone problems
     per Andrew's comment (HPA).


Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (3):
  selftests/x86: sysret_rip: Handle syscall in a FRED system
  selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`
  selftests/x86: sysret_rip: Test SYSRET with a signal handler

 tools/testing/selftests/x86/sysret_rip.c | 171 +++++++++++++++++++++--
 1 file changed, 162 insertions(+), 9 deletions(-)


base-commit: e12ad468c22065a2826b2fc4c11d2113a7975301
-- 
Ammar Faizi

[RFC PATCH v7 1/3] selftests/x86: sysret_rip: Handle syscall in a FRED system

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

The current selftest asserts (%r11 == %rflags) after the 'syscall'
returns to user. Such an assertion doesn't apply to the FRED system
because in that system the 'syscall' instruction does not set
%r11=%rflags and %rcx=%rip.

Handle the FRED case. Now, test that:

  - "syscall" in a FRED system doesn't clobber %rcx and %r11.
  - "syscall" in a non-FRED system sets %rcx=%rip and %r11=%rflags.

The 'raise()' function from libc can't be used to control those
registers. Therefore, create a syscall wrapper in inline Assembly to
fully control them.

Fixes: 660602140103 ("selftests/x86: Add a selftest for SYSRET to noncanonical addresses")
Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Reported-by: Xin Li <xin3.li@intel.com>
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 114 +++++++++++++++++++++--
 1 file changed, 107 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index 84d74be1d90207ab..ef3f492d95f6f2a1 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -39,6 +39,104 @@ asm (
 extern const char test_page[];
 static void const *current_test_page_addr = test_page;
 
+/* Arbitrary values */
+static const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
+static const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;
+
+/* An arbitrary *valid* RFLAGS value */
+static const unsigned long rflags_sentinel = 0x200a93;
+
+enum regs_ok {
+	REGS_UNDEFINED	= -1,	/* For consistency checker init, never returned */
+	REGS_SAVED	=  0,	/* Registers properly preserved */
+	REGS_SYSRET	=  1	/* Registers match syscall/sysret */
+};
+
+/*
+ * @rbx should be set to the syscall return %rip.
+ */
+static void check_regs_result(unsigned long r11, unsigned long rcx,
+			      unsigned long rbx)
+{
+	static enum regs_ok regs_ok_state = REGS_UNDEFINED;
+	enum regs_ok ret;
+
+	/*
+	 * REGS_SAVED  = %rcx and %r11 preserved (Intel FRED).
+	 * REGS_SYSRET = %rcx and %r11 set to %rflags and %rip.
+	 */
+	if (r11 == r11_sentinel && rcx == rcx_sentinel) {
+		ret = REGS_SAVED;
+	} else if (r11 == rflags_sentinel && rcx == rbx) {
+		ret = REGS_SYSRET;
+	} else {
+		printf("[FAIL] check_regs_result\n");
+		printf("        r11_sentinel = %#lx; %%r11 = %#lx;\n", r11_sentinel, r11);
+		printf("        rcx_sentinel = %#lx; %%rcx = %#lx;\n", rcx_sentinel, rcx);
+		printf("        rflags_sentinel = %#lx\n", rflags_sentinel);
+		exit(1);
+	}
+
+
+	/*
+	 * Test that we don't get a mix of REGS_SAVED and REGS_SYSRET.
+	 * It needs at least calling check_regs_result() twice to assert.
+	 */
+	if (regs_ok_state == REGS_UNDEFINED) {
+		/*
+		 * First time calling check_regs_result().
+		 */
+		regs_ok_state = ret;
+	} else {
+		assert(regs_ok_state == ret);
+	}
+}
+
+static long do_syscall(long nr_syscall, unsigned long arg1, unsigned long arg2,
+		       unsigned long arg3, unsigned long arg4,
+		       unsigned long arg5, unsigned long arg6)
+{
+	register unsigned long r11 asm("%r11");
+	register unsigned long r10 asm("%r10");
+	register unsigned long r8 asm("%r8");
+	register unsigned long r9 asm("%r9");
+	register void *rsp asm("%rsp");
+	unsigned long rcx, rbx;
+
+	r11 = r11_sentinel;
+	rcx = rcx_sentinel;
+	r10 = arg4;
+	r8 = arg5;
+	r9 = arg6;
+
+	asm volatile (
+		"pushq	%[rflags_sentinel]\n\t"
+		"popf\n\t"
+		"leaq	1f(%%rip), %[rbx]\n\t"
+		"syscall\n"
+		"1:"
+
+		: "+a" (nr_syscall),
+		  "+r" (r11),
+		  "+c" (rcx),
+		  [rbx] "=b" (rbx),
+		  "+r" (rsp)	/* Clobber the redzone */
+
+		: [rflags_sentinel] "g" (rflags_sentinel),
+		  "D" (arg1),	/* %rdi */
+		  "S" (arg2),	/* %rsi */
+		  "d" (arg3),	/* %rdx */
+		  "r" (r10),
+		  "r" (r8),
+		  "r" (r9)
+
+		: "memory"
+	);
+
+	check_regs_result(r11, rcx, rbx);
+	return nr_syscall;
+}
+
 static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
 		       int flags)
 {
@@ -88,24 +186,26 @@ static void sigusr1(int sig, siginfo_t *info, void *ctx_void)
 
 	memcpy(&initial_regs, &ctx->uc_mcontext.gregs, sizeof(gregset_t));
 
+	check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
+			  ctx->uc_mcontext.gregs[REG_RCX],
+			  ctx->uc_mcontext.gregs[REG_RBX]);
+
 	/* Set IP and CX to match so that SYSRET can happen. */
 	ctx->uc_mcontext.gregs[REG_RIP] = rip;
 	ctx->uc_mcontext.gregs[REG_RCX] = rip;
-
-	/* R11 and EFLAGS should already match. */
-	assert(ctx->uc_mcontext.gregs[REG_EFL] ==
-	       ctx->uc_mcontext.gregs[REG_R11]);
-
 	sethandler(SIGSEGV, sigsegv_for_sigreturn_test, SA_RESETHAND);
+}
 
-	return;
+static void __raise(int sig)
+{
+	do_syscall(__NR_kill, getpid(), sig, 0, 0, 0, 0);
 }
 
 static void test_sigreturn_to(unsigned long ip)
 {
 	rip = ip;
 	printf("[RUN]\tsigreturn to 0x%lx\n", ip);
-	raise(SIGUSR1);
+	__raise(SIGUSR1);
 }
 
 static jmp_buf jmpbuf;
-- 
Ammar Faizi

[RFC PATCH v7 2/3] selftests/x86: sysret_rip: Add more syscall tests with respect to `%rcx` and `%r11`

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Test that:

  - REGS_SAVED: "syscall" in a FRED system doesn't clobber %rcx and
    %r11.

  - REGS_SYSRET: "syscall" in a non-FRED system sets %rcx=%rip and
    %r11=%rflags.

Test them out with trivial system calls like __NR_getppid and friends
which are extremely likely to return with SYSRET on an IDT system.

Goals of this test:

  - Ensure that the syscall behavior is consistent. It must be either
    always REGS_SAVED or always REGS_SYSRET. Not a mix of them.

  - The kernel doesn't leak its internal data when returning to
    userspace.

Link: https://lore.kernel.org/lkml/25b96960-a07e-a952-5c23-786b55054126@zytor.com
Co-developed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 tools/testing/selftests/x86/sysret_rip.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index ef3f492d95f6f2a1..d688cb9d5ac919eb 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -258,8 +258,24 @@ static void test_syscall_fallthrough_to(unsigned long ip)
 	printf("[OK]\tWe survived\n");
 }
 
+/*
+ * Ensure that various system calls are consistent.
+ * We must not get a mix of REGS_SAVED and REGS_SYSRET.
+ */
+static void test_syscall_rcx_r11_consistent(void)
+{
+	do_syscall(__NR_getpid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_gettid, 0, 0, 0, 0, 0, 0);
+	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
+}
+
 int main()
 {
+	int i;
+
+	for (i = 0; i < 32; i++)
+		test_syscall_rcx_r11_consistent();
+
 	/*
 	 * When the kernel returns from a slow-path syscall, it will
 	 * detect whether SYSRET is appropriate.  If it incorrectly
@@ -267,7 +283,7 @@ int main()
 	 * it'll crash on Intel CPUs.
 	 */
 	sethandler(SIGUSR1, sigusr1, 0);
-	for (int i = 47; i < 64; i++)
+	for (i = 47; i < 64; i++)
 		test_sigreturn_to(1UL<<i);
 
 	clearhandler(SIGUSR1);
@@ -278,7 +294,7 @@ int main()
 	test_syscall_fallthrough_to((1UL << 47) - 2*PAGE_SIZE);
 
 	/* These are the interesting cases. */
-	for (int i = 47; i < 64; i++) {
+	for (i = 47; i < 64; i++) {
 		test_syscall_fallthrough_to((1UL<<i) - PAGE_SIZE);
 		test_syscall_fallthrough_to(1UL<<i);
 	}
-- 
Ammar Faizi

[RFC PATCH v7 3/3] selftests/x86: sysret_rip: Test SYSRET with a signal handler

[Ammar Faizi]

From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

When run on a non-FRED system, test the SYSRET path. Make sure the
%rcx/%r11 clobbering behavior is consistent.

When run on a FRED system, test that %rcx/%r11 are preserved when
invoking syscall.

This is similar to what test_syscall_rcx_r11_consistent() is doing, but
with addition it's done via the SIGUSR2 signal handler.

Link: https://lore.kernel.org/lkml/8770815f-0f23-d0c5-e56a-d401827842c9@zytor.com
Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

On Wed, 25 Jan 2023 00:39:26 -0800, "H. Peter Anvin" wrote:
> >    /* Set IP and CX to match so that SYSRET can happen. */
> >     ctx->uc_mcontext.gregs[REG_RIP] = rip;
> >     ctx->uc_mcontext.gregs[REG_RCX] = rip;
> 
> It would be interesting to have the syscall handler try both with and 
> without this (so it would end up doing both IRET and SYSCALL on legacy.) 
> Perhaps SIGUSR1 versus SIGUSR2...

 tools/testing/selftests/x86/sysret_rip.c | 37 ++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tools/testing/selftests/x86/sysret_rip.c b/tools/testing/selftests/x86/sysret_rip.c
index d688cb9d5ac919eb..630ee261559c14b1 100644
--- a/tools/testing/selftests/x86/sysret_rip.c
+++ b/tools/testing/selftests/x86/sysret_rip.c
@@ -269,6 +269,28 @@ static void test_syscall_rcx_r11_consistent(void)
 	do_syscall(__NR_getppid, 0, 0, 0, 0, 0, 0);
 }
 
+static unsigned long usr2_rcx;
+static unsigned long usr2_r11;
+
+static void sigusr2(int sig, siginfo_t *info, void *ctx_void)
+{
+	ucontext_t *ctx = (ucontext_t*)ctx_void;
+
+	usr2_r11 = ctx->uc_mcontext.gregs[REG_R11];
+	usr2_rcx = ctx->uc_mcontext.gregs[REG_RCX];
+
+	check_regs_result(ctx->uc_mcontext.gregs[REG_R11],
+			  ctx->uc_mcontext.gregs[REG_RCX],
+			  ctx->uc_mcontext.gregs[REG_RBX]);
+}
+
+static void test_sysret_consistent(void)
+{
+	printf("[RUN]\ttest_sysret_consistent\n");
+	__raise(SIGUSR2);
+	printf("[OK]\tRCX = %#lx;  R11 = %#lx\n", usr2_rcx, usr2_r11);
+}
+
 int main()
 {
 	int i;
@@ -286,6 +308,21 @@ int main()
 	for (i = 47; i < 64; i++)
 		test_sigreturn_to(1UL<<i);
 
+	/*
+	 *
+	 * When run on a non-FRED system, test the SYSRET path. Make
+	 * sure the %rcx/%r11 clobbering behavior is consistent.
+	 *
+	 * When run on a FRED system, test that %rcx/%r11 are preserved
+	 * when invoking syscall.
+	 *
+	 * This is similar to test_syscall_rcx_r11_consistent(), but via
+	 * a signal handler.
+	 */
+	sethandler(SIGUSR2, sigusr2, 0);
+	for (i = 0; i < 32; i++)
+		test_sysret_consistent();
+
 	clearhandler(SIGUSR1);
 
 	sethandler(SIGSEGV, sigsegv_for_fallthrough, 0);
-- 
Ammar Faizi