[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Fri, 11 Mar 2022 08:18:21 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=SyUJhWfsSyqUUHT6uq6nwQbPw81yxNi/kMNNb7x9pH8=; b=H9XVIacNIsM11ofWa/Aa4QP9WbEv9Lx3TxfxoWxetBTBtyvWJLFskX5J5zZcjV8kettCJI3OUyA1Bfqm2MvQamSLWbzesBJ64dI9j4kgaUGtpNQc1Ptonj2XmNA1ad7LlhYfJ1jpKFFtGDEKNPIMdzmHR1SERQewGqMAxYFG4FMqKI/LhwZ4WHIUKAKL2k/8tnFlr3M6nEeLtuBc3Qx2uqYoM0cJuOtz/IYDYF2vSDFkS8DFeVTOiWXrEqmX7mzDOaguVYd1p8k9KhNfGyhAPiqYDyGWrOjFdC+YoO1YJet6aGldeoF+v+z0Pae+NRmmFiAhEdZr00y6T2l25gb+zw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=RVxBC7Cd6/2/EFBkSQS66Y/QUDvwcnrInCojewzqrpjhQHnevPGvF+OAmlCW1mOzdJdf8S/uqllPBTG5SxlFJejrQ9QFtlooqVlFo7wmASIfjTWAbTMNP2g8uRkpsGvEPCZYpjyETWOmNE11wDrVhAXgsxs+hJNrIE+idBhx9xp/qSYMr1WfitoKT4nCMZKHhEZidSxZEpOgzDNfSq5jXzXScyh/44hR3T7MzqgJMGiBcTfdxyEk05W7K6O2wwUh9VZUn/n2WgUHqo7eP54o7dndsIyTafgSlEUIVT8W/4+mkrN1d8N1wjdV2Z6t2TGBdw+RymDjLAxgtA2frJuw5Q==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Bjoern Doebel <doebel@xxxxxxxxx>, Michael Kurth <mku@xxxxxxxxx>, Martin Pohlack <mpohlack@xxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 11 Mar 2022 07:18:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.