Re: [PATCH] x86/kvm, objtool: Avoid fastop ENDBR from being sealed

[Pengfei Xu <pengfei.xu@intel.com>]

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" <pengfei.xu@intel.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  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;