> From: Bjoern Doebel <doe...@amazon.de>
> Sent: Tuesday, March 8, 2022 10:29 AM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: Michael Kurth <m...@amazon.de>; Martin Pohlack <mpohl...@amazon.de>;
> Roger Pau Monne <roger....@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Bjoern Doebel <doe...@amazon.de>; Konrad
> Rzeszutek Wilk <konrad.w...@oracle.com>; Ross Lagerwall
> <ross.lagerw...@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 <doe...@amazon.de>
> CC: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> CC: Ross Lagerwall <ross.lagerw...@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?
