xen/x86: Livepatch: support patching CET-enhanced functions

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

Posted by Bjoern Doebel 2 years, 1 month ago

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 27 bytes.
---
 xen/arch/x86/livepatch.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 65530c1e57..bb7ecf36a6 100644
--- 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.
+ */
+struct __packed x86_livepatch_meta {
+    int32_t patch_offset;
+    uint8_t instruction[LIVEPATCH_OPAQUE_SIZE - sizeof(int32_t)];
+};
+
 static bool has_active_waitqueue(const struct vm_event_domain *ved)
 {
     /* ved may be xzalloc()'d without INIT_LIST_HEAD() yet. */
@@ -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;
         /* 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 )
@@ -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 ) )
+        lp->patch_offset += ENDBR64_LEN;
+
+    memcpy(lp->instruction, old_ptr + lp->patch_offset, len);
     if ( func->new_addr )
     {
         int32_t val;
@@ -143,14 +168,14 @@ 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 +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;
+
+    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] xen/x86: Livepatch: support patching CET-enhanced functions

Posted by Jan Beulich 2 years, 1 month ago

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@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 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));
>  }
>  
>  /*

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

Posted by Doebel, Bjoern 2 years, 1 month ago


On 07.03.22 10:37, Jan Beulich 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.
> 
> 
> 
> 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@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 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)?

Missing separator. Will fix as well as your style comments below.

> 
>> --- 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).

* Agree on the uint8_t.
* The __packed was mostly to be really sure there's no added padding.
   Likely will be needed once I change the type for the offset.
* I think using an explicit struct is more readable+comprehensible than
   magically defining macros to reinterpret the opaque[] array, so I'd
   prefer that. No hard feelings, though.

>> @@ -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));
>>   }
>>
>>   /*
> 
> 


Bjoern



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] xen/x86: Livepatch: support patching CET-enhanced functions

Posted by Jan Beulich 2 years, 1 month ago

On 07.03.2022 11:17, Doebel, Bjoern wrote:
> On 07.03.22 10:37, Jan Beulich wrote:
>> On 07.03.2022 10:17, Bjoern Doebel wrote:
>>> --- 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).
> 
> * Agree on the uint8_t.
> * The __packed was mostly to be really sure there's no added padding.
>    Likely will be needed once I change the type for the offset.
> * I think using an explicit struct is more readable+comprehensible than
>    magically defining macros to reinterpret the opaque[] array, so I'd
>    prefer that. No hard feelings, though.

While I agree on the readable/comprehensible aspect, casts - in particular
such as used here - come with a risk. But it'll be up to the maintainers
to judge anyway.

Jan