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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2022 15:01, Andrew Cooper wrote:
> For livepatching, we need to look at a potentially clobbered function and
> determine whether it used to have an ENDBR64 instruction.
> 
> Use a non-default 4-byte P6 long nop, not emitted by toolchains, and introduce
> the was_endbr64() predicate.

Did you consider using ENDBR32 for this purpose? I'm worried that
the pattern you picked is still too close to what might be used
(down the road) by a tool chain. If ENDBR32 isn't suitable for
some reason, how about "nop %sp" or "nopw (%rsp)" (and then maybe
even "data16" substituted by rex, cs, ds, es, or ss)?

One neat thing about ENDBR32 would be that you wouldn't even
need memcpy() - you'd merely flip the low main opcode bit.

> Bjoern: For the livepatching code, I think you want:
> 
>   if ( is_endbr64(...) || was_endbr64(...) )
>       needed += ENDBR64_LEN;

Looks like I didn't fully understand the problem then from your
initial description. The adjustment here (and the one needed in
Björn's patch) is to compensate for the advancing of the
targets of altcalls past the ENDBR?

> --- 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.

Jan