On Tue, Aug 09, 2022 at 02:44:28AM +0000, Xu, Pengfei wrote:
> Hi Peter,
>
> Greeting!
You again forgot to Cc LKML and relevant people...
> Platform: ADL-P (I tried that TGL-H could reproduce this issue also)
> Kernel: 5.19 mainline kernel with kernel IBT enabled.
>
> Boot up ADL-P, and then run syzkaller fuzzing tests, syzkaller will start up guests(Guest kernel is 5.19 mainline also but doesn't enable kernel IBT) to do fuzzing tests.
> After about 1 hour later, there was "traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]" info generated.
>
> [ 5048.057266] traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]
> [ 5048.057440] ------------[ cut here ]------------
> [ 5048.057457] kernel BUG at arch/x86/kernel/traps.c:253!
> [ 5048.057482] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [ 5048.057908] <TASK>
> [ 5048.057919] asm_exc_control_protection+0x2b/0x30
> [ 5048.057940] RIP: 0010:andw_ax_dx+0x0/0x10 [kvm]
> [ 5048.057999] 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
> ...
> [ 5048.058209] ? andb_al_dl+0x10/0x10 [kvm]
> [ 5048.058265] ? fastop+0x5d/0xa0 [kvm]
> [ 5048.058320] x86_emulate_insn+0x822/0x1060 [kvm]
> [ 5048.058376] x86_emulate_instruction+0x46f/0x750 [kvm]
> [ 5048.058433] complete_emulated_mmio+0x216/0x2c0 [kvm]
> [ 5048.058488] kvm_arch_vcpu_ioctl_run+0x604/0x650 [kvm]
> [ 5048.058543] kvm_vcpu_ioctl+0x2f4/0x6b0 [kvm]
> [ 5048.058590] ? wake_up_q+0xa0/0xa0
> ...
> [ 6324.778942] traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]
> ...
> [ 8760.430810] traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]
Urgh, fastops again :/
> Dmesg and all the ko files in "/lib/modules/5.19.0-m2/kernel/arch/x86" are in attached.
From your kvm.ko:
00000000000376f0 <andw_ax_dx>:
376f0: f3 0f 1e fa endbr64
376f4: 66 21 d0 and %dx,%ax
376f7: e9 00 00 00 00 jmp 376fc <andw_ax_dx+0xc> 376f8: R_X86_64_PLT32 __x86_return_thunk-0x4
376fc: 0f 1f 40 00 nopl 0x0(%rax)
However, the Code: thing above gives:
2a:* 66 0f 1f 00 nopw (%rax) <-- trapping instruction
2e: 66 21 d0 and %dx,%ax
31: c3 ret
32: cc int3
33: cc int3
34: cc int3
35: cc int3
36: 0f 1f 40 00 nopl 0x0(%rax)
From that we can tell gen_endbr_poison() has been used to scribble the
endbr -- so clearly objtool didn't manage to find code references here.
And indeed, kvm.ko's .rela.ibt_endbr_seal section includes:
00000000000002c0 0000000100000002 R_X86_64_PC32 0000000000000000 .text + 376f0
---
Subject: x86/kvm, objtool: Avoid fastop ENDBR from being sealed
Xu reported a number of "Missing ENDBR" triggers for the KVM fastop
emulation code. It turns out that because of how the fastops are set up,
only the first of a series -- the 8 byte variants that overlap with the
em_ symbols -- is found referenced.
Specifically:
.pushsection .text, "ax"
.global em_and
.align 16
em_and:
.align 16
.type andb_al_dl, @function
andb_al_dl:
endbr64
andb %dl, %al
11: jmp __x86_return_thunk
.size andb_al_dl, .-andb_al_dl
.align 16
.type andw_ax_dx, @function
andw_ax_dx:
endbr64
andw %dx, %ax
11: jmp __x86_return_thunk
.size andw_ax_dx, .-andw_ax_dx
.align 16
.type andl_eax_edx, @function
andl_eax_edx:
endbr64
andl %edx, %eax
11: jmp __x86_return_thunk
.size andl_eax_edx, .-andl_eax_edx
.align 16
.type andq_rax_rdx, @function
andq_rax_rdx:
endbr64
andq %rdx, %rax
11: jmp __x86_return_thunk
.size andq_rax_rdx, .-andq_rax_rdx
.popsection
Only has the em_and symbol referenced, resulting in and{w,l,q}_* getting
sealed.
Add (yet another) annotation to inhibit objtool from sealing a specific
ENDBR instance.
Fixes: 6649fa876da4 ("x86/ibt,kvm: Add ENDBR to fastops")
Reported-by: "Xu, Pengfei" <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/ibt.h | 5 +++++
arch/x86/kvm/emulate.c | 4 ++--
include/linux/objtool.h | 6 ++++++
tools/objtool/check.c | 44 ++++++++++++++++++++++++++++++++++++++------
4 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
index 689880eca9ba..f32ba1c4e619 100644
--- a/arch/x86/include/asm/ibt.h
+++ b/arch/x86/include/asm/ibt.h
@@ -29,6 +29,10 @@
#define ASM_ENDBR "endbr32\n\t"
#endif
+#define ASM_ENDBR_NOSEAL \
+ ANNOTATE_NOSEAL \
+ ASM_ENDBR
+
#define __noendbr __attribute__((nocf_check))
static inline __attribute_const__ u32 gen_endbr(void)
@@ -84,6 +88,7 @@ extern __noendbr void ibt_restore(u64 save);
#ifndef __ASSEMBLY__
#define ASM_ENDBR
+#define ASM_ENDBR_NOSEAL
#define __noendbr
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b4eeb7c75dfa..d51ee8a3bcae 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -326,7 +326,7 @@ 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_NOSEAL
#define FOP_FUNC(name) \
__FOP_FUNC(#name)
@@ -461,7 +461,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
".align " __stringify(SETCC_ALIGN) " \n\t" \
".type " #op ", @function \n\t" \
#op ": \n\t" \
- ASM_ENDBR \
+ ASM_ENDBR_NOSEAL \
#op " %al \n\t" \
__FOP_RET(#op) \
".skip " __stringify(SETCC_ALIGN) " - (.-" #op "), 0xcc \n\t"
diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 62c54ffbeeaa..ad752f8b3b36 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -90,6 +90,12 @@ struct unwind_hint {
_ASM_PTR " 986b\n\t" \
".popsection\n\t"
+#define ANNOTATE_NOSEAL \
+ "986: \n\t" \
+ ".pushsection .discard.noseal\n\t" \
+ _ASM_PTR " 986b\n\t" \
+ ".popsection\n\t"
+
#define ASM_REACHABLE \
"998:\n\t" \
".pushsection .discard.reachable\n\t" \
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0cec74da7ffe..0d04d0a707f4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2111,6 +2111,40 @@ static int read_noendbr_hints(struct objtool_file *file)
return 0;
}
+static void mark_endbr_used(struct instruction *insn)
+{
+ if (!list_empty(&insn->call_node))
+ list_del_init(&insn->call_node);
+}
+
+static int read_noseal_hints(struct objtool_file *file)
+{
+ struct section *sec;
+ struct instruction *insn;
+ struct reloc *reloc;
+
+ sec = find_section_by_name(file->elf, ".rela.discard.noseal");
+ if (!sec)
+ return 0;
+
+ list_for_each_entry(reloc, &sec->reloc_list, list) {
+ insn = find_insn(file, reloc->sym->sec, reloc->sym->offset + reloc->addend);
+ if (!insn) {
+ WARN("bad .discard.noseal entry");
+ return -1;
+ }
+
+ if (insn->type != INSN_ENDBR) {
+ WARN_FUNC("ANNOTATE_NOSEAL not on ENDBR", insn->sec, insn->offset);
+ continue;
+ }
+
+ mark_endbr_used(insn);
+ }
+
+ return 0;
+}
+
static int read_retpoline_hints(struct objtool_file *file)
{
struct section *sec;
@@ -2356,6 +2390,10 @@ static int decode_sections(struct objtool_file *file)
if (ret)
return ret;
+ ret = read_noseal_hints(file);
+ if (ret)
+ return ret;
+
/*
* Must be before add_{jump_call}_destination.
*/
@@ -3952,12 +3990,6 @@ static int validate_functions(struct objtool_file *file)
return warnings;
}
-static void mark_endbr_used(struct instruction *insn)
-{
- if (!list_empty(&insn->call_node))
- list_del_init(&insn->call_node);
-}
-
static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
{
struct instruction *dest;
The following commit has been merged into the core/urgent branch of tip:
Commit-ID: 52ad1c24d213f176a2424552dfd0db2f870d96bd
Gitweb: https://git.kernel.org/tip/52ad1c24d213f176a2424552dfd0db2f870d96bd
Author: Peter Zijlstra <[email protected]>
AuthorDate: Wed, 17 Aug 2022 12:56:15 +02:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Wed, 17 Aug 2022 18:05:51 +02:00
x86/kvm, objtool: Avoid fastop ENDBR from being sealed
Subject: x86/kvm, objtool: Avoid fastop ENDBR from being sealed
Xu reported a number of "Missing ENDBR" triggers for the KVM fastop
emulation code. It turns out that because of how the fastops are set up,
only the first of a series -- the 8-byte variants that overlap with the
em_ symbols -- is found referenced.
Specifically:
.pushsection .text, "ax"
.global em_and
.align 16
em_and:
.align 16
.type andb_al_dl, @function
andb_al_dl:
endbr64
andb %dl, %al
11: jmp __x86_return_thunk
.size andb_al_dl, .-andb_al_dl
.align 16
.type andw_ax_dx, @function
andw_ax_dx:
endbr64
andw %dx, %ax
11: jmp __x86_return_thunk
.size andw_ax_dx, .-andw_ax_dx
.align 16
.type andl_eax_edx, @function
andl_eax_edx:
endbr64
andl %edx, %eax
11: jmp __x86_return_thunk
.size andl_eax_edx, .-andl_eax_edx
.align 16
.type andq_rax_rdx, @function
andq_rax_rdx:
endbr64
andq %rdx, %rax
11: jmp __x86_return_thunk
.size andq_rax_rdx, .-andq_rax_rdx
.popsection
Only has the em_and symbol referenced, resulting in and{w,l,q}_* getting
sealed.
Add (yet another) annotation to inhibit objtool from sealing a specific
ENDBR instance.
[ mingo: Sync up tools/include/linux/objtool.h to include/linux/objtool.h. ]
Fixes: 6649fa876da4 ("x86/ibt,kvm: Add ENDBR to fastops")
Reported-by: "Xu, Pengfei" <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/include/asm/ibt.h | 5 ++++-
arch/x86/kvm/emulate.c | 4 +--
include/linux/objtool.h | 6 +++++-
tools/include/linux/objtool.h | 6 +++++-
tools/objtool/check.c | 44 +++++++++++++++++++++++++++++-----
5 files changed, 57 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
index 689880e..f32ba1c 100644
--- a/arch/x86/include/asm/ibt.h
+++ b/arch/x86/include/asm/ibt.h
@@ -29,6 +29,10 @@
#define ASM_ENDBR "endbr32\n\t"
#endif
+#define ASM_ENDBR_NOSEAL \
+ ANNOTATE_NOSEAL \
+ ASM_ENDBR
+
#define __noendbr __attribute__((nocf_check))
static inline __attribute_const__ u32 gen_endbr(void)
@@ -84,6 +88,7 @@ extern __noendbr void ibt_restore(u64 save);
#ifndef __ASSEMBLY__
#define ASM_ENDBR
+#define ASM_ENDBR_NOSEAL
#define __noendbr
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b4eeb7c..d51ee8a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -326,7 +326,7 @@ 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_NOSEAL
#define FOP_FUNC(name) \
__FOP_FUNC(#name)
@@ -461,7 +461,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
".align " __stringify(SETCC_ALIGN) " \n\t" \
".type " #op ", @function \n\t" \
#op ": \n\t" \
- ASM_ENDBR \
+ ASM_ENDBR_NOSEAL \
#op " %al \n\t" \
__FOP_RET(#op) \
".skip " __stringify(SETCC_ALIGN) " - (.-" #op "), 0xcc \n\t"
diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 62c54ff..ad752f8 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -90,6 +90,12 @@ struct unwind_hint {
_ASM_PTR " 986b\n\t" \
".popsection\n\t"
+#define ANNOTATE_NOSEAL \
+ "986: \n\t" \
+ ".pushsection .discard.noseal\n\t" \
+ _ASM_PTR " 986b\n\t" \
+ ".popsection\n\t"
+
#define ASM_REACHABLE \
"998:\n\t" \
".pushsection .discard.reachable\n\t" \
diff --git a/tools/include/linux/objtool.h b/tools/include/linux/objtool.h
index 62c54ff..ad752f8 100644
--- a/tools/include/linux/objtool.h
+++ b/tools/include/linux/objtool.h
@@ -90,6 +90,12 @@ struct unwind_hint {
_ASM_PTR " 986b\n\t" \
".popsection\n\t"
+#define ANNOTATE_NOSEAL \
+ "986: \n\t" \
+ ".pushsection .discard.noseal\n\t" \
+ _ASM_PTR " 986b\n\t" \
+ ".popsection\n\t"
+
#define ASM_REACHABLE \
"998:\n\t" \
".pushsection .discard.reachable\n\t" \
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0cec74d..0d04d0a 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2111,6 +2111,40 @@ static int read_noendbr_hints(struct objtool_file *file)
return 0;
}
+static void mark_endbr_used(struct instruction *insn)
+{
+ if (!list_empty(&insn->call_node))
+ list_del_init(&insn->call_node);
+}
+
+static int read_noseal_hints(struct objtool_file *file)
+{
+ struct section *sec;
+ struct instruction *insn;
+ struct reloc *reloc;
+
+ sec = find_section_by_name(file->elf, ".rela.discard.noseal");
+ if (!sec)
+ return 0;
+
+ list_for_each_entry(reloc, &sec->reloc_list, list) {
+ insn = find_insn(file, reloc->sym->sec, reloc->sym->offset + reloc->addend);
+ if (!insn) {
+ WARN("bad .discard.noseal entry");
+ return -1;
+ }
+
+ if (insn->type != INSN_ENDBR) {
+ WARN_FUNC("ANNOTATE_NOSEAL not on ENDBR", insn->sec, insn->offset);
+ continue;
+ }
+
+ mark_endbr_used(insn);
+ }
+
+ return 0;
+}
+
static int read_retpoline_hints(struct objtool_file *file)
{
struct section *sec;
@@ -2356,6 +2390,10 @@ static int decode_sections(struct objtool_file *file)
if (ret)
return ret;
+ ret = read_noseal_hints(file);
+ if (ret)
+ return ret;
+
/*
* Must be before add_{jump_call}_destination.
*/
@@ -3952,12 +3990,6 @@ static int validate_functions(struct objtool_file *file)
return warnings;
}
-static void mark_endbr_used(struct instruction *insn)
-{
- if (!list_empty(&insn->call_node))
- list_del_init(&insn->call_node);
-}
-
static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
{
struct instruction *dest;
On Wed, Aug 17, 2022 at 12:56:15PM +0200, Peter Zijlstra wrote:
> Add (yet another) annotation to inhibit objtool from sealing a specific
> ENDBR instance.
Nah, just add a throwaway reference to the function. Then no objtool
change is needed:
diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
index 689880eca9ba..3ae795150374 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.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/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f8382abe22ff..88623add0db4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -316,7 +316,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)
Hi Peter,
On 2022-08-17 at 12:56:15 +0200, Peter Zijlstra wrote:
> On Tue, Aug 09, 2022 at 02:44:28AM +0000, Xu, Pengfei wrote:
> > Hi Peter,
> >
> > Greeting!
>
> You again forgot to Cc LKML and relevant people...
Ah, I will Cc LKML and relevant people if I report new issue next time.
I have tested your patch, and I could not rerproduce this issue
when starting the syzkaller test for more than 6 hours.
And I will test Josh Poimboeuf's patch, will update email if I find
some problem.
Thanks!
BR.
>
> > Platform: ADL-P (I tried that TGL-H could reproduce this issue also)
> > Kernel: 5.19 mainline kernel with kernel IBT enabled.
> >
> > Boot up ADL-P, and then run syzkaller fuzzing tests, syzkaller will start up guests(Guest kernel is 5.19 mainline also but doesn't enable kernel IBT) to do fuzzing tests.
> > After about 1 hour later, there was "traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]" info generated.
> >
> > [ 5048.057266] traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]
> > [ 5048.057440] ------------[ cut here ]------------
> > [ 5048.057457] kernel BUG at arch/x86/kernel/traps.c:253!
> > [ 5048.057482] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> > [ 5048.057908] <TASK>
> > [ 5048.057919] asm_exc_control_protection+0x2b/0x30
> > [ 5048.057940] RIP: 0010:andw_ax_dx+0x0/0x10 [kvm]
> > [ 5048.057999] 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
> > ...
> > [ 5048.058209] ? andb_al_dl+0x10/0x10 [kvm]
> > [ 5048.058265] ? fastop+0x5d/0xa0 [kvm]
> > [ 5048.058320] x86_emulate_insn+0x822/0x1060 [kvm]
> > [ 5048.058376] x86_emulate_instruction+0x46f/0x750 [kvm]
> > [ 5048.058433] complete_emulated_mmio+0x216/0x2c0 [kvm]
> > [ 5048.058488] kvm_arch_vcpu_ioctl_run+0x604/0x650 [kvm]
> > [ 5048.058543] kvm_vcpu_ioctl+0x2f4/0x6b0 [kvm]
> > [ 5048.058590] ? wake_up_q+0xa0/0xa0
> > ...
> > [ 6324.778942] traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]
> > ...
> > [ 8760.430810] traps: Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]
>
> Urgh, fastops again :/
>
> > Dmesg and all the ko files in "/lib/modules/5.19.0-m2/kernel/arch/x86" are in attached.
>
> From your kvm.ko:
>
> 00000000000376f0 <andw_ax_dx>:
> 376f0: f3 0f 1e fa endbr64
> 376f4: 66 21 d0 and %dx,%ax
> 376f7: e9 00 00 00 00 jmp 376fc <andw_ax_dx+0xc> 376f8: R_X86_64_PLT32 __x86_return_thunk-0x4
> 376fc: 0f 1f 40 00 nopl 0x0(%rax)
>
> However, the Code: thing above gives:
>
> 2a:* 66 0f 1f 00 nopw (%rax) <-- trapping instruction
> 2e: 66 21 d0 and %dx,%ax
> 31: c3 ret
> 32: cc int3
> 33: cc int3
> 34: cc int3
> 35: cc int3
> 36: 0f 1f 40 00 nopl 0x0(%rax)
>
> From that we can tell gen_endbr_poison() has been used to scribble the
> endbr -- so clearly objtool didn't manage to find code references here.
>
> And indeed, kvm.ko's .rela.ibt_endbr_seal section includes:
>
> 00000000000002c0 0000000100000002 R_X86_64_PC32 0000000000000000 .text + 376f0
>
> ---
> Subject: x86/kvm, objtool: Avoid fastop ENDBR from being sealed
>
> Xu reported a number of "Missing ENDBR" triggers for the KVM fastop
> emulation code. It turns out that because of how the fastops are set up,
> only the first of a series -- the 8 byte variants that overlap with the
> em_ symbols -- is found referenced.
>
> Specifically:
>
> .pushsection .text, "ax"
> .global em_and
> .align 16
> em_and:
> .align 16
> .type andb_al_dl, @function
> andb_al_dl:
> endbr64
> andb %dl, %al
> 11: jmp __x86_return_thunk
> .size andb_al_dl, .-andb_al_dl
> .align 16
> .type andw_ax_dx, @function
> andw_ax_dx:
> endbr64
> andw %dx, %ax
> 11: jmp __x86_return_thunk
> .size andw_ax_dx, .-andw_ax_dx
> .align 16
> .type andl_eax_edx, @function
> andl_eax_edx:
> endbr64
> andl %edx, %eax
> 11: jmp __x86_return_thunk
> .size andl_eax_edx, .-andl_eax_edx
> .align 16
> .type andq_rax_rdx, @function
> andq_rax_rdx:
> endbr64
> andq %rdx, %rax
> 11: jmp __x86_return_thunk
> .size andq_rax_rdx, .-andq_rax_rdx
> .popsection
>
> Only has the em_and symbol referenced, resulting in and{w,l,q}_* getting
> sealed.
>
> Add (yet another) annotation to inhibit objtool from sealing a specific
> ENDBR instance.
>
> Fixes: 6649fa876da4 ("x86/ibt,kvm: Add ENDBR to fastops")
> Reported-by: "Xu, Pengfei" <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> arch/x86/include/asm/ibt.h | 5 +++++
> arch/x86/kvm/emulate.c | 4 ++--
> include/linux/objtool.h | 6 ++++++
> tools/objtool/check.c | 44 ++++++++++++++++++++++++++++++++++++++------
> 4 files changed, 51 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
> index 689880eca9ba..f32ba1c4e619 100644
> --- a/arch/x86/include/asm/ibt.h
> +++ b/arch/x86/include/asm/ibt.h
> @@ -29,6 +29,10 @@
> #define ASM_ENDBR "endbr32\n\t"
> #endif
>
> +#define ASM_ENDBR_NOSEAL \
> + ANNOTATE_NOSEAL \
> + ASM_ENDBR
> +
> #define __noendbr __attribute__((nocf_check))
>
> static inline __attribute_const__ u32 gen_endbr(void)
> @@ -84,6 +88,7 @@ extern __noendbr void ibt_restore(u64 save);
> #ifndef __ASSEMBLY__
>
> #define ASM_ENDBR
> +#define ASM_ENDBR_NOSEAL
>
> #define __noendbr
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index b4eeb7c75dfa..d51ee8a3bcae 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -326,7 +326,7 @@ 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_NOSEAL
>
> #define FOP_FUNC(name) \
> __FOP_FUNC(#name)
> @@ -461,7 +461,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
> ".align " __stringify(SETCC_ALIGN) " \n\t" \
> ".type " #op ", @function \n\t" \
> #op ": \n\t" \
> - ASM_ENDBR \
> + ASM_ENDBR_NOSEAL \
> #op " %al \n\t" \
> __FOP_RET(#op) \
> ".skip " __stringify(SETCC_ALIGN) " - (.-" #op "), 0xcc \n\t"
> diff --git a/include/linux/objtool.h b/include/linux/objtool.h
> index 62c54ffbeeaa..ad752f8b3b36 100644
> --- a/include/linux/objtool.h
> +++ b/include/linux/objtool.h
> @@ -90,6 +90,12 @@ struct unwind_hint {
> _ASM_PTR " 986b\n\t" \
> ".popsection\n\t"
>
> +#define ANNOTATE_NOSEAL \
> + "986: \n\t" \
> + ".pushsection .discard.noseal\n\t" \
> + _ASM_PTR " 986b\n\t" \
> + ".popsection\n\t"
> +
> #define ASM_REACHABLE \
> "998:\n\t" \
> ".pushsection .discard.reachable\n\t" \
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0cec74da7ffe..0d04d0a707f4 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2111,6 +2111,40 @@ static int read_noendbr_hints(struct objtool_file *file)
> return 0;
> }
>
> +static void mark_endbr_used(struct instruction *insn)
> +{
> + if (!list_empty(&insn->call_node))
> + list_del_init(&insn->call_node);
> +}
> +
> +static int read_noseal_hints(struct objtool_file *file)
> +{
> + struct section *sec;
> + struct instruction *insn;
> + struct reloc *reloc;
> +
> + sec = find_section_by_name(file->elf, ".rela.discard.noseal");
> + if (!sec)
> + return 0;
> +
> + list_for_each_entry(reloc, &sec->reloc_list, list) {
> + insn = find_insn(file, reloc->sym->sec, reloc->sym->offset + reloc->addend);
> + if (!insn) {
> + WARN("bad .discard.noseal entry");
> + return -1;
> + }
> +
> + if (insn->type != INSN_ENDBR) {
> + WARN_FUNC("ANNOTATE_NOSEAL not on ENDBR", insn->sec, insn->offset);
> + continue;
> + }
> +
> + mark_endbr_used(insn);
> + }
> +
> + return 0;
> +}
> +
> static int read_retpoline_hints(struct objtool_file *file)
> {
> struct section *sec;
> @@ -2356,6 +2390,10 @@ static int decode_sections(struct objtool_file *file)
> if (ret)
> return ret;
>
> + ret = read_noseal_hints(file);
> + if (ret)
> + return ret;
> +
> /*
> * Must be before add_{jump_call}_destination.
> */
> @@ -3952,12 +3990,6 @@ static int validate_functions(struct objtool_file *file)
> return warnings;
> }
>
> -static void mark_endbr_used(struct instruction *insn)
> -{
> - if (!list_empty(&insn->call_node))
> - list_del_init(&insn->call_node);
> -}
> -
> static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
> {
> struct instruction *dest;
On Wed, Aug 17, 2022 at 06:10:45PM -0700, Josh Poimboeuf wrote:
> On Wed, Aug 17, 2022 at 12:56:15PM +0200, Peter Zijlstra wrote:
> > Add (yet another) annotation to inhibit objtool from sealing a specific
> > ENDBR instance.
>
> Nah, just add a throwaway reference to the function. Then no objtool
> change is needed:
Nice!, should I 'borrow' this and respin the patch or you want to make a
real patch out of it?
> diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
> index 689880eca9ba..3ae795150374 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.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/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f8382abe22ff..88623add0db4 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -316,7 +316,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)
Hi Poimboeuf and Peter,
I installed the new patch based on v5.19 mainline kernel.
And this "Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]" issue was
reproduced again after ran syzkaller 5min on ADL-P.
I'm not sure is there something missed for attached patch.
Patch, kvm.ko and dmesg are in attached.
Thanks!
BR.
On 2022-08-18 at 09:28:28 +0200, Peter Zijlstra wrote:
> On Wed, Aug 17, 2022 at 06:10:45PM -0700, Josh Poimboeuf wrote:
> > On Wed, Aug 17, 2022 at 12:56:15PM +0200, Peter Zijlstra wrote:
> > > Add (yet another) annotation to inhibit objtool from sealing a specific
> > > ENDBR instance.
> >
> > Nah, just add a throwaway reference to the function. Then no objtool
> > change is needed:
>
> Nice!, should I 'borrow' this and respin the patch or you want to make a
> real patch out of it?
>
> > diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
> > index 689880eca9ba..3ae795150374 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.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/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index f8382abe22ff..88623add0db4 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -316,7 +316,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)
On Thu, Aug 18, 2022 at 03:38:45PM +0800, Pengfei Xu wrote:
> And this "Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]" issue was
> reproduced again after ran syzkaller 5min on ADL-P.
Ha, indeed :/
> > > +/*
> > > + * 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.endbr.noseal\n\t" \
> > > + _ASM_PTR fname "\n\t" \
> > > + ".popsection\n\t"
> > > +
objtool/check.c:validate_ibt():
if (!strncmp(sec->name, ".discard", 8) ||
...)
continue;
So yeah, that'll just get ignored. So at the very least we need to
special case this section.
On Thu, Aug 18, 2022 at 01:06:16PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 18, 2022 at 03:38:45PM +0800, Pengfei Xu wrote:
> > And this "Missing ENDBR: andw_ax_dx+0x0/0x10 [kvm]" issue was
> > reproduced again after ran syzkaller 5min on ADL-P.
>
> Ha, indeed :/
>
> > > > +/*
> > > > + * 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.endbr.noseal\n\t" \
> > > > + _ASM_PTR fname "\n\t" \
> > > > + ".popsection\n\t"
> > > > +
>
> objtool/check.c:validate_ibt():
>
> if (!strncmp(sec->name, ".discard", 8) ||
> ...)
> continue;
>
> So yeah, that'll just get ignored. So at the very least we need to
> special case this section.
Indeed. I swear that patch worked last night.
I also found that not all fastops use __FOP_FUNC().
Anyway I'll post a new (official) pair of patches shortly.
--
Josh