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

Commit Message

Doebel, Bjoern March 8, 2022, 10:29 a.m. UTC

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 <[e-mail]>
CC: Konrad Rzeszutek Wilk <[e-mail]>
CC: Ross Lagerwall <[e-mail]>
----
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(-)

Comments

Andrew Cooper March 8, 2022, 12:44 p.m. UTC | #1

On 08/03/2022 10:29, Bjoern Doebel wrote:
> @@ -104,18 +122,34 @@ 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 )
>      {
>          /* Only do up to maximum amount we can put in the ->opaque. */
> -        if ( func->new_size > sizeof(func->opaque) )
> +        if ( func->new_size > sizeof_field(struct x86_livepatch_meta,
> +                                           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;

This won't work for cf_clobber targets, I don't think.  The ENDBR gets
converted to NOP4 and fails this check, but the altcalls calling
old_func had their displacements adjusted by +4.

The is_endbr64() check will fail, and the 5-byte jmp will be written at
the start of the function, and corrupt the instruction stream for the
altcall()'d callers.

Let me write an incremental patch to help.

~Andrew

Konrad Rzeszutek Wilk March 8, 2022, 1:06 p.m. UTC | #2

On Tue, Mar 08, 2022 at 12:44:54PM +0000, Andrew Cooper wrote:
> On 08/03/2022 10:29, Bjoern Doebel wrote:
> > @@ -104,18 +122,34 @@ 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 )
> >      {
> >          /* Only do up to maximum amount we can put in the ->opaque. */
> > -        if ( func->new_size > sizeof(func->opaque) )
> > +        if ( func->new_size > sizeof_field(struct x86_livepatch_meta,
> > +                                           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;
> 
> This won't work for cf_clobber targets, I don't think.  The ENDBR gets
> converted to NOP4 and fails this check, but the altcalls calling
> old_func had their displacements adjusted by +4.
> 
> The is_endbr64() check will fail, and the 5-byte jmp will be written at
> the start of the function, and corrupt the instruction stream for the
> altcall()'d callers.
> 
> Let me write an incremental patch to help.

Please add Acked-by: Konrad Rzeszutek Wilk <[e-mail]>
on the patches.

Thank you
> 
> ~Andrew

Doebel, Bjoern March 8, 2022, 1:20 p.m. UTC | #3

On 08.03.22 13:44, Andrew Cooper 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 08/03/2022 10:29, Bjoern Doebel wrote:
>> @@ -104,18 +122,34 @@ 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 )
>>       {
>>           /* Only do up to maximum amount we can put in the ->opaque. */
>> -        if ( func->new_size > sizeof(func->opaque) )
>> +        if ( func->new_size > sizeof_field(struct x86_livepatch_meta,
>> +                                           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;
> 
> This won't work for cf_clobber targets, I don't think.  The ENDBR gets
> converted to NOP4 and fails this check, but the altcalls calling
> old_func had their displacements adjusted by +4.
> 
> The is_endbr64() check will fail, and the 5-byte jmp will be written at
> the start of the function, and corrupt the instruction stream for the
> altcall()'d callers.
> 
> Let me write an incremental patch to help.

Thanks. Will you be adding a

    memcmp(func->old_addr, ideal_nops[4], 4)

or is that once more too naive?

Bjoern

> ~Andrew



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

Doebel, Bjoern March 8, 2022, 1:21 p.m. UTC | #4

On 08.03.22 14:06, Konrad Rzeszutek Wilk 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 Tue, Mar 08, 2022 at 12:44:54PM +0000, Andrew Cooper wrote:
>> On 08/03/2022 10:29, Bjoern Doebel wrote:
>>> @@ -104,18 +122,34 @@ 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 )
>>>       {
>>>           /* Only do up to maximum amount we can put in the ->opaque. */
>>> -        if ( func->new_size > sizeof(func->opaque) )
>>> +        if ( func->new_size > sizeof_field(struct x86_livepatch_meta,
>>> +                                           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;
>>
>> This won't work for cf_clobber targets, I don't think.  The ENDBR gets
>> converted to NOP4 and fails this check, but the altcalls calling
>> old_func had their displacements adjusted by +4.
>>
>> The is_endbr64() check will fail, and the 5-byte jmp will be written at
>> the start of the function, and corrupt the instruction stream for the
>> altcall()'d callers.
>>
>> Let me write an incremental patch to help.
> 
> Please add Acked-by: Konrad Rzeszutek Wilk <[e-mail]>
> on the patches.

Thanks, will do!

Bjoern

> Thank you
>>
>> ~Andrew



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

Ross Lagerwall March 8, 2022, 3:25 p.m. UTC | #5

> From: Bjoern Doebel <[e-mail]>
> Sent: Tuesday, March 8, 2022 10:29 AM
> To: [e-mail] <[e-mail]>
> Cc: Michael Kurth <[e-mail]>; Martin Pohlack <[e-mail]>; Roger Pau Monne <[e-mail]>; Andrew Cooper <[e-mail]>; Bjoern Doebel <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; Ross Lagerwall <[e-mail]>
> 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 <[e-mail]>
> CC: Konrad Rzeszutek Wilk <[e-mail]>
> CC: Ross Lagerwall <[e-mail]>
> ----
> 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?

Doebel, Bjoern March 8, 2022, 3:41 p.m. UTC | #6

On 08.03.22 16:25, Ross Lagerwall 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.
> 
> 
> 
>> From: Bjoern Doebel <[e-mail]>
>> Sent: Tuesday, March 8, 2022 10:29 AM
>> To: [e-mail] <[e-mail]>
>> Cc: Michael Kurth <[e-mail]>; Martin Pohlack <[e-mail]>; Roger Pau Monne <[e-mail]>; Andrew Cooper <[e-mail]>; Bjoern Doebel <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; Ross Lagerwall <[e-mail]>
>> 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 <[e-mail]>
>> CC: Konrad Rzeszutek Wilk <[e-mail]>
>> CC: Ross Lagerwall <[e-mail]>
>> ----
>> 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?

No specific reason. Are you suggesting updating the public livepatch 
interface to add a new member and reduce the padding size by 1 byte? I 
guess that will also require a patch to livepatch-build-tools?

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

Ross Lagerwall March 8, 2022, 4:01 p.m. UTC | #7

> From: Doebel, Bjoern <[e-mail]>
> Sent: Tuesday, March 8, 2022 3:41 PM
> To: Ross Lagerwall <[e-mail]>; [e-mail] <[e-mail]>
> Cc: Michael Kurth <[e-mail]>; Martin Pohlack <[e-mail]>; Roger Pau Monne <[e-mail]>; Andrew Cooper <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>
> Subject: Re: [PATCH v3 2/2] xen/x86: Livepatch: support patching CET-enhanced functions 
>  
> 
> On 08.03.22 16:25, Ross Lagerwall 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.
> > 
> > 
> > 
> >> From: Bjoern Doebel <[e-mail]>
> >> Sent: Tuesday, March 8, 2022 10:29 AM
> >> To: [e-mail] <[e-mail]>
> >> Cc: Michael Kurth <[e-mail]>; Martin Pohlack <[e-mail]>; Roger Pau Monne <[e-mail]>; Andrew Cooper <[e-mail]>; Bjoern Doebel <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; Ross Lagerwall <[e-mail]>
> >> 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 <[e-mail]>
> >> CC: Konrad Rzeszutek Wilk <[e-mail]>
> >> CC: Ross Lagerwall <[e-mail]>
> >> ----
> >> 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?
> 
> No specific reason. Are you suggesting updating the public livepatch 
> interface to add a new member and reduce the padding size by 1 byte? I 
> guess that will also require a patch to livepatch-build-tools?
> 
> Bjoern

struct livepatch_func contains info that the build tool needs to
communicate to Xen as well as space for Xen's internal book keeping
while the live patch is applied. This includes the array for storing
instructions, the applied flag, and now additionally the patch offset.
(It's a somewhat odd arrangement but it's what we've got...)

The build tool does not need to know the details about any of Xen's internal
book keeping. So my preference would be to have patch_offset as an additional
member next to applied (reducing padding by 1) and then in livepatch-build-tools
replace:

        unsigned char pad[MAX_REPLACEMENT_SIZE];
        uint8_t applied;
        uint8_t _pad[7];

with simply:

        uint8_t opaque[39];

		
What do you think?

Ross

Doebel, Bjoern March 8, 2022, 4:54 p.m. UTC | #8

On 08.03.22 17:01, Ross Lagerwall 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.
> 
> 
> 
>> From: Doebel, Bjoern <[e-mail]>
>> Sent: Tuesday, March 8, 2022 3:41 PM
>> To: Ross Lagerwall <[e-mail]>; [e-mail] <[e-mail]>
>> Cc: Michael Kurth <[e-mail]>; Martin Pohlack <[e-mail]>; Roger Pau Monne <[e-mail]>; Andrew Cooper <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>
>> Subject: Re: [PATCH v3 2/2] xen/x86: Livepatch: support patching CET-enhanced functions
>>
>>
>> On 08.03.22 16:25, Ross Lagerwall 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.
>>>
>>>
>>>
>>>> From: Bjoern Doebel <[e-mail]>
>>>> Sent: Tuesday, March 8, 2022 10:29 AM
>>>> To: [e-mail] <[e-mail]>
>>>> Cc: Michael Kurth <[e-mail]>; Martin Pohlack <[e-mail]>; Roger Pau Monne <[e-mail]>; Andrew Cooper <[e-mail]>; Bjoern Doebel <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; Ross Lagerwall <[e-mail]>
>>>> 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 <[e-mail]>
>>>> CC: Konrad Rzeszutek Wilk <[e-mail]>
>>>> CC: Ross Lagerwall <[e-mail]>
>>>> ----
>>>> 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?
>>
>> No specific reason. Are you suggesting updating the public livepatch
>> interface to add a new member and reduce the padding size by 1 byte? I
>> guess that will also require a patch to livepatch-build-tools?
>>
>> Bjoern
> 
> struct livepatch_func contains info that the build tool needs to
> communicate to Xen as well as space for Xen's internal book keeping
> while the live patch is applied. This includes the array for storing
> instructions, the applied flag, and now additionally the patch offset.
> (It's a somewhat odd arrangement but it's what we've got...)
> 
> The build tool does not need to know the details about any of Xen's internal
> book keeping. So my preference would be to have patch_offset as an additional
> member next to applied (reducing padding by 1) and then in livepatch-build-tools
> replace:
> 
>          unsigned char pad[MAX_REPLACEMENT_SIZE];
>          uint8_t applied;
>          uint8_t _pad[7];
> 
> with simply:
> 
>          uint8_t opaque[39];
> 
> 
> What do you think?

That will simplify this patch - I like it. Will send update + 
livepatch-build patch.

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

Patch

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)];
+};
+
 static bool has_active_waitqueue(const struct vm_event_domain *ved)
 {
     /* ved may be xzalloc()'d without INIT_LIST_HEAD() yet. */
@@ -104,18 +122,34 @@  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 )
     {
         /* Only do up to maximum amount we can put in the ->opaque. */
-        if ( func->new_size > sizeof(func->opaque) )
+        if ( func->new_size > sizeof_field(struct x86_livepatch_meta,
+                                           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 +161,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 +183,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 +200,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));
+    const 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));
 }
 
 /*