Re: [PATCH v3 2/2] xen/x86: Livepatch: support patching CET-enhanced functions

[Ross Lagerwall <ross.lagerwall@citrix.com>]

> From: Bjoern Doebel <doebel@amazon.de>
> Sent: Tuesday, March 8, 2022 10:29 AM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: Michael Kurth <mku@amazon.de>; Martin Pohlack <mpohlack@amazon.de>; Roger Pau Monne <roger.pau@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Bjoern Doebel <doebel@amazon.de>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com>
> Subject: [PATCH v3 2/2] xen/x86: Livepatch: support patching CET-enhanced functions 
>  
> Xen enabled CET for supporting architectures. The control flow aspect of
> CET expects functions that can be called indirectly (i.e., via function
> pointers) to start with an ENDBR64 instruction. Otherwise a control flow
> exception is raised.
> 
> This expectation breaks livepatching flows because we patch functions by
> overwriting their first 5 bytes with a JMP + <offset>, thus breaking the
> ENDBR64. We fix this by checking the start of a patched function for
> being ENDBR64. In the positive case we move the livepatch JMP to start
> behind the ENDBR64 instruction.
> 
> To avoid having to guess the ENDBR64 offset again on patch reversal
> (which might race with other mechanisms adding/removing ENDBR
> dynamically), use the livepatch metadata to store the computed offset
> along with the saved bytes of the overwritten function.
> 
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> ----
> Note that on top of livepatching functions, Xen supports an additional
> mode where we can "remove" a function by overwriting it with NOPs. This
> is only supported for functions up to 31 bytes in size and this patch
> reduces this limit to 30 bytes.
> 
> Changes since r1:
> * use sizeof_field() to avoid unused variable warning
> * make metadata variable const in arch_livepatch_revert
> ---
>  xen/arch/x86/livepatch.c | 61 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 65530c1e57..0fd97f2a00 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -14,11 +14,29 @@
>  #include <xen/vm_event.h>
>  #include <xen/virtual_region.h>
>  
> +#include <asm/endbr.h>
>  #include <asm/fixmap.h>
>  #include <asm/nmi.h>
>  #include <asm/livepatch.h>
>  #include <asm/setup.h>
>  
> +/*
> + * CET hotpatching support: We may have functions starting with an ENDBR64
> + * instruction that MUST remain the first instruction of the function, hence
> + * we need to move any hotpatch trampoline further into the function. For that
> + * we need to keep track of the patching offset used for any loaded hotpatch
> + * (to avoid racing against other fixups adding/removing ENDBR64 or similar
> + * instructions).
> + *
> + * We do so by making use of the existing opaque metadata area. We use its
> + * first 4 bytes to track the offset into the function used for patching and
> + * the remainder of the data to store overwritten code bytes.
> + */
> +struct x86_livepatch_meta {
> +    uint8_t patch_offset;
> +    uint8_t instruction[LIVEPATCH_OPAQUE_SIZE - sizeof(uint8_t)];
> +};
> +

I think it would make things a bit simpler to use one of the spare _pad bits
from struct livepatch_func  ather than hacking it into the opaque area. Is
there a reason you chose this approach?