Re: [PATCH] x86/cet: Use dedicated NOP4 for cf_clobber

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2022 19:42, Andrew Cooper wrote:
> On 08/03/2022 16:03, Andrew Cooper wrote:
>>>>>> --- a/xen/arch/x86/include/asm/endbr.h
>>>>>> +++ b/xen/arch/x86/include/asm/endbr.h
>>>>>> @@ -52,4 +52,16 @@ static inline void place_endbr64(void *ptr)
>>>>>>      *(uint32_t *)ptr = gen_endbr64();
>>>>>>  }
>>>>>>  
>>>>>> +/*
>>>>>> + * After clobbering ENDBR64, we may need to confirm that the site used 
>>>>>> to
>>>>>> + * contain an ENDBR64 instruction.  Use an encoding which isn't the 
>>>>>> default
>>>>>> + * P6_NOP4.
>>>>>> + */
>>>>>> +#define ENDBR64_POISON "\x66\x0f\x1f\x00" /* osp nopl (%rax) */
>>>>> In case this remains as is - did you mean "opsz" instead of "osp"?
>>>>> But this really is "nopw (%rax)" anyway.
>>>> Oh, osp is the nasm name.  I'll switch to nopw.
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Thanks.
> 
> It does occur to me that we can extend check-endbr.sh for this.
> 
> diff --git a/xen/arch/x86/indirect-thunk.S b/xen/arch/x86/indirect-thunk.S
> index 7cc22da0ef93..3baaf7ab4983 100644
> --- a/xen/arch/x86/indirect-thunk.S
> +++ b/xen/arch/x86/indirect-thunk.S
> @@ -38,6 +38,7 @@
>          .section .text.__x86_indirect_thunk_\reg, "ax", @progbits
>  
>  ENTRY(__x86_indirect_thunk_\reg)
> +       nopw (%rax)
>          ALTERNATIVE_2 __stringify(IND_THUNK_RETPOLINE \reg),              \
>          __stringify(IND_THUNK_LFENCE \reg), X86_FEATURE_IND_THUNK_LFENCE, \
>          __stringify(IND_THUNK_JMP \reg),    X86_FEATURE_IND_THUNK_JMP
> diff --git a/xen/tools/check-endbr.sh b/xen/tools/check-endbr.sh
> index 9799c451a18d..652ac8d0b983 100755
> --- a/xen/tools/check-endbr.sh
> +++ b/xen/tools/check-endbr.sh
> @@ -67,7 +67,7 @@ eval $(${OBJDUMP} -j .text $1 -h |
>  ${OBJCOPY} -j .text $1 -O binary $TEXT_BIN
>  if $perl_re
>  then
> -    LC_ALL=C grep -aobP '\363\17\36\372' $TEXT_BIN
> +    LC_ALL=C grep -aobP '\363\17\36\372|\x66\x0f\x1f\x00' $TEXT_BIN
>  else
>      grep -aob "$(printf '\363\17\36\372')" $TEXT_BIN
>  fi | awk -F':' '{printf "%s%x\n", "'$vma_hi'", int(0x'$vma_lo') + $1}'
>> $ALL
> 
> yields:
> 
> check-endbr.sh xen-syms Fail: Found 15 embedded endbr64 instructions
> 0xffff82d040377f00: __x86_indirect_thunk_rax at
> /local/xen.git/xen/arch/x86/indirect-thunk.S:55
> 0xffff82d040377f20: __x86_indirect_thunk_rcx at ??:?
> 0xffff82d040377f40: __x86_indirect_thunk_rdx at ??:?
> 0xffff82d040377f60: __x86_indirect_thunk_rbx at ??:?
> 0xffff82d040377f80: __x86_indirect_thunk_rbp at ??:?
> 0xffff82d040377fa0: __x86_indirect_thunk_rsi at ??:?
> 0xffff82d040377fc0: __x86_indirect_thunk_rdi at ??:?
> 0xffff82d040377fe0: __x86_indirect_thunk_r8 at ??:?
> 0xffff82d040378000: __x86_indirect_thunk_r9 at ??:?
> 0xffff82d040378020: __x86_indirect_thunk_r10 at ??:?
> 0xffff82d040378040: __x86_indirect_thunk_r11 at ??:?
> 0xffff82d040378060: __x86_indirect_thunk_r12 at ??:?
> 0xffff82d040378080: __x86_indirect_thunk_r13 at ??:?
> 0xffff82d0403780a0: __x86_indirect_thunk_r14 at ??:?
> 0xffff82d0403780c0: __x86_indirect_thunk_r15 at ??:?
> ...
> check-endbr.sh xen.efi Fail: Found 15 embedded endbr64 instructions
> 0xffff82d040377f00: ?? at /local/xen.git/xen/arch/x86/indirect-thunk.S:55
> 0xffff82d040377f20: ?? at head.o:?
> 0xffff82d040377f40: ?? at head.o:?
> 0xffff82d040377f60: ?? at head.o:?
> 0xffff82d040377f80: ?? at head.o:?
> 0xffff82d040377fa0: ?? at head.o:?
> 0xffff82d040377fc0: ?? at head.o:?
> 0xffff82d040377fe0: ?? at head.o:?
> 0xffff82d040378000: ?? at head.o:?
> 0xffff82d040378020: ?? at head.o:?
> 0xffff82d040378040: ?? at head.o:?
> 0xffff82d040378060: ?? at head.o:?
> 0xffff82d040378080: ?? at head.o:?
> 0xffff82d0403780a0: ?? at head.o:?
> 0xffff82d0403780c0: ?? at head.o:?
> 
> Obviously the changes to check-endbr want cleaning up, but I think it's
> entirely within scope to check for ENDBR64_POISON too, and we can do it
> without adding an extra pass.  Would you be happier with this check added?

Yes, this would feel better. Thanks for having continued to think
about it.

> But we also have some clear errors with debug symbols.  It's perhaps not
> terribly surprising that irp/endr only gets file/line for the first
> instance,

I have to admit I would expect it to at least figure the file. But
there's no .debug_line contents at all for ..._rcx .. ..._r15.

> and at least ELF manage to get the function name right, but
> EFI is a mess and manages to get the wrong file.  Any idea how to get
> rather less nonsense out of the debug symbols?

A random example with a symbol from a C file works here, at least.

Jan