Re: [kvm-unit-tests PATCH 4/4] x86: Extend ASM_TRY to handle #UD thrown by FEP-triggered emulator

[Sean Christopherson <seanjc@google.com>]

On Wed, Aug 03, 2022, Michal Luczaj wrote:
> TRY_ASM() mishandles #UD thrown by the forced-emulation-triggered emulator.
> While the faulting address stored in the exception table points at forced
> emulation prefix, when #UD comes, RIP is 5 bytes (size of KVM_FEP) ahead
> and the exception ends up unhandled.

Ah, but that's only the behavior if FEP is allowed.  If FEP is disabled, then the
#UD will be on the prefix.

> Suggested-by: Sean Christopherson <seanjc@google.com>

Heh, I didn't really suggest this because I didn't even realize it was a problem :-)

> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> While here, I've also took the opportunity to merge both 32 and 64-bit
> versions of ASM_TRY() (.dc.a for .long and .quad), but perhaps there
> were some reasons for not using .dc.a?

This should be a separate patch, and probably as the very last patch in case dc.a
isn't viable for whatever reason.  I've never seen/used dc.a so I really have no
idea whether or not it's ok to use.

As a "safe" alternative that can be done before adding ASM_TRY_FEP(), we can steal
the kernel's __ASM_SEL() approach so that you don't have to implement two versions
of ASM_TRY_FEP().  That's worth doing because __ASM_SEL() can probably be used in
other places too.  Patch at the bottom.

>  lib/x86/desc.h | 11 +++++------
>  x86/emulator.c |  4 ++--
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index 2a285eb..99cc224 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -80,21 +80,20 @@ typedef struct  __attribute__((packed)) {
>  	u16 iomap_base;
>  } tss64_t;
>  
> -#ifdef __x86_64
>  #define ASM_TRY(catch)			\
>  	"movl $0, %%gs:4 \n\t"		\
>  	".pushsection .data.ex \n\t"	\
> -	".quad 1111f, " catch "\n\t"	\
> +	".dc.a 1111f, " catch "\n\t"	\
>  	".popsection \n\t"		\
>  	"1111:"
> -#else
> -#define ASM_TRY(catch)			\
> +
> +#define ASM_TRY_PREFIXED(prefix, catch)	\

Rather than a generic ASM_TRY_PREFIXED, I think it makes sense to add an explicit
ASM_TRY_FEP() that takes in only the label and hardcodes the FEP prefix.  The "#UD
skips the prefix" behavior is unique to "successful" forced emulation, so this is
literally the only scenario where skipping a prefix is expected/correct.

*sigh*

That'll require moving the KVM_FEP definition, but that's a good change on its
own.  Probably throw it in lib/x86/processor.h?

>  	"movl $0, %%gs:4 \n\t"		\
>  	".pushsection .data.ex \n\t"	\
> -	".long 1111f, " catch "\n\t"	\
> +	".dc.a 1111f, " catch "\n\t"	\
>  	".popsection \n\t"		\
> +	prefix "\n\t"			\
>  	"1111:"
> -#endif
>  
>  /*
>   * selector     32-bit                        64-bit
> diff --git a/x86/emulator.c b/x86/emulator.c
> index df0bc49..d2a5302 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -900,8 +900,8 @@ static void test_illegal_lea(void)
>  {
>  	unsigned int vector;
>  
> -	asm volatile (ASM_TRY("1f")
> -		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
> +	asm volatile (ASM_TRY_PREFIXED(KVM_FEP, "1f")
> +		      ".byte 0x8d; .byte 0xc0\n\t"

Put this patch earlier in the series, before test_illegal_lea() is introduced.
Both so that there's no unnecessary churn, and also because running the illegal
LEA testcase without this patch will fail.

>  		      "1:"
>  		      : : : "memory", "eax");

---
From: Sean Christopherson <seanjc@google.com>
Date: Wed, 3 Aug 2022 11:09:41 -0700
Subject: [PATCH] x86: Dedup 32-bit vs. 64-bit ASM_TRY() by stealing kernel's
 __ASM_SEL()

Steal the kernel's __ASM_SEL() implementation and use it to consolidate
ASM_TRY().  The only difference between the 32-bit and 64-bit versions is
the size of the address stored in the table.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 lib/x86/desc.h      | 19 +++++--------------
 lib/x86/processor.h | 12 ++++++++++++
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 2a285eb6..e670ebf2 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -80,21 +80,12 @@ typedef struct  __attribute__((packed)) {
 	u16 iomap_base;
 } tss64_t;

-#ifdef __x86_64
-#define ASM_TRY(catch)			\
-	"movl $0, %%gs:4 \n\t"		\
-	".pushsection .data.ex \n\t"	\
-	".quad 1111f, " catch "\n\t"	\
-	".popsection \n\t"		\
+#define ASM_TRY(catch)						\
+	"movl $0, %%gs:4 \n\t"					\
+	".pushsection .data.ex \n\t"				\
+	__ASM_SEL(.long, .quad) " 1111f,  " catch "\n\t"	\
+	".popsection \n\t"					\
 	"1111:"
-#else
-#define ASM_TRY(catch)			\
-	"movl $0, %%gs:4 \n\t"		\
-	".pushsection .data.ex \n\t"	\
-	".long 1111f, " catch "\n\t"	\
-	".popsection \n\t"		\
-	"1111:"
-#endif

 /*
  * selector     32-bit                        64-bit
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 03242206..30e2de87 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -19,6 +19,18 @@
 #  define S "4"
 #endif

+#ifdef __ASSEMBLY__
+#define __ASM_FORM(x, ...)	x,## __VA_ARGS__
+#else
+#define __ASM_FORM(x, ...)	" " xstr(x,##__VA_ARGS__) " "
+#endif
+
+#ifndef __x86_64__
+#define __ASM_SEL(a,b)		__ASM_FORM(a)
+#else
+#define __ASM_SEL(a,b)		__ASM_FORM(b)
+#endif
+
 #define DB_VECTOR 1
 #define BP_VECTOR 3
 #define UD_VECTOR 6

base-commit: a106b30d39425b7afbaa3bbd4aab16fd26d333e7
--