[PATCH 0/2] Livepatch: support for livepatching CET functions

[PATCH 0/2] Livepatch: support for livepatching CET functions

[Bjoern Doebel]

This series enables support for livepatching functions compiled with
Intel CET support where we have to make sure we leave a potential
leading ENDBR64 instruction intact on livepatch application.

The series also requires a change to generic livepatching code: we need
to make sure that the address of the patched function is known at the
time Xen tries to verify the livepatch for applicability, hence we need
to resolve this address before verifying the patch.

Bjoern Doebel (2):
  Livepatch: resolve old address before function verification
  xen/x86: Livepatch: support patching CET-enhanced functions

 xen/arch/x86/livepatch.c | 63 +++++++++++++++++++++++++++++++++++-----
 xen/common/livepatch.c   |  4 +--
 2 files changed, 57 insertions(+), 10 deletions(-)

-- 
2.32.0




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

[PATCH 1/2] Livepatch: resolve old address before function verification

[Bjoern Doebel]

When verifying that a livepatch can be applied, we may as well want to
inspect the target function to be patched. To do so, we need to resolve
this function's address before running the arch-specific
livepatch_verify hook.

Signed-off-by: Bjoern Doebel <doebel@amazon.de>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/common/livepatch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index ec301a9f12..be2cf75c2d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -684,11 +684,11 @@ static int prepare_payload(struct payload *payload,
                 return -EINVAL;
             }
 
-            rc = arch_livepatch_verify_func(f);
+            rc = resolve_old_address(f, elf);
             if ( rc )
                 return rc;
 
-            rc = resolve_old_address(f, elf);
+            rc = arch_livepatch_verify_func(f);
             if ( rc )
                 return rc;
 
-- 
2.32.0




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

Re: [PATCH 1/2] Livepatch: resolve old address before function verification

[Ross Lagerwall]

> From: Bjoern Doebel <doebel@amazon.de>
> Sent: Monday, March 7, 2022 11:53 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 1/2] Livepatch: resolve old address before function verification 
>  
> When verifying that a livepatch can be applied, we may as well want to
> inspect the target function to be patched. To do so, we need to resolve
> this function's address before running the arch-specific
> livepatch_verify hook.
> 
> Signed-off-by: Bjoern Doebel <doebel@amazon.de>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/common/livepatch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index ec301a9f12..be2cf75c2d 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -684,11 +684,11 @@ static int prepare_payload(struct payload *payload,
>                  return -EINVAL;
>              }
>  
> -            rc = arch_livepatch_verify_func(f);
> +            rc = resolve_old_address(f, elf);
>              if ( rc )
>                  return rc;
>  
> -            rc = resolve_old_address(f, elf);
> +            rc = arch_livepatch_verify_func(f);
>              if ( rc )
>                  return rc;
>  
> -- 
> 2.32.0

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

[Bjoern Doebel]

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.
---
 xen/arch/x86/livepatch.c | 63 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 65530c1e57..da7611c01d 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)];
+};
+
 static bool has_active_waitqueue(const struct vm_event_domain *ved)
 {
     /* ved may be xzalloc()'d without INIT_LIST_HEAD() yet. */
@@ -104,18 +122,36 @@ 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;
+
+        lp = (struct x86_livepatch_meta *)func->opaque;
         /* Only do up to maximum amount we can put in the ->opaque. */
-        if ( func->new_size > sizeof(func->opaque) )
+        if ( func->new_size > sizeof(lp->instruction) )
             return -EOPNOTSUPP;
 
         if ( func->old_size < func->new_size )
             return -EINVAL;
     }
-    else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
-        return -EINVAL;
+    else
+    {
+        /*
+         * Space needed now depends on whether the target function
+         * starts with an ENDBR64 instruction.
+         */
+        uint8_t needed;
+
+        needed = ARCH_PATCH_INSN_SIZE;
+        if ( is_endbr64(func->old_addr) )
+            needed += ENDBR64_LEN;
+
+        if ( func->old_size < needed )
+            return -EINVAL;
+    }
 
     return 0;
 }
@@ -127,15 +163,21 @@ 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;
+    uint8_t insn[sizeof(lp->instruction)];
     unsigned int len;
 
+    lp = (struct x86_livepatch_meta *)func->opaque;
+    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) )
+        lp->patch_offset += ENDBR64_LEN;
+
+    memcpy(lp->instruction, old_ptr + lp->patch_offset, len);
     if ( func->new_addr )
     {
         int32_t val;
@@ -143,14 +185,15 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
         BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
 
         insn[0] = 0xe9; /* Relative jump. */
-        val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
+        val = func->new_addr - (func->old_addr + lp->patch_offset
+                                + ARCH_PATCH_INSN_SIZE);
 
         memcpy(&insn[1], &val, sizeof(val));
     }
     else
         add_nops(insn, len);
 
-    memcpy(old_ptr, insn, len);
+    memcpy(old_ptr + lp->patch_offset, insn, len);
 }
 
 /*
@@ -159,7 +202,11 @@ 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;
+
+    lp = (struct x86_livepatch_meta *)func->opaque;
+
+    memcpy(func->old_addr + lp->patch_offset, lp->instruction, livepatch_insn_len(func));
 }
 
 /*
-- 
2.32.0




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

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

[Jan Beulich]

On 07.03.2022 12:53, Bjoern Doebel wrote:
> @@ -104,18 +122,36 @@ 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;
> +
> +        lp = (struct x86_livepatch_meta *)func->opaque;
>          /* Only do up to maximum amount we can put in the ->opaque. */
> -        if ( func->new_size > sizeof(func->opaque) )
> +        if ( func->new_size > sizeof(lp->instruction) )
>              return -EOPNOTSUPP;
>  
>          if ( func->old_size < func->new_size )
>              return -EINVAL;
>      }

I continue to be concerned of the new local variable causing compiler
warnings. With the adjustment made compared to v1, the specific
warning would have changed, and we're now liable to see set-but-never-
used ones.

Taking of versions - please tag your patches accordingly, and please
have, in each patch, a short summary of what has changed from the last
version.

> @@ -159,7 +202,11 @@ 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;

As before - const please (wherever possible).

Jan

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

[David Vrabel]

On 07/03/2022 14:03, Jan Beulich wrote:
> On 07.03.2022 12:53, Bjoern Doebel wrote:
>> @@ -104,18 +122,36 @@ 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;
>> +
>> +        lp = (struct x86_livepatch_meta *)func->opaque;
>>           /* Only do up to maximum amount we can put in the ->opaque. */
>> -        if ( func->new_size > sizeof(func->opaque) )
>> +        if ( func->new_size > sizeof(lp->instruction) )
>>               return -EOPNOTSUPP;
>>   
>>           if ( func->old_size < func->new_size )
>>               return -EINVAL;
>>       }
> 
> I continue to be concerned of the new local variable causing compiler
> warnings. With the adjustment made compared to v1, the specific
> warning would have changed, and we're now liable to see set-but-never-
> used ones.

Linux has a sizeof_field() macro for this sort of use.

     /**
      * sizeof_field() - Report the size of a struct field in bytes
      *
      * @TYPE: The structure containing the field of interest
      * @MEMBER: The field to return the size of
      */
     #define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))

David

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

[Jan Beulich]

On 07.03.2022 15:21, David Vrabel wrote:
> On 07/03/2022 14:03, Jan Beulich wrote:
>> On 07.03.2022 12:53, Bjoern Doebel wrote:
>>> @@ -104,18 +122,36 @@ 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;
>>> +
>>> +        lp = (struct x86_livepatch_meta *)func->opaque;
>>>           /* Only do up to maximum amount we can put in the ->opaque. */
>>> -        if ( func->new_size > sizeof(func->opaque) )
>>> +        if ( func->new_size > sizeof(lp->instruction) )
>>>               return -EOPNOTSUPP;
>>>   
>>>           if ( func->old_size < func->new_size )
>>>               return -EINVAL;
>>>       }
>>
>> I continue to be concerned of the new local variable causing compiler
>> warnings. With the adjustment made compared to v1, the specific
>> warning would have changed, and we're now liable to see set-but-never-
>> used ones.
> 
> Linux has a sizeof_field() macro for this sort of use.
> 
>      /**
>       * sizeof_field() - Report the size of a struct field in bytes
>       *
>       * @TYPE: The structure containing the field of interest
>       * @MEMBER: The field to return the size of
>       */
>      #define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))

Oh, I should have thought of this. Iirc it was Paul who did pull in
this one, so it should be readily available.

Thanks for pointing out,
Jan