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
Add a macro which prevents a function from getting sealed if there are
no compile-time references to it.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Debugged-by: Peter Zijlstra <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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
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
Add a macro which prevents a function from getting sealed if there are
no compile-time references to it.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 ;-)