[PATCH 0/3] x86/kvm, objtool: Fix KVM "missing ENDBR" bug

[PATCH 0/3] x86/kvm, objtool: Fix KVM "missing ENDBR" bug

[Josh Poimboeuf]

Fix the "missing ENDBR" KVM bug, along with a couple of preparatory
patches.

Josh Poimboeuf (3):
  x86/ibt, objtool: Add IBT_NOSEAL()
  x86/kvm: Simplify FOP_SETCC()
  x86/kvm: Fix "missing ENDBR" BUG for fastop functions

 arch/x86/include/asm/ibt.h | 10 ++++++++++
 arch/x86/kvm/emulate.c     | 26 ++++++--------------------
 tools/objtool/check.c      |  3 ++-
 3 files changed, 18 insertions(+), 21 deletions(-)

-- 
2.37.2

[PATCH 1/3] x86/ibt, objtool: Add IBT_NOSEAL()

[Josh Poimboeuf]

Add a macro which prevents a function from getting sealed if there are
no compile-time references to it.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/include/asm/ibt.h | 10 ++++++++++
 tools/objtool/check.c      |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
index 689880eca9ba..372e8eee6e02 100644
--- a/arch/x86/include/asm/ibt.h
+++ b/arch/x86/include/asm/ibt.h
@@ -31,6 +31,16 @@
 
 #define __noendbr	__attribute__((nocf_check))
 
+/*
+ * Create a dummy function pointer reference to prevent objtool from marking
+ * the function as needing to be "sealed" (i.e. ENDBR converted to NOP by
+ * apply_ibt_endbr()).
+ */
+#define IBT_NOSEAL(fname)				\
+	".pushsection .discard.ibt_endbr_noseal\n\t"	\
+	_ASM_PTR fname "\n\t"				\
+	".popsection\n\t"
+
 static inline __attribute_const__ u32 gen_endbr(void)
 {
 	u32 endbr;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0cec74da7ffe..91678252a9b6 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4096,7 +4096,8 @@ static int validate_ibt(struct objtool_file *file)
 		 * These sections can reference text addresses, but not with
 		 * the intent to indirect branch to them.
 		 */
-		if (!strncmp(sec->name, ".discard", 8)			||
+		if ((!strncmp(sec->name, ".discard", 8) &&
+		     strcmp(sec->name, ".discard.ibt_endbr_noseal"))	||
 		    !strncmp(sec->name, ".debug", 6)			||
 		    !strcmp(sec->name, ".altinstructions")		||
 		    !strcmp(sec->name, ".ibt_endbr_seal")		||
-- 
2.37.2

[PATCH 2/3] x86/kvm: Simplify FOP_SETCC()

[Josh Poimboeuf]

SETCC_ALIGN and FOP_ALIGN are both 16.  Remove the special casing for
FOP_SETCC() and just make it a normal fastop.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/kvm/emulate.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b4eeb7c75dfa..205d566ebd72 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -446,27 +446,12 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 	FOP_END
 
 /* Special case for SETcc - 1 instruction per cc */
-
-/*
- * Depending on .config the SETcc functions look like:
- *
- * ENDBR			[4 bytes; CONFIG_X86_KERNEL_IBT]
- * SETcc %al			[3 bytes]
- * RET | JMP __x86_return_thunk	[1,5 bytes; CONFIG_RETHUNK]
- * INT3				[1 byte; CONFIG_SLS]
- */
-#define SETCC_ALIGN	16
-
 #define FOP_SETCC(op) \
-	".align " __stringify(SETCC_ALIGN) " \n\t" \
-	".type " #op ", @function \n\t" \
-	#op ": \n\t" \
-	ASM_ENDBR \
+	FOP_FUNC(op) \
 	#op " %al \n\t" \
-	__FOP_RET(#op) \
-	".skip " __stringify(SETCC_ALIGN) " - (.-" #op "), 0xcc \n\t"
+	FOP_RET(op)
 
-__FOP_START(setcc, SETCC_ALIGN)
+FOP_START(setcc)
 FOP_SETCC(seto)
 FOP_SETCC(setno)
 FOP_SETCC(setc)
@@ -1079,7 +1064,7 @@ static int em_bsr_c(struct x86_emulate_ctxt *ctxt)
 static __always_inline u8 test_cc(unsigned int condition, unsigned long flags)
 {
 	u8 rc;
-	void (*fop)(void) = (void *)em_setcc + SETCC_ALIGN * (condition & 0xf);
+	void (*fop)(void) = (void *)em_setcc + FASTOP_SIZE * (condition & 0xf);
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
 	asm("push %[flags]; popf; " CALL_NOSPEC
-- 
2.37.2

[PATCH 3/3] x86/kvm: Fix "missing ENDBR" BUG for fastop functions

[Josh Poimboeuf]

The following BUG was reported:

  traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]
  ------------[ cut here ]------------
  kernel BUG at arch/x86/kernel/traps.c:253!
  invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
   <TASK>
   asm_exc_control_protection+0x2b/0x30
  RIP: 0010:andw_ax_dx+0x0/0x10 [kvm]
  Code: c3 cc cc cc cc 0f 1f 44 00 00 66 0f 1f 00 48 19 d0 c3 cc cc cc
        cc 0f 1f 40 00 f3 0f 1e fa 20 d0 c3 cc cc cc cc 0f 1f 44 00 00
        <66> 0f 1f 00 66 21 d0 c3 cc cc cc cc 0f 1f 40 00 66 0f 1f 00 21
        d0

   ? andb_al_dl+0x10/0x10 [kvm]
   ? fastop+0x5d/0xa0 [kvm]
   x86_emulate_insn+0x822/0x1060 [kvm]
   x86_emulate_instruction+0x46f/0x750 [kvm]
   complete_emulated_mmio+0x216/0x2c0 [kvm]
   kvm_arch_vcpu_ioctl_run+0x604/0x650 [kvm]
   kvm_vcpu_ioctl+0x2f4/0x6b0 [kvm]
   ? wake_up_q+0xa0/0xa0

The BUG occurred because the ENDBR in the andw_ax_dx() fastop function
had been incorrectly "sealed" (converted to a NOP) by apply_ibt_endbr().

Objtool marked it to be sealed because KVM has no compile-time
references to the function.  Instead KVM calculates its address at
runtime.

Prevent objtool from annotating fastop functions as sealable by creating
throwaway dummy compile-time references to the functions.

Fixes: 6649fa876da4 ("x86/ibt,kvm: Add ENDBR to fastops")
Reported-by: Pengfei Xu <pengfei.xu@intel.com>
Debugged-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/kvm/emulate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 205d566ebd72..f092c54d1a2f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -326,7 +326,8 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 	".align " __stringify(FASTOP_SIZE) " \n\t" \
 	".type " name ", @function \n\t" \
 	name ":\n\t" \
-	ASM_ENDBR
+	ASM_ENDBR \
+	IBT_NOSEAL(name)
 
 #define FOP_FUNC(name) \
 	__FOP_FUNC(#name)
-- 
2.37.2

Re: [PATCH 0/3] x86/kvm, objtool: Fix KVM "missing ENDBR" bug

[Paolo Bonzini]

On 8/18/22 17:53, Josh Poimboeuf wrote:
> Fix the "missing ENDBR" KVM bug, along with a couple of preparatory
> patches.
> 
> Josh Poimboeuf (3):
>    x86/ibt, objtool: Add IBT_NOSEAL()
>    x86/kvm: Simplify FOP_SETCC()
>    x86/kvm: Fix "missing ENDBR" BUG for fastop functions
> 
>   arch/x86/include/asm/ibt.h | 10 ++++++++++
>   arch/x86/kvm/emulate.c     | 26 ++++++--------------------
>   tools/objtool/check.c      |  3 ++-
>   3 files changed, 18 insertions(+), 21 deletions(-)
> 

Queued, thanks!

Paolo

[PATCH v1.1] x86/ibt, objtool: Add IBT_NOSEAL()

[Josh Poimboeuf]

Add a macro which prevents a function from getting sealed if there are
no compile-time references to it.

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
v1.1:
- add empty IBT_NOSEAL for CONFIG_X86_KERNEL_IBT=n

 arch/x86/include/asm/ibt.h | 11 +++++++++++
 tools/objtool/check.c      |  3 ++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
index 689880eca9ba..9b08082a5d9f 100644
--- a/arch/x86/include/asm/ibt.h
+++ b/arch/x86/include/asm/ibt.h
@@ -31,6 +31,16 @@
 
 #define __noendbr	__attribute__((nocf_check))
 
+/*
+ * Create a dummy function pointer reference to prevent objtool from marking
+ * the function as needing to be "sealed" (i.e. ENDBR converted to NOP by
+ * apply_ibt_endbr()).
+ */
+#define IBT_NOSEAL(fname)				\
+	".pushsection .discard.ibt_endbr_noseal\n\t"	\
+	_ASM_PTR fname "\n\t"				\
+	".popsection\n\t"
+
 static inline __attribute_const__ u32 gen_endbr(void)
 {
 	u32 endbr;
@@ -84,6 +94,7 @@ extern __noendbr void ibt_restore(u64 save);
 #ifndef __ASSEMBLY__
 
 #define ASM_ENDBR
+#define IBT_NOSEAL(name)
 
 #define __noendbr
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0cec74da7ffe..91678252a9b6 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4096,7 +4096,8 @@ static int validate_ibt(struct objtool_file *file)
 		 * These sections can reference text addresses, but not with
 		 * the intent to indirect branch to them.
 		 */
-		if (!strncmp(sec->name, ".discard", 8)			||
+		if ((!strncmp(sec->name, ".discard", 8) &&
+		     strcmp(sec->name, ".discard.ibt_endbr_noseal"))	||
 		    !strncmp(sec->name, ".debug", 6)			||
 		    !strcmp(sec->name, ".altinstructions")		||
 		    !strcmp(sec->name, ".ibt_endbr_seal")		||
-- 
2.37.2

Re: [PATCH v1.1] x86/ibt, objtool: Add IBT_NOSEAL()

[Pengfei Xu]

Hi Poimboeuf,
  I installed your patches based on v5.19 kernel.

  And ran syzkaller test on TGL-H and ADL-P for more than
  4 hours with above kernel, this issue could not be reproduced.
  This issue should be fixed.

  Thanks!
  BR.

On 2022-08-18 at 14:39:27 -0700, Josh Poimboeuf wrote:
> Add a macro which prevents a function from getting sealed if there are
> no compile-time references to it.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
> v1.1:
> - add empty IBT_NOSEAL for CONFIG_X86_KERNEL_IBT=n
> 
>  arch/x86/include/asm/ibt.h | 11 +++++++++++
>  tools/objtool/check.c      |  3 ++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
> index 689880eca9ba..9b08082a5d9f 100644
> --- a/arch/x86/include/asm/ibt.h
> +++ b/arch/x86/include/asm/ibt.h
> @@ -31,6 +31,16 @@
>  
>  #define __noendbr	__attribute__((nocf_check))
>  
> +/*
> + * Create a dummy function pointer reference to prevent objtool from marking
> + * the function as needing to be "sealed" (i.e. ENDBR converted to NOP by
> + * apply_ibt_endbr()).
> + */
> +#define IBT_NOSEAL(fname)				\
> +	".pushsection .discard.ibt_endbr_noseal\n\t"	\
> +	_ASM_PTR fname "\n\t"				\
> +	".popsection\n\t"
> +
>  static inline __attribute_const__ u32 gen_endbr(void)
>  {
>  	u32 endbr;
> @@ -84,6 +94,7 @@ extern __noendbr void ibt_restore(u64 save);
>  #ifndef __ASSEMBLY__
>  
>  #define ASM_ENDBR
> +#define IBT_NOSEAL(name)
>  
>  #define __noendbr
>  
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0cec74da7ffe..91678252a9b6 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -4096,7 +4096,8 @@ static int validate_ibt(struct objtool_file *file)
>  		 * These sections can reference text addresses, but not with
>  		 * the intent to indirect branch to them.
>  		 */
> -		if (!strncmp(sec->name, ".discard", 8)			||
> +		if ((!strncmp(sec->name, ".discard", 8) &&
> +		     strcmp(sec->name, ".discard.ibt_endbr_noseal"))	||
>  		    !strncmp(sec->name, ".debug", 6)			||
>  		    !strcmp(sec->name, ".altinstructions")		||
>  		    !strcmp(sec->name, ".ibt_endbr_seal")		||
> -- 
> 2.37.2
> 

Re: [PATCH 0/3] x86/kvm, objtool: Fix KVM "missing ENDBR" bug

[Peter Zijlstra]

On Thu, Aug 18, 2022 at 07:25:57PM +0200, Paolo Bonzini wrote:
> On 8/18/22 17:53, Josh Poimboeuf wrote:
> > Fix the "missing ENDBR" KVM bug, along with a couple of preparatory
> > patches.
> > 
> > Josh Poimboeuf (3):
> >    x86/ibt, objtool: Add IBT_NOSEAL()
> >    x86/kvm: Simplify FOP_SETCC()
> >    x86/kvm: Fix "missing ENDBR" BUG for fastop functions
> > 
> >   arch/x86/include/asm/ibt.h | 10 ++++++++++
> >   arch/x86/kvm/emulate.c     | 26 ++++++--------------------
> >   tools/objtool/check.c      |  3 ++-
> >   3 files changed, 18 insertions(+), 21 deletions(-)
> > 
> 
> Queued, thanks!

OK, let me drop them from the queue then ;-)