[kvm-unit-tests PATCH 0/8] x86: CET fixes and enhancements

[kvm-unit-tests PATCH 0/8] x86: CET fixes and enhancements

[Mathias Krause]

Hi,

I'm playing with the CET virtualization patch set[1] and was looking at
the CET tests and noticed a few obvious issues with it (flushing the
wrong address) as well as some missing parts (testing far rets).

[1] https://lore.kernel.org/kvm/20240219074733.122080-1-weijiang.yang@intel.com/

Below is a small series with fixes and cleanups.

Please apply!

Thanks,
Mathias


Mathias Krause (8):
  x86: Avoid top-most page for vmalloc on x86-64
  x86/cet: Fix flushing shadow stack mapping
  x86/cet: Use NONCANONICAL for non-canonical address
  x86/cet: Make shadow stack less fragile
  x86/cet: Avoid unnecessary function pointer casts
  x86/cet: Simplify IBT test
  x86/cet: Track and verify #CP error code
  x86/cet: Test far returns too

 lib/x86/vm.c |  2 ++
 x86/cet.c    | 81 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 64 insertions(+), 19 deletions(-)

-- 
2.47.2

[kvm-unit-tests PATCH 1/8] x86: Avoid top-most page for vmalloc on x86-64

[Mathias Krause]

The x86-64 implementation of setup_mmu() doesn't initialize 'vfree_top'
and leaves it at its zero-value. This isn't wrong per se, however, it
leads to odd configurations when the first vmalloc/vmap page gets
allocated. It'll be the very last page in the virtual address space --
which is an interesting corner case -- but its boundary will probably
wrap. It does so for CET's shadow stack, at least, which loads the
shadow stack pointer with the base address of the mapped page plus its
size, i.e. 0xffffffff_fffff000 + 4096, which wraps to 0x0.

The CPU seems to handle such configurations just fine. However, it feels
odd to set the shadow stack pointer to "NULL".

To avoid the wrapping, ignore the top most page by initializing
'vfree_top' to just one page below.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 lib/x86/vm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 90f73fbb2dfd..27e7bb4004ef 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -191,6 +191,8 @@ void *setup_mmu(phys_addr_t end_of_memory, void *opt_mask)
         end_of_memory = (1ul << 32);  /* map mmio 1:1 */
 
     setup_mmu_range(cr3, 0, end_of_memory);
+    /* skip the last page for out-of-bound and wrap-around reasons */
+    init_alloc_vpage((void *)(~(PAGE_SIZE - 1)));
 #else
     setup_mmu_range(cr3, 0, (2ul << 30));
     setup_mmu_range(cr3, 3ul << 30, (1ul << 30));
-- 
2.47.2

[kvm-unit-tests PATCH 2/8] x86/cet: Fix flushing shadow stack mapping

[Mathias Krause]

After toggling the writable bit for the shadow stack pte, its related
TLB entry needs to be flushed to avoid accidental modifications through
the stale TLB entry. The code therefore does an invlpg() for that.
However, it passes the physical address to invlpg() while it should have
been the virtual one.

Fix that.

Fixes: 79e53994616f ("x86: Add test cases for user-mode CET validation")
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/cet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/cet.c b/x86/cet.c
index 42d2b1fc043f..2d704ef8e2a2 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -100,7 +100,7 @@ int main(int ac, char **av)
 	*ptep |= PT_DIRTY_MASK;
 
 	/* Flush the paging cache. */
-	invlpg((void *)shstk_phys);
+	invlpg(shstk_virt);
 
 	/* Enable shadow-stack protection */
 	wrmsr(MSR_IA32_U_CET, ENABLE_SHSTK_BIT);
-- 
2.47.2

[kvm-unit-tests PATCH 3/8] x86/cet: Use NONCANONICAL for non-canonical address

[Mathias Krause]

The #CP exception handler finishes by triggering a general protection
fault (#GP) and letting the test framework around run_in_user() handle
the cleanup part. The #GP exception gets triggered by jumping to a
non-canonical address.

Use the NONCANONICAL define we have for these instead of using a one-off
value for it.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/cet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/cet.c b/x86/cet.c
index 2d704ef8e2a2..83371240018a 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -9,7 +9,7 @@
 #include "fault_test.h"
 
 static int cp_count;
-static unsigned long invalid_offset = 0xffffffffffffff;
+static unsigned long invalid_offset = NONCANONICAL;
 
 static u64 cet_shstk_func(void)
 {
-- 
2.47.2

[kvm-unit-tests PATCH 4/8] x86/cet: Make shadow stack less fragile

[Mathias Krause]

The CET shadow stack test has certain assumptions about the code, namely
that it was compiled with frame pointers enabled and the return address
won't be 0xdeaddead.

Make the code less fragile by actually lifting these assumptions to (1)
explicitly mention the dependency to the frame pointer by making us of
__builtin_frame_address(0) and (2) modify the return address by toggling
bits instead of writing a fixed value. Also ensure that write will
actually be generated by the compiler by making it a 'volatile' write.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/cet.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/x86/cet.c b/x86/cet.c
index 83371240018a..8bfa1e057112 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -13,14 +13,14 @@ static unsigned long invalid_offset = NONCANONICAL;
 
 static u64 cet_shstk_func(void)
 {
-	unsigned long *ret_addr, *ssp;
+	unsigned long *ret_addr = __builtin_frame_address(0) + sizeof(void *);
+	unsigned long *ssp;
 
 	/* rdsspq %rax */
 	asm volatile (".byte 0xf3, 0x48, 0x0f, 0x1e, 0xc8" : "=a"(ssp));
 
-	asm("movq %%rbp,%0" : "=r"(ret_addr));
 	printf("The return-address in shadow-stack = 0x%lx, in normal stack = 0x%lx\n",
-	       *ssp, *(ret_addr + 1));
+	       *ssp, *ret_addr);
 
 	/*
 	 * In below line, it modifies the return address, it'll trigger #CP
@@ -29,7 +29,7 @@ static u64 cet_shstk_func(void)
 	 * when HW detects the violation.
 	 */
 	printf("Try to temper the return-address, this causes #CP on returning...\n");
-	*(ret_addr + 1) = 0xdeaddead;
+	*(volatile unsigned long *)ret_addr ^= 0xdeaddead;
 
 	return 0;
 }
-- 
2.47.2

[kvm-unit-tests PATCH 5/8] x86/cet: Avoid unnecessary function pointer casts

[Mathias Krause]

Change the types of cet_shstk_func() and cet_ibt_func() to align with
what run_in_user() expects instead of casting them to a non-matching
type.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/cet.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/x86/cet.c b/x86/cet.c
index 8bfa1e057112..fbfcf7d1ab23 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -11,7 +11,7 @@
 static int cp_count;
 static unsigned long invalid_offset = NONCANONICAL;
 
-static u64 cet_shstk_func(void)
+static uint64_t cet_shstk_func(void)
 {
 	unsigned long *ret_addr = __builtin_frame_address(0) + sizeof(void *);
 	unsigned long *ssp;
@@ -34,7 +34,7 @@ static u64 cet_shstk_func(void)
 	return 0;
 }
 
-static u64 cet_ibt_func(void)
+static uint64_t cet_ibt_func(void)
 {
 	/*
 	 * In below assembly code, the first instruction at label 2 is not
@@ -112,14 +112,14 @@ int main(int ac, char **av)
 	write_cr4(read_cr4() | X86_CR4_CET);
 
 	printf("Unit test for CET user mode...\n");
-	run_in_user((usermode_func)cet_shstk_func, GP_VECTOR, 0, 0, 0, 0, &rvc);
+	run_in_user(cet_shstk_func, GP_VECTOR, 0, 0, 0, 0, &rvc);
 	report(cp_count == 1, "Completed shadow-stack protection test successfully.");
 	cp_count = 0;
 
 	/* Enable indirect-branch tracking */
 	wrmsr(MSR_IA32_U_CET, ENABLE_IBT_BIT);
 
-	run_in_user((usermode_func)cet_ibt_func, GP_VECTOR, 0, 0, 0, 0, &rvc);
+	run_in_user(cet_ibt_func, GP_VECTOR, 0, 0, 0, 0, &rvc);
 	report(cp_count == 1, "Completed Indirect-branch tracking test successfully.");
 
 	write_cr4(read_cr4() & ~X86_CR4_CET);
-- 
2.47.2

[kvm-unit-tests PATCH 6/8] x86/cet: Simplify IBT test

[Mathias Krause]

The inline assembly of cet_ibt_func() does unnecessary things and
doesn't mention the clobbered registers.

Fix that by reducing the code to what's needed (an indirect jump to a
target lacking the ENDBR instruction) and passing and output register
variable for it.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/cet.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/x86/cet.c b/x86/cet.c
index fbfcf7d1ab23..b41443c1e67d 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -36,18 +36,17 @@ static uint64_t cet_shstk_func(void)
 
 static uint64_t cet_ibt_func(void)
 {
+	unsigned long tmp;
 	/*
 	 * In below assembly code, the first instruction at label 2 is not
 	 * endbr64, it'll trigger #CP with error code 0x3, and the execution
 	 * is terminated when HW detects the violation.
 	 */
 	printf("No endbr64 instruction at jmp target, this triggers #CP...\n");
-	asm volatile ("movq $2, %rcx\n"
-		      "dec %rcx\n"
-		      "leaq 2f(%rip), %rax\n"
-		      "jmp *%rax \n"
-		      "2:\n"
-		      "dec %rcx\n");
+	asm volatile ("leaq 2f(%%rip), %0\n\t"
+		      "jmpq *%0\n\t"
+		      "2:"
+		      : "=r"(tmp));
 	return 0;
 }
 
-- 
2.47.2

[kvm-unit-tests PATCH 7/8] x86/cet: Track and verify #CP error code

[Mathias Krause]

Verify that the #CP error code matches what we expect.

Also shorten the reported test summary to omit the test status (it'll be
prepended by report()) and just output what was tested.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/cet.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/x86/cet.c b/x86/cet.c
index b41443c1e67d..c99458af2eab 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -9,6 +9,7 @@
 #include "fault_test.h"
 
 static int cp_count;
+static unsigned long cp_err;
 static unsigned long invalid_offset = NONCANONICAL;
 
 static uint64_t cet_shstk_func(void)
@@ -50,12 +51,20 @@ static uint64_t cet_ibt_func(void)
 	return 0;
 }
 
+#define CP_ERR_NEAR_RET	0x0001
+#define CP_ERR_FAR_RET	0x0002
+#define CP_ERR_ENDBR	0x0003
+#define CP_ERR_RSTORSSP	0x0004
+#define CP_ERR_SETSSBSY	0x0005
+#define CP_ERR_ENCL		BIT(15)
+
 #define ENABLE_SHSTK_BIT 0x1
 #define ENABLE_IBT_BIT   0x4
 
 static void handle_cp(struct ex_regs *regs)
 {
 	cp_count++;
+	cp_err = regs->error_code;
 	printf("In #CP exception handler, error_code = 0x%lx\n",
 		regs->error_code);
 	/* Below jmp is expected to trigger #GP */
@@ -110,16 +119,18 @@ int main(int ac, char **av)
 	/* Enable CET master control bit in CR4. */
 	write_cr4(read_cr4() | X86_CR4_CET);
 
-	printf("Unit test for CET user mode...\n");
+	printf("Unit tests for CET user mode...\n");
 	run_in_user(cet_shstk_func, GP_VECTOR, 0, 0, 0, 0, &rvc);
-	report(cp_count == 1, "Completed shadow-stack protection test successfully.");
+	report(cp_count == 1 && cp_err == CP_ERR_NEAR_RET,
+	       "NEAR RET shadow-stack protection test");
 	cp_count = 0;
 
 	/* Enable indirect-branch tracking */
 	wrmsr(MSR_IA32_U_CET, ENABLE_IBT_BIT);
 
 	run_in_user(cet_ibt_func, GP_VECTOR, 0, 0, 0, 0, &rvc);
-	report(cp_count == 1, "Completed Indirect-branch tracking test successfully.");
+	report(cp_count == 1 && cp_err == CP_ERR_ENDBR,
+	       "Indirect-branch tracking test");
 
 	write_cr4(read_cr4() & ~X86_CR4_CET);
 	wrmsr(MSR_IA32_U_CET, 0);
-- 
2.47.2

[kvm-unit-tests PATCH 8/8] x86/cet: Test far returns too

[Mathias Krause]

Add a test for far returns which has a dedicated error code.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/cet.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/x86/cet.c b/x86/cet.c
index c99458af2eab..0f7046580778 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -35,6 +35,34 @@ static uint64_t cet_shstk_func(void)
 	return 0;
 }
 
+static uint64_t cet_shstk_far_ret(void)
+{
+	struct far_pointer32 fp = {
+		.offset = (uintptr_t)&&far_func,
+		.selector = USER_CS,
+	};
+
+	if (fp.offset != (uintptr_t)&&far_func) {
+		printf("Code address too high.\n");
+		return -1;
+	}
+
+	printf("Try to temper the return-address of far-called function...\n");
+
+	/* The NOP isn't superfluous, the called function tries to skip it. */
+	asm goto ("lcall *%0; nop" : : "m" (fp) : : far_func);
+
+	printf("Uhm... how did we get here?! This should have #CP'ed!\n");
+
+	return 0;
+far_func:
+	asm volatile (/* mess with the ret addr, make it point past the NOP */
+		      "incq (%rsp)\n\t"
+		      /* 32-bit return, just as we have been called */
+		      "lret");
+	__builtin_unreachable();
+}
+
 static uint64_t cet_ibt_func(void)
 {
 	unsigned long tmp;
@@ -125,6 +153,11 @@ int main(int ac, char **av)
 	       "NEAR RET shadow-stack protection test");
 	cp_count = 0;
 
+	run_in_user(cet_shstk_far_ret, GP_VECTOR, 0, 0, 0, 0, &rvc);
+	report(cp_count == 1 && cp_err == CP_ERR_FAR_RET,
+	       "FAR RET shadow-stack protection test");
+	cp_count = 0;
+
 	/* Enable indirect-branch tracking */
 	wrmsr(MSR_IA32_U_CET, ENABLE_IBT_BIT);
 
-- 
2.47.2

Re: [kvm-unit-tests PATCH 0/8] x86: CET fixes and enhancements

[Chao Gao]

On Fri, Jun 20, 2025 at 05:39:04PM +0200, Mathias Krause wrote:
>Hi,
>
>I'm playing with the CET virtualization patch set[1] and was looking at
>the CET tests and noticed a few obvious issues with it (flushing the
>wrong address) as well as some missing parts (testing far rets).
>
>[1] https://lore.kernel.org/kvm/20240219074733.122080-1-weijiang.yang@intel.com/

Hi Mathias,

Thank you.

I posted a series https://lore.kernel.org/kvm/20250513072250.568180-1-chao.gao@intel.com/
to fix issues and add nested test cases. we may consider merging them into one series. e.g.,

>
>Below is a small series with fixes and cleanups.
>
>Please apply!
>
>Thanks,
>Mathias
>
>
>Mathias Krause (8):
>  x86: Avoid top-most page for vmalloc on x86-64
>  x86/cet: Fix flushing shadow stack mapping
>  x86/cet: Use NONCANONICAL for non-canonical address

This will be not needed as my series eliminates the jump to a non-canonical
address.

>  x86/cet: Make shadow stack less fragile
>  x86/cet: Avoid unnecessary function pointer casts
>  x86/cet: Simplify IBT test
>  x86/cet: Track and verify #CP error code

We can use exception_error_code() to retrieve the error code instead of adding
a global variable.

>  x86/cet: Test far returns too
>
> lib/x86/vm.c |  2 ++
> x86/cet.c    | 81 ++++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 64 insertions(+), 19 deletions(-)
>
>-- 
>2.47.2
>

Re: [kvm-unit-tests PATCH 1/8] x86: Avoid top-most page for vmalloc on x86-64

[Chao Gao]

On Fri, Jun 20, 2025 at 05:39:05PM +0200, Mathias Krause wrote:
>The x86-64 implementation of setup_mmu() doesn't initialize 'vfree_top'
>and leaves it at its zero-value. This isn't wrong per se, however, it
>leads to odd configurations when the first vmalloc/vmap page gets
>allocated. It'll be the very last page in the virtual address space --
>which is an interesting corner case -- but its boundary will probably
>wrap. It does so for CET's shadow stack, at least, which loads the
>shadow stack pointer with the base address of the mapped page plus its
>size, i.e. 0xffffffff_fffff000 + 4096, which wraps to 0x0.
>
>The CPU seems to handle such configurations just fine. However, it feels
>odd to set the shadow stack pointer to "NULL".

Not sure if adjusting this is necessary. As a unit test, exercising this corner
case might be beneficial. But I don't have a strong opinion. So,

Reviewed-by: Chao Gao <chao.gao@intel.com>

>
>To avoid the wrapping, ignore the top most page by initializing
>'vfree_top' to just one page below.

Nit: this makes the comment in test_lam_sup() stale, specifically "KUT
initializes vfree_top to 0 for X86_64". So, that comment needs an update.

Re: [kvm-unit-tests PATCH 6/8] x86/cet: Simplify IBT test

[Chao Gao]

On Fri, Jun 20, 2025 at 05:39:10PM +0200, Mathias Krause wrote:
>The inline assembly of cet_ibt_func() does unnecessary things and
>doesn't mention the clobbered registers.
>
>Fix that by reducing the code to what's needed (an indirect jump to a
>target lacking the ENDBR instruction) and passing and output register
>variable for it.
>
>Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>---
> x86/cet.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
>diff --git a/x86/cet.c b/x86/cet.c
>index fbfcf7d1ab23..b41443c1e67d 100644
>--- a/x86/cet.c
>+++ b/x86/cet.c
>@@ -36,18 +36,17 @@ static uint64_t cet_shstk_func(void)
> 
> static uint64_t cet_ibt_func(void)
> {
>+	unsigned long tmp;
> 	/*
> 	 * In below assembly code, the first instruction at label 2 is not
> 	 * endbr64, it'll trigger #CP with error code 0x3, and the execution
> 	 * is terminated when HW detects the violation.
> 	 */
> 	printf("No endbr64 instruction at jmp target, this triggers #CP...\n");
>-	asm volatile ("movq $2, %rcx\n"
>-		      "dec %rcx\n"
>-		      "leaq 2f(%rip), %rax\n"
>-		      "jmp *%rax \n"
>-		      "2:\n"
>-		      "dec %rcx\n");
>+	asm volatile ("leaq 2f(%%rip), %0\n\t"
>+		      "jmpq *%0\n\t"
>+		      "2:"
>+		      : "=r"(tmp));

@tmp isn't needed. We can still use "rax" and list it as clobbered. 

> 	return 0;
> }
> 
>-- 
>2.47.2
>

Re: [kvm-unit-tests PATCH 8/8] x86/cet: Test far returns too

[Chao Gao]

On Fri, Jun 20, 2025 at 05:39:12PM +0200, Mathias Krause wrote:
>Add a test for far returns which has a dedicated error code.
>
>Signed-off-by: Mathias Krause <minipli@grsecurity.net>

Tested-by: Chao Gao <chao.gao@intel.com>

Re: [kvm-unit-tests PATCH 0/8] x86: CET fixes and enhancements

[Mathias Krause]

On 23.06.25 04:36, Chao Gao wrote:
> On Fri, Jun 20, 2025 at 05:39:04PM +0200, Mathias Krause wrote:
>> Hi,
>>
>> I'm playing with the CET virtualization patch set[1] and was looking at
>> the CET tests and noticed a few obvious issues with it (flushing the
>> wrong address) as well as some missing parts (testing far rets).
>>
>> [1] https://lore.kernel.org/kvm/20240219074733.122080-1-weijiang.yang@intel.com/
> 
> Hi Mathias,
> 
> Thank you.
> 
> I posted a series https://lore.kernel.org/kvm/20250513072250.568180-1-chao.gao@intel.com/
> to fix issues and add nested test cases. we may consider merging them into one series. e.g.,

Oh, I completely missed that one! I only looked at the KUT git tree.
Looks like Paolo / Sean haven't seen it yet either :/

But sure, happy to merge the patches! I'll apply your series locally and
will try to put still relevant changes on top. I'll probably send out an
updated series later this week.

Thanks,
Mathias

Re: [kvm-unit-tests PATCH 0/8] x86: CET fixes and enhancements

[Sean Christopherson]

On Mon, Jun 23, 2025, Mathias Krause wrote:
> On 23.06.25 04:36, Chao Gao wrote:
> > On Fri, Jun 20, 2025 at 05:39:04PM +0200, Mathias Krause wrote:
> >> Hi,
> >>
> >> I'm playing with the CET virtualization patch set[1] and was looking at
> >> the CET tests and noticed a few obvious issues with it (flushing the
> >> wrong address) as well as some missing parts (testing far rets).
> >>
> >> [1] https://lore.kernel.org/kvm/20240219074733.122080-1-weijiang.yang@intel.com/
> > 
> > Hi Mathias,
> > 
> > Thank you.
> > 
> > I posted a series https://lore.kernel.org/kvm/20250513072250.568180-1-chao.gao@intel.com/
> > to fix issues and add nested test cases. we may consider merging them into one series. e.g.,
> 
> Oh, I completely missed that one! I only looked at the KUT git tree.
> Looks like Paolo / Sean haven't seen it yet either :/

FWIW, I've seen it, just haven't had time to actually look at it.

> But sure, happy to merge the patches! I'll apply your series locally and
> will try to put still relevant changes on top. I'll probably send out an
> updated series later this week.