[kvm-unit-tests PATCH v2 00/16] x86: cleanups, fixes and new tests

[kvm-unit-tests PATCH v2 00/16] x86: cleanups, fixes and new tests

[Mathias Krause]

v1: https://lore.kernel.org/kvm/b6322bd0-3639-fb2a-7211-974386865bac@grsecurity.net/

This is v2 of the "non-canonical memory access" test. It evolved into a
small series, bringing cleanups and fixes along the way.

I integrated Sean's feedback and changed the test to make use of
ASM_TRY() instead of using the hand-rolled exception handler. I also
switched all other users in emulator64.c to ASM_TRY() and was able to
drop the one-off exception handler all together.

Sean, this should be a solid ground to refine it further when [1] lands?

[1] https://lkml.kernel.org/r/20230406025117.738014-1-seanjc@google.com

As for the fixes, run_in_user() didn't restore the exception handler it
overwrites, which leads to interesting bugs when the handler fires again
for an unrelated exception -- that longjmp() won't do the right thing in
this case ;)

I fixed fault_test() as well, as it has the same behaviour.

For new tests, I added the non-canonical memory access exception test of
v1 and added another SS segment register load test to check non-NULL
selectors as well, as I stumbled over the bugs in run_in_user() while
switching test_sreg() over to TRY_ASM().

Be aware that the types.h removal (first patch) has an unfortunate side
effect. It breaks compilation in already build trees, as the dependency
files (.*.d) don't get regenerated / cleaned if a source file changes.
This leads to stale references to types.h which can only be solved by a
'make clean'. :(

We really should change the dependency file generation to avoid that
problem, as the current state is kinda awkward. Tho, I didn't had the
time to look into it further myself.

Please apply!

Thanks,
Mathias

PS: I'm on holidays for three weeks from Saturday on, so won't respond
to feedback any time soon.

Mathias Krause (16):
  x86: Drop types.h
  x86: Use symbolic names in exception_mnemonic()
  x86: Add vendor specific exception vectors
  x86/cet: Use symbolic name for #CP
  x86/access: Use 'bool' type as defined via libcflat.h
  x86/run_in_user: Change type of code label
  x86/run_in_user: Preserve exception handler
  x86/run_in_user: Relax register constraints of inline asm
  x86/run_in_user: Reload SS after successful return
  x86/fault_test: Preserve exception handler
  x86/emulator64: Relax register constraints for usr_gs_mov()
  x86/emulator64: Switch test_sreg() to ASM_TRY()
  x86/emulator64: Add non-null selector test
  x86/emulator64: Switch test_jmp_noncanonical() to ASM_TRY()
  x86/emulator64: Switch test_mmx_movq_mf() to ASM_TRY()
  x86/emulator64: Test non-canonical memory access exceptions

 lib/x86/processor.h  |  13 ++++++
 lib/x86/desc.c       |  43 ++++++++++--------
 lib/x86/fault_test.c |   4 +-
 lib/x86/usermode.c   |  42 ++++++++++-------
 x86/types.h          |  21 ---------
 x86/access.c         |  11 ++---
 x86/cet.c            |   2 +-
 x86/cmpxchg8b.c      |   1 -
 x86/emulator.c       |   1 -
 x86/emulator64.c     | 105 ++++++++++++++++++++++++-------------------
 x86/pmu_pebs.c       |   1 -
 x86/svm.c            |   1 -
 x86/svm_tests.c      |   1 -
 x86/vmx_tests.c      |   1 -
 14 files changed, 129 insertions(+), 118 deletions(-)
 delete mode 100644 x86/types.h

-- 
2.39.2

[kvm-unit-tests PATCH v2 01/16] x86: Drop types.h

[Mathias Krause]

The file types.h never declared any types, only exception vectors which
are partially re-defined in processor.h.

Move the remaining vector definitions to processor.h and remove types.h,
as all users already include processor.h

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 lib/x86/processor.h |  7 +++++++
 x86/types.h         | 21 ---------------------
 x86/cmpxchg8b.c     |  1 -
 x86/emulator.c      |  1 -
 x86/pmu_pebs.c      |  1 -
 x86/svm.c           |  1 -
 x86/svm_tests.c     |  1 -
 x86/vmx_tests.c     |  1 -
 8 files changed, 7 insertions(+), 27 deletions(-)
 delete mode 100644 x86/types.h

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 6555056e8a13..91a9022ef43c 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -19,16 +19,23 @@
 #  define S "4"
 #endif
 
+#define DE_VECTOR 0
 #define DB_VECTOR 1
+#define NMI_VECTOR 2
 #define BP_VECTOR 3
+#define OF_VECTOR 4
+#define BR_VECTOR 5
 #define UD_VECTOR 6
+#define NM_VECTOR 7
 #define DF_VECTOR 8
 #define TS_VECTOR 10
 #define NP_VECTOR 11
 #define SS_VECTOR 12
 #define GP_VECTOR 13
 #define PF_VECTOR 14
+#define MF_VECTOR 16
 #define AC_VECTOR 17
+#define MC_VECTOR 18
 #define CP_VECTOR 21
 
 #define X86_CR0_PE_BIT		(0)
diff --git a/x86/types.h b/x86/types.h
deleted file mode 100644
index 56ce5ececdec..000000000000
--- a/x86/types.h
+++ /dev/null
@@ -1,21 +0,0 @@
-#ifndef X86_TYPES_H
-#define X86_TYPES_H
-
-#define DE_VECTOR 0
-#define DB_VECTOR 1
-#define NMI_VECTOR 2
-#define BP_VECTOR 3
-#define OF_VECTOR 4
-#define BR_VECTOR 5
-#define UD_VECTOR 6
-#define NM_VECTOR 7
-#define DF_VECTOR 8
-#define TS_VECTOR 10
-#define NP_VECTOR 11
-#define SS_VECTOR 12
-#define GP_VECTOR 13
-#define PF_VECTOR 14
-#define MF_VECTOR 16
-#define MC_VECTOR 18
-
-#endif
diff --git a/x86/cmpxchg8b.c b/x86/cmpxchg8b.c
index a416f44f2067..8afe629ea257 100644
--- a/x86/cmpxchg8b.c
+++ b/x86/cmpxchg8b.c
@@ -2,7 +2,6 @@
 #include "vm.h"
 #include "libcflat.h"
 #include "desc.h"
-#include "types.h"
 #include "processor.h"
 
 static void test_cmpxchg8b(u32 *mem)
diff --git a/x86/emulator.c b/x86/emulator.c
index ad9437403fef..f8bdc26b70ad 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -4,7 +4,6 @@
 #include "vm.h"
 #include "libcflat.h"
 #include "desc.h"
-#include "types.h"
 #include "processor.h"
 #include "vmalloc.h"
 #include "alloc_page.h"
diff --git a/x86/pmu_pebs.c b/x86/pmu_pebs.c
index 894ae6c784f9..d1a68ca336ab 100644
--- a/x86/pmu_pebs.c
+++ b/x86/pmu_pebs.c
@@ -8,7 +8,6 @@
 #include "alloc.h"
 
 #include "vm.h"
-#include "types.h"
 #include "processor.h"
 #include "vmalloc.h"
 #include "alloc_page.h"
diff --git a/x86/svm.c b/x86/svm.c
index ba435b4ac3af..63a84720709d 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -10,7 +10,6 @@
 #include "vm.h"
 #include "fwcfg.h"
 #include "smp.h"
-#include "types.h"
 #include "alloc_page.h"
 #include "isr.h"
 #include "apic.h"
diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 27ce47b4f98e..691ac937288a 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -5,7 +5,6 @@
 #include "msr.h"
 #include "vm.h"
 #include "smp.h"
-#include "types.h"
 #include "alloc_page.h"
 #include "isr.h"
 #include "apic.h"
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 7952ccb932ea..96104ab018d8 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -16,7 +16,6 @@
 #include "isr.h"
 #include "desc.h"
 #include "apic.h"
-#include "types.h"
 #include "vmalloc.h"
 #include "alloc_page.h"
 #include "smp.h"
-- 
2.39.2

[kvm-unit-tests PATCH v2 02/16] x86: Use symbolic names in exception_mnemonic()

[Mathias Krause]

Use existing symbolic definitions for vector numbers instead of plain
numbers and streamline the stringification further by using a macro.

While at it, add the missing case for #CP.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 lib/x86/processor.h |  1 +
 lib/x86/desc.c      | 39 +++++++++++++++++++++------------------
 2 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 91a9022ef43c..5dd7bce024fd 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -36,6 +36,7 @@
 #define MF_VECTOR 16
 #define AC_VECTOR 17
 #define MC_VECTOR 18
+#define XM_VECTOR 19
 #define CP_VECTOR 21
 
 #define X86_CR0_PE_BIT		(0)
diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index b293ae420f86..9402c0ef59d0 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -110,25 +110,28 @@ extern struct ex_record exception_table_start, exception_table_end;
 const char* exception_mnemonic(int vector)
 {
 	switch(vector) {
-	case 0: return "#DE";
-	case 1: return "#DB";
-	case 2: return "#NMI";
-	case 3: return "#BP";
-	case 4: return "#OF";
-	case 5: return "#BR";
-	case 6: return "#UD";
-	case 7: return "#NM";
-	case 8: return "#DF";
-	case 10: return "#TS";
-	case 11: return "#NP";
-	case 12: return "#SS";
-	case 13: return "#GP";
-	case 14: return "#PF";
-	case 16: return "#MF";
-	case 17: return "#AC";
-	case 18: return "#MC";
-	case 19: return "#XM";
+#define VEC(v) case v##_VECTOR: return "#" #v
+	VEC(DE);
+	VEC(DB);
+	VEC(NMI);
+	VEC(BP);
+	VEC(OF);
+	VEC(BR);
+	VEC(UD);
+	VEC(NM);
+	VEC(DF);
+	VEC(TS);
+	VEC(NP);
+	VEC(SS);
+	VEC(GP);
+	VEC(PF);
+	VEC(MF);
+	VEC(AC);
+	VEC(MC);
+	VEC(XM);
+	VEC(CP);
 	default: return "#??";
+#undef VEC
 	}
 }
 
-- 
2.39.2

[kvm-unit-tests PATCH v2 03/16] x86: Add vendor specific exception vectors

[Mathias Krause]

Intel and AMD have some vendor specific exception vectors, namely:
- Intel only: #VE (20),
- AMD only: #HV (28), #VC (29) and #SX (30).

Also Intel's #XM (19) is called #XF for AMD.

Add definitions for all of these and add comments stating they're vendor
specific.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 lib/x86/processor.h | 5 +++++
 lib/x86/desc.c      | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 5dd7bce024fd..7590c0c44c79 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -37,7 +37,12 @@
 #define AC_VECTOR 17
 #define MC_VECTOR 18
 #define XM_VECTOR 19
+#define XF_VECTOR XM_VECTOR /* AMD */
+#define VE_VECTOR 20 /* Intel only */
 #define CP_VECTOR 21
+#define HV_VECTOR 28 /* AMD only */
+#define VC_VECTOR 29 /* AMD only */
+#define SX_VECTOR 30 /* AMD only */
 
 #define X86_CR0_PE_BIT		(0)
 #define X86_CR0_PE		BIT(X86_CR0_PE_BIT)
diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 9402c0ef59d0..06bb3e3c1e5d 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -129,7 +129,11 @@ const char* exception_mnemonic(int vector)
 	VEC(AC);
 	VEC(MC);
 	VEC(XM);
+	VEC(VE);
 	VEC(CP);
+	VEC(HV);
+	VEC(VC);
+	VEC(SX);
 	default: return "#??";
 #undef VEC
 	}
-- 
2.39.2

[kvm-unit-tests PATCH v2 04/16] x86/cet: Use symbolic name for #CP

[Mathias Krause]

Make use of the symbolic name for the #CP vector number instead of
hard-coding its value.

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 c01dd89d9082..42d2b1fc043f 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -83,7 +83,7 @@ int main(int ac, char **av)
 	}
 
 	setup_vm();
-	handle_exception(21, handle_cp);
+	handle_exception(CP_VECTOR, handle_cp);
 
 	/* Allocate one page for shadow-stack. */
 	shstk_virt = alloc_vpage();
-- 
2.39.2

[kvm-unit-tests PATCH v2 05/16] x86/access: Use 'bool' type as defined via libcflat.h

[Mathias Krause]

Drop the unneeded definitions of 'true' and 'false' and make use of the
common 'bool' type instead of using the pre-C99 / post-C23 definitions.

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

diff --git a/x86/access.c b/x86/access.c
index 70d81bf02d9d..f90a72d6e951 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -5,10 +5,7 @@
 #include "x86/vm.h"
 #include "access.h"
 
-#define true 1
-#define false 0
-
-static _Bool verbose = false;
+static bool verbose = false;
 
 typedef unsigned long pt_element_t;
 static int invalid_mask;
@@ -406,7 +403,7 @@ static int ac_test_bump_one(ac_test_t *at)
 
 #define F(x)  ((flags & x##_MASK) != 0)
 
-static _Bool ac_test_legal(ac_test_t *at)
+static bool ac_test_legal(ac_test_t *at)
 {
 	int flags = at->flags;
 	unsigned reserved;
@@ -738,7 +735,7 @@ static void dump_mapping(ac_test_t *at)
 	walk_va(at, F(AC_PDE_PSE) ? 2 : 1, virt, __dump_pte, false);
 }
 
-static void ac_test_check(ac_test_t *at, _Bool *success_ret, _Bool cond,
+static void ac_test_check(ac_test_t *at, bool *success_ret, bool cond,
 			  const char *fmt, ...)
 {
 	va_list ap;
@@ -780,7 +777,7 @@ static int ac_test_do_access(ac_test_t *at)
 	unsigned e;
 	static unsigned char user_stack[4096];
 	unsigned long rsp;
-	_Bool success = true;
+	bool success = true;
 	int flags = at->flags;
 
 	++unique;
-- 
2.39.2

[kvm-unit-tests PATCH v2 06/16] x86/run_in_user: Change type of code label

[Mathias Krause]

Use an array type to refer to the code label 'ret_to_kernel'.

No functional change.

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

diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index e22fb8f0132b..b976123ca753 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -35,12 +35,12 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 		uint64_t arg1, uint64_t arg2, uint64_t arg3,
 		uint64_t arg4, bool *raised_vector)
 {
-	extern char ret_to_kernel;
+	extern char ret_to_kernel[];
 	uint64_t rax = 0;
 	static unsigned char user_stack[USERMODE_STACK_SIZE];
 
 	*raised_vector = 0;
-	set_idt_entry(RET_TO_KERNEL_IRQ, &ret_to_kernel, 3);
+	set_idt_entry(RET_TO_KERNEL_IRQ, ret_to_kernel, 3);
 	handle_exception(fault_vector,
 			restore_exec_to_jmpbuf_exception_handler);
 
-- 
2.39.2

[kvm-unit-tests PATCH v2 07/16] x86/run_in_user: Preserve exception handler

[Mathias Krause]

run_in_user() replaces the exception handler for the expected fault
vector to ensure the code can properly return back to kernel mode in
case of such exceptions. However, it leaves the exception handler in
place which may confuse later test code triggering the same exception
without installing a handler first.

Fix this be restoring the previous exception handler. Running the
longjmp() handler out of context will lead to no good.

We now also need to make 'rax' volatile to avoid a related compiler
warning.

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

diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index b976123ca753..10fcea288a62 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -36,15 +36,17 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 		uint64_t arg4, bool *raised_vector)
 {
 	extern char ret_to_kernel[];
-	uint64_t rax = 0;
+	volatile uint64_t rax = 0;
 	static unsigned char user_stack[USERMODE_STACK_SIZE];
+	handler old_ex;
 
 	*raised_vector = 0;
 	set_idt_entry(RET_TO_KERNEL_IRQ, ret_to_kernel, 3);
-	handle_exception(fault_vector,
-			restore_exec_to_jmpbuf_exception_handler);
+	old_ex = handle_exception(fault_vector,
+				  restore_exec_to_jmpbuf_exception_handler);
 
 	if (setjmp(jmpbuf) != 0) {
+		handle_exception(fault_vector, old_ex);
 		*raised_vector = 1;
 		return 0;
 	}
@@ -114,5 +116,7 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 			:
 			"rsi", "rdi", "rcx", "rdx");
 
+	handle_exception(fault_vector, old_ex);
+
 	return rax;
 }
-- 
2.39.2

[kvm-unit-tests PATCH v2 08/16] x86/run_in_user: Relax register constraints of inline asm

[Mathias Krause]

The code doesn't clobber all the registers it states it would. It
explicitly preserves the values of rcx, rdi and rsi. With a minor code
change rdx can be preserved as well. The code does, however, needlessly
save and restore rbx around the function call.

Change the code to not clobber rdx and drop all the register clobbers
from the asm constraints, as these registers are, in fact, preserved.
The function call either returns without throwing an exception (and
restoring all call clobbered registers itself) or via longjmp() (doing
the same, basically, but with special handling in the compiler as well).

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

diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index 10fcea288a62..fd19551a7a2d 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -63,21 +63,20 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 			"pushq %[user_stack_top]\n\t"
 			"pushfq\n\t"
 			"pushq %[user_cs]\n\t"
-			"lea user_mode(%%rip), %%rdx\n\t"
-			"pushq %%rdx\n\t"
+			"lea user_mode(%%rip), %%rax\n\t"
+			"pushq %%rax\n\t"
 			"iretq\n"
 
 			"user_mode:\n\t"
-			/* Back up registers before invoking func */
-			"push %%rbx\n\t"
+			/* Back up volatile registers before invoking func */
 			"push %%rcx\n\t"
 			"push %%rdx\n\t"
+			"push %%rdi\n\t"
+			"push %%rsi\n\t"
 			"push %%r8\n\t"
 			"push %%r9\n\t"
 			"push %%r10\n\t"
 			"push %%r11\n\t"
-			"push %%rdi\n\t"
-			"push %%rsi\n\t"
 			/* Call user mode function */
 			"mov %[arg1], %%rdi\n\t"
 			"mov %[arg2], %%rsi\n\t"
@@ -85,15 +84,14 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 			"mov %[arg4], %%rcx\n\t"
 			"call *%[func]\n\t"
 			/* Restore registers */
-			"pop %%rsi\n\t"
-			"pop %%rdi\n\t"
 			"pop %%r11\n\t"
 			"pop %%r10\n\t"
 			"pop %%r9\n\t"
 			"pop %%r8\n\t"
+			"pop %%rsi\n\t"
+			"pop %%rdi\n\t"
 			"pop %%rdx\n\t"
 			"pop %%rcx\n\t"
-			"pop %%rbx\n\t"
 			/* Return to kernel via system call */
 			"int %[kernel_entry_vector]\n\t"
 			/* Kernel Mode */
@@ -112,9 +110,7 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 			[user_cs]"i"(USER_CS),
 			[user_stack_top]"r"(user_stack +
 					sizeof(user_stack)),
-			[kernel_entry_vector]"i"(RET_TO_KERNEL_IRQ)
-			:
-			"rsi", "rdi", "rcx", "rdx");
+			[kernel_entry_vector]"i"(RET_TO_KERNEL_IRQ));
 
 	handle_exception(fault_vector, old_ex);
 
-- 
2.39.2

[kvm-unit-tests PATCH v2 09/16] x86/run_in_user: Reload SS after successful return

[Mathias Krause]

Complement commit 663f9e447b98 ("x86: Fix a #GP from occurring in
usermode library's exception handlers") and restore SS on a regular
return as well.

The INT-based "syscall" will make it get loaded with the NULL selector
(see SDM Vol. 1, Interrupt and Exception Behavior in 64-Bit Mode: "The
new SS is set to NULL if there is a change in CPL.") which makes the
"mov null, %%ss" test of emulator64.c dubious, as SS is already loaded
with the NULL selector.

Fix this by loading SS with KERNEL_DS after a successful userland
function call as well, as we already do in case of exceptions.

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

diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index fd19551a7a2d..9ae4cb17fd63 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -97,6 +97,13 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 			/* Kernel Mode */
 			"ret_to_kernel:\n\t"
 			"mov %[rsp0], %%rsp\n\t"
+#ifdef __x86_64__
+			/* Restore SS, as it forcibly gets loaded with NULL */
+			"push %%rax\n\t"
+			"mov %[kernel_ds], %%ax\n\t"
+			"mov %%ax, %%ss\n\t"
+			"pop %%rax\n\t"
+#endif
 			:
 			"+a"(rax),
 			[rsp0]"=m"(tss[0].rsp0)
@@ -108,6 +115,7 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 			[func]"m"(func),
 			[user_ds]"i"(USER_DS),
 			[user_cs]"i"(USER_CS),
+			[kernel_ds]"i"(KERNEL_DS),
 			[user_stack_top]"r"(user_stack +
 					sizeof(user_stack)),
 			[kernel_entry_vector]"i"(RET_TO_KERNEL_IRQ));
-- 
2.39.2

[kvm-unit-tests PATCH v2 10/16] x86/fault_test: Preserve exception handler

[Mathias Krause]

fault_test() replaces the exception handler for in-kernel tests with a
longjmp() based exception handling. However, it leaves the exception
handler in place which may confuse later test code triggering the same
exception without installing a handler first.

Fix this be restoring the previous exception handler, as running the
longjmp() handler out of context will lead to no good.

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

diff --git a/lib/x86/fault_test.c b/lib/x86/fault_test.c
index e15a21864562..614bdcb42535 100644
--- a/lib/x86/fault_test.c
+++ b/lib/x86/fault_test.c
@@ -19,18 +19,20 @@ static bool fault_test(struct fault_test_arg *arg)
 	test_fault_func func = (test_fault_func) arg->func;
 	/* Init as success in case there isn't callback */
 	bool callback_success = true;
+	handler old;
 
 	if (arg->usermode) {
 		val = run_in_user((usermode_func) func, arg->fault_vector,
 				arg->arg[0], arg->arg[1], arg->arg[2],
 				arg->arg[3], &raised_vector);
 	} else {
-		handle_exception(arg->fault_vector, fault_test_fault);
+		old = handle_exception(arg->fault_vector, fault_test_fault);
 		if (setjmp(jmpbuf) == 0)
 			val = func(arg->arg[0], arg->arg[1], arg->arg[2],
 					arg->arg[3]);
 		else
 			raised_vector = true;
+		handle_exception(arg->fault_vector, old);
 	}
 
 	if (!raised_vector) {
-- 
2.39.2

[kvm-unit-tests PATCH v2 11/16] x86/emulator64: Relax register constraints for usr_gs_mov()

[Mathias Krause]

There's no need to hard-code the registers, allow the compiler to choose
ones that fit.

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

diff --git a/x86/emulator64.c b/x86/emulator64.c
index c58441ca798c..4138eaae00c9 100644
--- a/x86/emulator64.c
+++ b/x86/emulator64.c
@@ -421,7 +421,7 @@ static uint64_t usr_gs_mov(void)
 	uint64_t ret;
 
 	dummy_ptr -= GS_BASE;
-	asm volatile("mov %%gs:(%%rcx), %%rax" : "=a"(ret): "c"(dummy_ptr) :);
+	asm volatile("mov %%gs:(%1), %0" : "=r"(ret) : "r"(dummy_ptr));
 
 	return ret;
 }
-- 
2.39.2

[kvm-unit-tests PATCH v2 12/16] x86/emulator64: Switch test_sreg() to ASM_TRY()

[Mathias Krause]

Instead of registering a one-off exception handler, make use of
ASM_TRY() to catch the exception. Also test the error code to match the
failing segment selector (NULL) as the code now easily can access it.

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

diff --git a/x86/emulator64.c b/x86/emulator64.c
index 4138eaae00c9..a98c66c2b44f 100644
--- a/x86/emulator64.c
+++ b/x86/emulator64.c
@@ -385,18 +385,9 @@ static void test_push16(uint64_t *mem)
 	report(rsp1 == rsp2, "push16");
 }
 
-static void ss_bad_rpl(struct ex_regs *regs)
-{
-	extern char ss_bad_rpl_cont;
-
-	++exceptions;
-	regs->rip = (ulong)&ss_bad_rpl_cont;
-}
-
 static void test_sreg(volatile uint16_t *mem)
 {
 	u16 ss = read_ss();
-	handler old;
 
 	// check for null segment load
 	*mem = 0;
@@ -404,13 +395,12 @@ static void test_sreg(volatile uint16_t *mem)
 	report(read_ss() == 0, "mov null, %%ss");
 
 	// check for exception when ss.rpl != cpl on null segment load
-	exceptions = 0;
-	old = handle_exception(GP_VECTOR, ss_bad_rpl);
 	*mem = 3;
-	asm volatile("mov %0, %%ss; ss_bad_rpl_cont:" : : "m"(*mem));
-	report(exceptions == 1 && read_ss() == 0,
+	asm volatile(ASM_TRY("1f") "mov %0, %%ss; 1:" : : "m"(*mem));
+	report(exception_vector() == GP_VECTOR &&
+	       exception_error_code() == 0 && read_ss() == 0,
 	       "mov null, %%ss (with ss.rpl != cpl)");
-	handle_exception(GP_VECTOR, old);
+
 	write_ss(ss);
 }
 
-- 
2.39.2

[kvm-unit-tests PATCH v2 13/16] x86/emulator64: Add non-null selector test

[Mathias Krause]

Complement the NULL selector based RPL!=CPL test with a non-NULL one to
ensure the failing segment selector is correctly reported through the
exception error code.

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

diff --git a/x86/emulator64.c b/x86/emulator64.c
index a98c66c2b44f..492e8a292839 100644
--- a/x86/emulator64.c
+++ b/x86/emulator64.c
@@ -401,6 +401,13 @@ static void test_sreg(volatile uint16_t *mem)
 	       exception_error_code() == 0 && read_ss() == 0,
 	       "mov null, %%ss (with ss.rpl != cpl)");
 
+	// check for exception when ss.rpl != cpl on non-null segment load
+	*mem = KERNEL_DS | 3;
+	asm volatile(ASM_TRY("1f") "mov %0, %%ss; 1:" : : "m"(*mem));
+	report(exception_vector() == GP_VECTOR &&
+	       exception_error_code() == KERNEL_DS && read_ss() == 0,
+	       "mov non-null, %%ss (with ss.rpl != cpl)");
+
 	write_ss(ss);
 }
 
-- 
2.39.2

[kvm-unit-tests PATCH v2 14/16] x86/emulator64: Switch test_jmp_noncanonical() to ASM_TRY()

[Mathias Krause]

Instead of registering a one-off exception handler, make use of
ASM_TRY() to catch the exception. Also make use of the 'NONCANONICAL'
define to refer to a non-canonical address.

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

diff --git a/x86/emulator64.c b/x86/emulator64.c
index 492e8a292839..50a02bca6ac8 100644
--- a/x86/emulator64.c
+++ b/x86/emulator64.c
@@ -334,17 +334,10 @@ static void test_mmx_movq_mf(uint64_t *mem)
 
 static void test_jmp_noncanonical(uint64_t *mem)
 {
-	extern char nc_jmp_start, nc_jmp_end;
-	handler old;
-
-	*mem = 0x1111111111111111ul;
-
-	exceptions = 0;
-	rip_advance = &nc_jmp_end - &nc_jmp_start;
-	old = handle_exception(GP_VECTOR, advance_rip_and_note_exception);
-	asm volatile ("nc_jmp_start: jmp *%0; nc_jmp_end:" : : "m"(*mem));
-	report(exceptions == 1, "jump to non-canonical address");
-	handle_exception(GP_VECTOR, old);
+	*mem = NONCANONICAL;
+	asm volatile (ASM_TRY("1f") "jmp *%0; 1:" : : "m"(*mem));
+	report(exception_vector() == GP_VECTOR,
+	       "jump to non-canonical address");
 }
 
 static void test_movabs(uint64_t *mem)
-- 
2.39.2

[kvm-unit-tests PATCH v2 15/16] x86/emulator64: Switch test_mmx_movq_mf() to ASM_TRY()

[Mathias Krause]

Drop the last user of the one-off exception handler by making use of
ASM_TRY() for the #MF test.

Also streamline the multiple scattered asm() statements into a single
one making use of a real output value instead of hard-coding rax and
relying on the instruction to generate an exception (instead of
clobbering rax and not making gcc aware of it).

As this removes the last user of advance_rip_and_note_exception() we can
remove it for good!

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 x86/emulator64.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/x86/emulator64.c b/x86/emulator64.c
index 50a02bca6ac8..f8ff99fc39cc 100644
--- a/x86/emulator64.c
+++ b/x86/emulator64.c
@@ -1,14 +1,6 @@
 #define MAGIC_NUM 0xdeadbeefdeadbeefUL
 #define GS_BASE 0x400000
 
-static unsigned long rip_advance;
-
-static void advance_rip_and_note_exception(struct ex_regs *regs)
-{
-	++exceptions;
-	regs->rip += rip_advance;
-}
-
 static void test_cr8(void)
 {
 	unsigned long src, dst;
@@ -313,23 +305,24 @@ static void test_cmov(u32 *mem)
 
 static void test_mmx_movq_mf(uint64_t *mem)
 {
-	/* movq %mm0, (%rax) */
-	extern char movq_start, movq_end;
-	handler old;
-
 	uint16_t fcw = 0;  /* all exceptions unmasked */
-	write_cr0(read_cr0() & ~6);  /* TS, EM */
-	exceptions = 0;
-	old = handle_exception(MF_VECTOR, advance_rip_and_note_exception);
-	asm volatile("fninit; fldcw %0" : : "m"(fcw));
-	asm volatile("fldz; fldz; fdivp"); /* generate exception */
+	uint64_t val;
 
-	rip_advance = &movq_end - &movq_start;
-	asm(KVM_FEP "movq_start: movq %mm0, (%rax); movq_end:");
-	/* exit MMX mode */
-	asm volatile("fnclex; emms");
-	report(exceptions == 1, "movq mmx generates #MF");
-	handle_exception(MF_VECTOR, old);
+	write_cr0(read_cr0() & ~(X86_CR0_TS | X86_CR0_EM));
+	asm volatile("fninit\n\t"
+		     "fldcw %[fcw]\n\t"
+		     "fldz\n\t"
+		     "fldz\n\t"
+		     /* generate exception (0.0 / 0.0) */
+		     "fdivp\n\t"
+		     /* trigger #MF */
+		     ASM_TRY_FEP("1f") "movq %%mm0, %[val]\n\t"
+		     /* exit MMX mode */
+		     "1: fnclex\n\t"
+		     "emms\n\t"
+		     : [val]"=m"(val)
+		     : [fcw]"m"(fcw));
+	report(exception_vector() == MF_VECTOR, "movq mmx generates #MF");
 }
 
 static void test_jmp_noncanonical(uint64_t *mem)
-- 
2.39.2

[kvm-unit-tests PATCH v2 16/16] x86/emulator64: Test non-canonical memory access exceptions

[Mathias Krause]

A stack based memory access should generate a #SS(0) exception but
QEMU/TCG as of now (7.2) makes all exceptions based on a non-canonical
address generate a #GP(0) instead (issue linked below).

Add a test that will succeed when run under KVM but fail when using TCG.

Link: https://gitlab.com/qemu-project/qemu/-/issues/928
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
v2: use ASM_TRY() as suggested by Sean

The non-canonical jump test is, apparently, broken under TCG as well.
It "succeeds," as in changing RIP and thereby creating a #GP loop.
I therefore put the new test in front of it to allow it to run.

 x86/emulator64.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/x86/emulator64.c b/x86/emulator64.c
index f8ff99fc39cc..e1a0968f5236 100644
--- a/x86/emulator64.c
+++ b/x86/emulator64.c
@@ -333,6 +333,33 @@ static void test_jmp_noncanonical(uint64_t *mem)
 	       "jump to non-canonical address");
 }
 
+static void test_reg_noncanonical(void)
+{
+	/* RAX based, should #GP(0) */
+	asm volatile(ASM_TRY("1f") "orq $0, (%[noncanonical]); 1:"
+		     : : [noncanonical]"a"(NONCANONICAL));
+	report(exception_vector() == GP_VECTOR && exception_error_code() == 0,
+	       "non-canonical memory access, should %s(0), got %s(%u)",
+	       exception_mnemonic(GP_VECTOR),
+	       exception_mnemonic(exception_vector()), exception_error_code());
+
+	/* RSP based, should #SS(0) */
+	asm volatile(ASM_TRY("1f") "orq $0, (%%rsp,%[noncanonical],1); 1:"
+		     : : [noncanonical]"r"(NONCANONICAL));
+	report(exception_vector() == SS_VECTOR && exception_error_code() == 0,
+	       "non-canonical rsp-based access, should %s(0), got %s(%u)",
+	       exception_mnemonic(SS_VECTOR),
+	       exception_mnemonic(exception_vector()), exception_error_code());
+
+	/* RBP based, should #SS(0) */
+	asm volatile(ASM_TRY("1f") "orq $0, (%%rbp,%[noncanonical],1); 1:"
+		     : : [noncanonical]"r"(NONCANONICAL));
+	report(exception_vector() == SS_VECTOR && exception_error_code() == 0,
+	       "non-canonical rbp-based access, should %s(0), got %s(%u)",
+	       exception_mnemonic(SS_VECTOR),
+	       exception_mnemonic(exception_vector()), exception_error_code());
+}
+
 static void test_movabs(uint64_t *mem)
 {
 	/* mov $0x9090909090909090, %rcx */
@@ -459,5 +486,6 @@ static void test_emulator_64(void *mem)
 
 	test_push16(mem);
 
+	test_reg_noncanonical();
 	test_jmp_noncanonical(mem);
 }
-- 
2.39.2

Re: [kvm-unit-tests PATCH v2 06/16] x86/run_in_user: Change type of code label

[Sean Christopherson]

On Thu, Apr 13, 2023, Mathias Krause wrote:
> Use an array type to refer to the code label 'ret_to_kernel'.

Why?  Taking the address of a label when setting what is effectively the target
of a branch seems more intuitive than pointing at an array (that's not an array).

> No functional change.
> 
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>  lib/x86/usermode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
> index e22fb8f0132b..b976123ca753 100644
> --- a/lib/x86/usermode.c
> +++ b/lib/x86/usermode.c
> @@ -35,12 +35,12 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
>  		uint64_t arg1, uint64_t arg2, uint64_t arg3,
>  		uint64_t arg4, bool *raised_vector)
>  {
> -	extern char ret_to_kernel;
> +	extern char ret_to_kernel[];
>  	uint64_t rax = 0;
>  	static unsigned char user_stack[USERMODE_STACK_SIZE];
>  
>  	*raised_vector = 0;
> -	set_idt_entry(RET_TO_KERNEL_IRQ, &ret_to_kernel, 3);
> +	set_idt_entry(RET_TO_KERNEL_IRQ, ret_to_kernel, 3);
>  	handle_exception(fault_vector,
>  			restore_exec_to_jmpbuf_exception_handler);
>  
> -- 
> 2.39.2
> 

Re: [kvm-unit-tests PATCH v2 00/16] x86: cleanups, fixes and new tests

[Sean Christopherson]

On Thu, 13 Apr 2023 20:42:03 +0200, Mathias Krause wrote:
> v1: https://lore.kernel.org/kvm/b6322bd0-3639-fb2a-7211-974386865bac@grsecurity.net/
> 
> This is v2 of the "non-canonical memory access" test. It evolved into a
> small series, bringing cleanups and fixes along the way.
> 
> I integrated Sean's feedback and changed the test to make use of
> ASM_TRY() instead of using the hand-rolled exception handler. I also
> switched all other users in emulator64.c to ASM_TRY() and was able to
> drop the one-off exception handler all together.
> 
> [...]

Applied everything except the code label change to kvm-x86 next.  I replied to
that specific patch, feel free to follow-up there.

I tweaked "x86/run_in_user: Reload SS after successful return" to use an "rm"
constraint instead of hardcoding use of AX, holler if that's wrong for some
reason.  I'm planning on sending a pull request later this week, so you've got
a few days to object.

Thanks a ton for the cleanups!

[01/16] x86: Drop types.h
        https://github.com/kvm-x86/kvm-unit-tests/commit/0452fa5aecea
[02/16] x86: Use symbolic names in exception_mnemonic()
        https://github.com/kvm-x86/kvm-unit-tests/commit/8cfb268d401b
[03/16] x86: Add vendor specific exception vectors
        https://github.com/kvm-x86/kvm-unit-tests/commit/f224dba008df
[04/16] x86/cet: Use symbolic name for #CP
        https://github.com/kvm-x86/kvm-unit-tests/commit/00d585d8731b
[05/16] x86/access: Use 'bool' type as defined via libcflat.h
        https://github.com/kvm-x86/kvm-unit-tests/commit/c304eda6ae7f
[06/16] x86/run_in_user: Change type of code label
	DID NOT APPLY
[07/16] x86/run_in_user: Preserve exception handler
        https://github.com/kvm-x86/kvm-unit-tests/commit/d0ef95181cfb
[08/16] x86/run_in_user: Relax register constraints of inline asm
        https://github.com/kvm-x86/kvm-unit-tests/commit/45bafaf28fbb
[09/16] x86/run_in_user: Reload SS after successful return
        https://github.com/kvm-x86/kvm-unit-tests/commit/8338209b8245
[10/16] x86/fault_test: Preserve exception handler
        https://github.com/kvm-x86/kvm-unit-tests/commit/11aac640d01b
[11/16] x86/emulator64: Relax register constraints for usr_gs_mov()
        https://github.com/kvm-x86/kvm-unit-tests/commit/c66547850058
[12/16] x86/emulator64: Switch test_sreg() to ASM_TRY()
        https://github.com/kvm-x86/kvm-unit-tests/commit/9d74b31d1c81
[13/16] x86/emulator64: Add non-null selector test
        https://github.com/kvm-x86/kvm-unit-tests/commit/23c647d0ef29
[14/16] x86/emulator64: Switch test_jmp_noncanonical() to ASM_TRY()
        https://github.com/kvm-x86/kvm-unit-tests/commit/ac4f843474b4
[15/16] x86/emulator64: Switch test_mmx_movq_mf() to ASM_TRY()
        https://github.com/kvm-x86/kvm-unit-tests/commit/5a3515ea1bc2
[16/16] x86/emulator64: Test non-canonical memory access exceptions
        https://github.com/kvm-x86/kvm-unit-tests/commit/e3a9b2f5490e

--
https://github.com/kvm-x86/kvm-unit-tests/tree/next

Re: [kvm-unit-tests PATCH v2 06/16] x86/run_in_user: Change type of code label

[Mathias Krause]

On 13.06.23 02:32, Sean Christopherson wrote:
> On Thu, Apr 13, 2023, Mathias Krause wrote:
>> Use an array type to refer to the code label 'ret_to_kernel'.
> 
> Why?  Taking the address of a label when setting what is effectively the target
> of a branch seems more intuitive than pointing at an array (that's not an array).

Well, the flexible array notation is what my understanding of referring
to a "label" defined in ASM is. I'm probably biased, as that's a pattern
used a lot in the Linux kernel but trying to look at the individual
semantics may make it clearer why I prefer 'extern char sym[]' over
'extern char sym'.

The current code refers to the code sequence starting at 'ret_to_kernel'
by creating an alias of it's first instruction byte. However, we're not
interested in the first instruction byte at all. We want a symbolic
handle of the begin of that sequence, which might be an unknown number
of bytes. But that's also a detail that doesn't matter. We only what to
know the start. By referring to it as 'extern char' implies that there
is at least a single byte. (Let's ignore the hair splitting about just
taking the address vs. actually dereferencing it (which we do not).) By
looking at another code example, that byte may not actually be there!
From x86/vmx_tests.c:

  extern char vmx_mtf_pdpte_guest_begin;
  extern char vmx_mtf_pdpte_guest_end;

  asm("vmx_mtf_pdpte_guest_begin:\n\t"
      "mov %cr0, %rax\n\t"    /* save CR0 with PG=1                 */
      "vmcall\n\t"            /* on return from this CR0.PG=0       */
      "mov %rax, %cr0\n\t"    /* restore CR0.PG=1 to enter PAE mode */
      "vmcall\n\t"
      "retq\n\t"
      "vmx_mtf_pdpte_guest_end:");

The byte referred to via &vmx_mtf_pdpte_guest_end may not even be mapped
due to -ftoplevel-reorder possibly putting that asm block at the very
end of the compilation unit.

By using 'extern char []' instead this nastiness is avoided by referring
to an unknown sized byte sequence starting at that symbol (with zero
being a totally valid length). We don't need to know how many bytes
follow the label. All we want to know is its address. And that's what an
array type provides easily.

But I can understand, that YMMV and you prefer it the other way around.

> 
>> No functional change.
>>
>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>> ---
>>  lib/x86/usermode.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
>> index e22fb8f0132b..b976123ca753 100644
>> --- a/lib/x86/usermode.c
>> +++ b/lib/x86/usermode.c
>> @@ -35,12 +35,12 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
>>  		uint64_t arg1, uint64_t arg2, uint64_t arg3,
>>  		uint64_t arg4, bool *raised_vector)
>>  {
>> -	extern char ret_to_kernel;
>> +	extern char ret_to_kernel[];
>>  	uint64_t rax = 0;
>>  	static unsigned char user_stack[USERMODE_STACK_SIZE];
>>  
>>  	*raised_vector = 0;
>> -	set_idt_entry(RET_TO_KERNEL_IRQ, &ret_to_kernel, 3);
>> +	set_idt_entry(RET_TO_KERNEL_IRQ, ret_to_kernel, 3);
>>  	handle_exception(fault_vector,
>>  			restore_exec_to_jmpbuf_exception_handler);
>>  
>> -- 
>> 2.39.2
>>

Re: [kvm-unit-tests PATCH v2 00/16] x86: cleanups, fixes and new tests

[Mathias Krause]

On 13.06.23 23:40, Sean Christopherson wrote:
> On Thu, 13 Apr 2023 20:42:03 +0200, Mathias Krause wrote:
>> v1: https://lore.kernel.org/kvm/b6322bd0-3639-fb2a-7211-974386865bac@grsecurity.net/
>>
>> This is v2 of the "non-canonical memory access" test. It evolved into a
>> small series, bringing cleanups and fixes along the way.
>>
>> I integrated Sean's feedback and changed the test to make use of
>> ASM_TRY() instead of using the hand-rolled exception handler. I also
>> switched all other users in emulator64.c to ASM_TRY() and was able to
>> drop the one-off exception handler all together.
>>
>> [...]
> 
> Applied everything except the code label change to kvm-x86 next.  I replied to
> that specific patch, feel free to follow-up there.

I did, but I have no strong opinion on getting it merged. It's just the
coding style I'm used to. If KUT's different, then that be it.

> 
> I tweaked "x86/run_in_user: Reload SS after successful return" to use an "rm"
> constraint instead of hardcoding use of AX, holler if that's wrong for some
> reason.  I'm planning on sending a pull request later this week, so you've got
> a few days to object.

I used "i" as a constraint as that's what KERNEL_DS really is: an
integer constant. But I can see that the stunt of actually loading %ss
without clobbering a register isn't all that nice looking. I used (R)AX
specifically as that avoids allocating yet another register for this ASM
block. However, there are still enough left and the stack pointer is
already restored at that point so using "rm" instead should be fine.

Just noticed that the constraints for 'rax' should be "=&a" instead of
"+a" as the ASM doesn't care about its initial value, just needs to
prevent the compiler from allocating AX for any of the input register
variables. But that can be a separate cleanup.

> 
> Thanks a ton for the cleanups!
> 
> [01/16] x86: Drop types.h
>         https://github.com/kvm-x86/kvm-unit-tests/commit/0452fa5aecea
> [02/16] x86: Use symbolic names in exception_mnemonic()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/8cfb268d401b
> [03/16] x86: Add vendor specific exception vectors
>         https://github.com/kvm-x86/kvm-unit-tests/commit/f224dba008df
> [04/16] x86/cet: Use symbolic name for #CP
>         https://github.com/kvm-x86/kvm-unit-tests/commit/00d585d8731b
> [05/16] x86/access: Use 'bool' type as defined via libcflat.h
>         https://github.com/kvm-x86/kvm-unit-tests/commit/c304eda6ae7f
> [06/16] x86/run_in_user: Change type of code label
> 	DID NOT APPLY
> [07/16] x86/run_in_user: Preserve exception handler
>         https://github.com/kvm-x86/kvm-unit-tests/commit/d0ef95181cfb
> [08/16] x86/run_in_user: Relax register constraints of inline asm
>         https://github.com/kvm-x86/kvm-unit-tests/commit/45bafaf28fbb
> [09/16] x86/run_in_user: Reload SS after successful return
>         https://github.com/kvm-x86/kvm-unit-tests/commit/8338209b8245
> [10/16] x86/fault_test: Preserve exception handler
>         https://github.com/kvm-x86/kvm-unit-tests/commit/11aac640d01b
> [11/16] x86/emulator64: Relax register constraints for usr_gs_mov()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/c66547850058
> [12/16] x86/emulator64: Switch test_sreg() to ASM_TRY()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/9d74b31d1c81
> [13/16] x86/emulator64: Add non-null selector test
>         https://github.com/kvm-x86/kvm-unit-tests/commit/23c647d0ef29
> [14/16] x86/emulator64: Switch test_jmp_noncanonical() to ASM_TRY()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/ac4f843474b4
> [15/16] x86/emulator64: Switch test_mmx_movq_mf() to ASM_TRY()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/5a3515ea1bc2
> [16/16] x86/emulator64: Test non-canonical memory access exceptions
>         https://github.com/kvm-x86/kvm-unit-tests/commit/e3a9b2f5490e

Thanks,
Mathias

> 
> --
> https://github.com/kvm-x86/kvm-unit-tests/tree/next

Re: [kvm-unit-tests PATCH v2 06/16] x86/run_in_user: Change type of code label

[Sean Christopherson]

On Wed, Jun 14, 2023, Mathias Krause wrote:
> On 13.06.23 02:32, Sean Christopherson wrote:
> > On Thu, Apr 13, 2023, Mathias Krause wrote:
> >> Use an array type to refer to the code label 'ret_to_kernel'.
> > 
> > Why?  Taking the address of a label when setting what is effectively the target
> > of a branch seems more intuitive than pointing at an array (that's not an array).
> 
> Well, the flexible array notation is what my understanding of referring
> to a "label" defined in ASM is. I'm probably biased, as that's a pattern
> used a lot in the Linux kernel but trying to look at the individual
> semantics may make it clearer why I prefer 'extern char sym[]' over
> 'extern char sym'.
> 
> The current code refers to the code sequence starting at 'ret_to_kernel'
> by creating an alias of it's first instruction byte. However, we're not
> interested in the first instruction byte at all. We want a symbolic
> handle of the begin of that sequence, which might be an unknown number
> of bytes. But that's also a detail that doesn't matter. We only what to
> know the start. By referring to it as 'extern char' implies that there
> is at least a single byte. (Let's ignore the hair splitting about just
> taking the address vs. actually dereferencing it (which we do not).) By
> looking at another code example, that byte may not actually be there!
> >From x86/vmx_tests.c:
> 
>   extern char vmx_mtf_pdpte_guest_begin;
>   extern char vmx_mtf_pdpte_guest_end;
> 
>   asm("vmx_mtf_pdpte_guest_begin:\n\t"
>       "mov %cr0, %rax\n\t"    /* save CR0 with PG=1                 */
>       "vmcall\n\t"            /* on return from this CR0.PG=0       */
>       "mov %rax, %cr0\n\t"    /* restore CR0.PG=1 to enter PAE mode */
>       "vmcall\n\t"
>       "retq\n\t"
>       "vmx_mtf_pdpte_guest_end:");
> 
> The byte referred to via &vmx_mtf_pdpte_guest_end may not even be mapped
> due to -ftoplevel-reorder possibly putting that asm block at the very
> end of the compilation unit.
> 
> By using 'extern char []' instead this nastiness is avoided by referring
> to an unknown sized byte sequence starting at that symbol (with zero
> being a totally valid length). We don't need to know how many bytes
> follow the label. All we want to know is its address. And that's what an
> array type provides easily.

I think my hangup is that arrays make me think of data, not code.  I wouldn't
object if this were new code, but I dislike changing existing code to something
that's arguably just as flawed.

What if we declare the label as a function?  That's not exactly correct either,
but IMO it's closer to the truth, and let's us document that the kernel trampoline
has a return value, i.e. preserves/outputs RAX.

diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index c3ec0ad7..a46a369a 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -31,17 +31,18 @@ static void restore_exec_to_jmpbuf_exception_handler(struct ex_regs *regs)
 #endif
 }
 
+uint64_t ret_to_kernel(void);
+
 uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
                uint64_t arg1, uint64_t arg2, uint64_t arg3,
                uint64_t arg4, bool *raised_vector)
 {
-       extern char ret_to_kernel;
        volatile uint64_t rax = 0;
        static unsigned char user_stack[USERMODE_STACK_SIZE];
        handler old_ex;
 
        *raised_vector = 0;
-       set_idt_entry(RET_TO_KERNEL_IRQ, &ret_to_kernel, 3);
+       set_idt_entry(RET_TO_KERNEL_IRQ, ret_to_kernel, 3);
        old_ex = handle_exception(fault_vector,
                                  restore_exec_to_jmpbuf_exception_handler);
 

Re: [kvm-unit-tests PATCH v2 06/16] x86/run_in_user: Change type of code label

[Mathias Krause]

On 15.06.23 17:18, Sean Christopherson wrote:
> On Wed, Jun 14, 2023, Mathias Krause wrote:
>> On 13.06.23 02:32, Sean Christopherson wrote:
>>> On Thu, Apr 13, 2023, Mathias Krause wrote:
>>>> Use an array type to refer to the code label 'ret_to_kernel'.
>>>
>>> Why?  Taking the address of a label when setting what is effectively the target
>>> of a branch seems more intuitive than pointing at an array (that's not an array).
>>
>> Well, the flexible array notation is what my understanding of referring
>> to a "label" defined in ASM is. I'm probably biased, as that's a pattern
>> used a lot in the Linux kernel but trying to look at the individual
>> semantics may make it clearer why I prefer 'extern char sym[]' over
>> 'extern char sym'.
>>
>> The current code refers to the code sequence starting at 'ret_to_kernel'
>> by creating an alias of it's first instruction byte. However, we're not
>> interested in the first instruction byte at all. We want a symbolic
>> handle of the begin of that sequence, which might be an unknown number
>> of bytes. But that's also a detail that doesn't matter. We only what to
>> know the start. By referring to it as 'extern char' implies that there
>> is at least a single byte. (Let's ignore the hair splitting about just
>> taking the address vs. actually dereferencing it (which we do not).) By
>> looking at another code example, that byte may not actually be there!
>> >From x86/vmx_tests.c:
>>
>>   extern char vmx_mtf_pdpte_guest_begin;
>>   extern char vmx_mtf_pdpte_guest_end;
>>
>>   asm("vmx_mtf_pdpte_guest_begin:\n\t"
>>       "mov %cr0, %rax\n\t"    /* save CR0 with PG=1                 */
>>       "vmcall\n\t"            /* on return from this CR0.PG=0       */
>>       "mov %rax, %cr0\n\t"    /* restore CR0.PG=1 to enter PAE mode */
>>       "vmcall\n\t"
>>       "retq\n\t"
>>       "vmx_mtf_pdpte_guest_end:");
>>
>> The byte referred to via &vmx_mtf_pdpte_guest_end may not even be mapped
>> due to -ftoplevel-reorder possibly putting that asm block at the very
>> end of the compilation unit.
>>
>> By using 'extern char []' instead this nastiness is avoided by referring
>> to an unknown sized byte sequence starting at that symbol (with zero
>> being a totally valid length). We don't need to know how many bytes
>> follow the label. All we want to know is its address. And that's what an
>> array type provides easily.
> 
> I think my hangup is that arrays make me think of data, not code.

I can say the same for 'extern char' -- that's clearly data, not even
r/o data, less so .text ;)

>                                                                    I wouldn't
> object if this were new code, but I dislike changing existing code to something
> that's arguably just as flawed.

Fair enough.

> What if we declare the label as a function?  That's not exactly correct either,
> but IMO it's closer to the truth, and let's us document that the kernel trampoline
> has a return value, i.e. preserves/outputs RAX.

No, please don't. It's *not* a function, not at all. First thing that
code block does is switching the stack, wich is already a bummer. But it
also has no return instruction, making it fall-through to the middle of
run_in_user() and execute code out of context. It's really just a label
to mark the beginning of a code sequence we have to care about.

Now, we do not want to ever call ret_to_kernel() from C, but declaring
it as a function just feels way more worse than having a "wrong" extern
char declaration.

So, let's leave the code as-is. It's not broken, just imperfect.

> 
> diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
> index c3ec0ad7..a46a369a 100644
> --- a/lib/x86/usermode.c
> +++ b/lib/x86/usermode.c
> @@ -31,17 +31,18 @@ static void restore_exec_to_jmpbuf_exception_handler(struct ex_regs *regs)
>  #endif
>  }
>  
> +uint64_t ret_to_kernel(void);
> +
>  uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
>                 uint64_t arg1, uint64_t arg2, uint64_t arg3,
>                 uint64_t arg4, bool *raised_vector)
>  {
> -       extern char ret_to_kernel;
>         volatile uint64_t rax = 0;
>         static unsigned char user_stack[USERMODE_STACK_SIZE];
>         handler old_ex;
>  
>         *raised_vector = 0;
> -       set_idt_entry(RET_TO_KERNEL_IRQ, &ret_to_kernel, 3);
> +       set_idt_entry(RET_TO_KERNEL_IRQ, ret_to_kernel, 3);
>         old_ex = handle_exception(fault_vector,
>                                   restore_exec_to_jmpbuf_exception_handler);
>  
>