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

From: "Doebel, Bjoern" <doebel@amazon.de>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Cc: "Jan Beulich" <JBeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>
Subject: Re: [PATCH] x86/cet: Use dedicated NOP4 for cf_clobber
Date: Thu, 10 Mar 2022 08:30:33 +0100	[thread overview]
Message-ID: <1c4fa029-44a9-1e0a-bb09-458ad8d28a64@amazon.de> (raw)
In-Reply-To: <20220308140126.8815-1-andrew.cooper3@citrix.com>

On 08.03.22 15:01, Andrew Cooper wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> 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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Bjoern Doebel <doebel@amazon.de>
> CC: Michael Kurth <mku@amazon.de>
> CC: Martin Pohlack <mpohlack@amazon.de>
> 
> Bjoern: For the livepatching code, I think you want:
> 
>    if ( is_endbr64(...) || was_endbr64(...) )
>        needed += ENDBR64_LEN;
> ---
>   xen/arch/x86/alternative.c       | 10 +++++++++-
>   xen/arch/x86/include/asm/endbr.h | 12 ++++++++++++
>   2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index d41eeef1bcaf..ffb1b1d960c8 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -362,7 +362,15 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>               if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
>                   continue;
> 
> -            add_nops(ptr, ENDBR64_LEN);
> +            /*
> +             * Can't use add_nops() here.  ENDBR64_POISON is specifically
> +             * different to NOP4 so it can be spotted after the fact.
> +             *
> +             * All CET-capable hardware uses P6 NOPS (no need to plumb through
> +             * ideal_nops), and doesn't require a branch to synchronise the
> +             * instruction stream.
> +             */
> +            memcpy(ptr, ENDBR64_POISON, ENDBR64_LEN);
>               clobbered++;
>           }
> 
> diff --git a/xen/arch/x86/include/asm/endbr.h b/xen/arch/x86/include/asm/endbr.h
> index 6090afeb0bd8..5e1e55cb467d 100644
> --- 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) */
> +
> +static inline bool was_endbr64(const void *ptr)
> +{
> +    return *(const uint32_t *)ptr == 0x001f0f66;
> +}
> +
>   #endif /* XEN_ASM_ENDBR_H */
> --
> 2.11.0

Reviewed-by: Bjoern Doebel <doebel@amazon.de>

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879