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

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>, Wei Liu <wl@xen.org>,
	"Bjoern Doebel" <doebel@amazon.de>, Michael Kurth <mku@amazon.de>,
	Martin Pohlack <mpohlack@amazon.de>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] x86/cet: Use dedicated NOP4 for cf_clobber
Date: Thu, 10 Mar 2022 18:42:07 +0000	[thread overview]
Message-ID: <d65a64fe-72b1-efc5-1804-8f74aca6d803@citrix.com> (raw)
In-Reply-To: <48c6720a-070e-85db-f1c3-448714232946@citrix.com>

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@suse.com>
> 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?

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, 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?

~Andrew
