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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2022 10:17, Bjoern Doebel wrote:
> 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@xxxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> 
> 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 27 bytes.

Is this intended to be part of the description (then it should move up)
or a post-commit-message remark (then there should be a --- separator
ahead of it)?

> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -14,11 +14,28 @@
>  #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.
> + */

Style: Line length (also elsewhere).

> +struct __packed x86_livepatch_meta {
> +    int32_t patch_offset;

I was first going to suggest to use plain int here to comply with
./CODING_STYLE, but it looks like int8_t or uint8_t will do, leaving
more space for the insn. I'm also not convinced you really need the
__packed annotation. Furthermore, to avoid the need for casts,
simply using the ->opaque[] directly would look to be an option then
(with suitable #define-s to identify the two parts).

> @@ -104,11 +121,14 @@ void noinline arch_livepatch_revive(void)
>  
>  int arch_livepatch_verify_func(const struct livepatch_func *func)
>  {
> +    BUILD_BUG_ON(sizeof(struct x86_livepatch_meta) != LIVEPATCH_OPAQUE_SIZE);
> +
>      /* If NOPing.. */
>      if ( !func->new_addr )
>      {
> +        struct x86_livepatch_meta *lp = (struct 
> x86_livepatch_meta*)func->opaque;

_If_ there is to remain a struct, please insert the missing blank before
the star (applicable elsewhere as well), and please separate declaration(s)
from statement(s) by a blank line.

> -        if ( func->new_size > sizeof(func->opaque) )
> +        if ( func->new_size > sizeof(lp->instruction) )

This being the only use of the new variable, I expect compilers to warn
about the variable being actually unused.


> @@ -127,15 +147,20 @@ int arch_livepatch_verify_func(const struct 
> livepatch_func *func)
>  void noinline arch_livepatch_apply(struct livepatch_func *func)
>  {
>      uint8_t *old_ptr;
> -    uint8_t insn[sizeof(func->opaque)];
> +    struct x86_livepatch_meta *lp = (struct x86_livepatch_meta*)func->opaque;
> +    uint8_t insn[sizeof(lp->instruction)];
>      unsigned int len;
>  
> +    lp->patch_offset = 0;
>      old_ptr = func->old_addr;
>      len = livepatch_insn_len(func);
>      if ( !len )
>          return;
>  
> -    memcpy(func->opaque, old_ptr, len);
> +    if ( is_endbr64( old_ptr ) )

Style: No blanks inside the inner parentheses, please.

> @@ -159,7 +184,9 @@ void noinline arch_livepatch_apply(struct livepatch_func 
> *func)
>   */
>  void noinline arch_livepatch_revert(const struct livepatch_func *func)
>  {
> -    memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
> +    struct x86_livepatch_meta *lp = (struct x86_livepatch_meta*)func->opaque;

const (if the variable was to remain in the first place).

Jan

> +    memcpy(func->old_addr + lp->patch_offset, lp->instruction, 
> livepatch_insn_len(func));
>  }
>  
>  /*