While not triggered by the trivial xen_nop in-tree patch on
staging/master, that patch exposes a problem on the stable trees, where
all functions have ENDBR inserted. When NOP-ing out a range, we need to
account for this. Handle this right in livepatch_insn_len().

Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Only build tested, as I don't have a live patching environment available.

For Arm this assumes that the patch_offset field starts out as zero; I
think we can make such an assumption, yet otoh on x86 explicit
initialization was added by the cited commit.

--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -90,7 +90,7 @@ static inline
 unsigned int livepatch_insn_len(const struct livepatch_func *func)
 {
     if ( !func->new_addr )
-        return func->new_size;
+        return func->new_size - func->patch_offset;
 
     return ARCH_PATCH_INSN_SIZE;
 }

On Wed, Mar 30, 2022 at 10:03:11AM +0200, Jan Beulich wrote:
> While not triggered by the trivial xen_nop in-tree patch on
> staging/master, that patch exposes a problem on the stable trees, where
> all functions have ENDBR inserted. When NOP-ing out a range, we need to
> account for this. Handle this right in livepatch_insn_len().
> 
> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Only build tested, as I don't have a live patching environment available.
> 
> For Arm this assumes that the patch_offset field starts out as zero; I
> think we can make such an assumption, yet otoh on x86 explicit
> initialization was added by the cited commit.
> 
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -90,7 +90,7 @@ static inline
>  unsigned int livepatch_insn_len(const struct livepatch_func *func)
>  {
>      if ( !func->new_addr )
> -        return func->new_size;
> +        return func->new_size - func->patch_offset;
>  
>      return ARCH_PATCH_INSN_SIZE;
>  }

Don't you also need to move the call to livepatch_insn_len() in
arch_livepatch_apply() after func->patch_offset has been adjusted to
account for ENDBR presence?

Thanks, Roger.

On 30.03.2022 12:19, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 10:03:11AM +0200, Jan Beulich wrote:
>> While not triggered by the trivial xen_nop in-tree patch on
>> staging/master, that patch exposes a problem on the stable trees, where
>> all functions have ENDBR inserted. When NOP-ing out a range, we need to
>> account for this. Handle this right in livepatch_insn_len().
>>
>> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Only build tested, as I don't have a live patching environment available.
>>
>> For Arm this assumes that the patch_offset field starts out as zero; I
>> think we can make such an assumption, yet otoh on x86 explicit
>> initialization was added by the cited commit.
>>
>> --- a/xen/include/xen/livepatch.h
>> +++ b/xen/include/xen/livepatch.h
>> @@ -90,7 +90,7 @@ static inline
>>  unsigned int livepatch_insn_len(const struct livepatch_func *func)
>>  {
>>      if ( !func->new_addr )
>> -        return func->new_size;
>> +        return func->new_size - func->patch_offset;
>>  
>>      return ARCH_PATCH_INSN_SIZE;
>>  }
> 
> Don't you also need to move the call to livepatch_insn_len() in
> arch_livepatch_apply() after func->patch_offset has been adjusted to
> account for ENDBR presence?

Oh, yes, I definitely need to.

Jan

On 30.03.2022 12:43, Jan Beulich wrote:
> On 30.03.2022 12:19, Roger Pau Monné wrote:
>> On Wed, Mar 30, 2022 at 10:03:11AM +0200, Jan Beulich wrote:
>>> While not triggered by the trivial xen_nop in-tree patch on
>>> staging/master, that patch exposes a problem on the stable trees, where
>>> all functions have ENDBR inserted. When NOP-ing out a range, we need to
>>> account for this. Handle this right in livepatch_insn_len().
>>>
>>> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> Only build tested, as I don't have a live patching environment available.
>>>
>>> For Arm this assumes that the patch_offset field starts out as zero; I
>>> think we can make such an assumption, yet otoh on x86 explicit
>>> initialization was added by the cited commit.
>>>
>>> --- a/xen/include/xen/livepatch.h
>>> +++ b/xen/include/xen/livepatch.h
>>> @@ -90,7 +90,7 @@ static inline
>>>  unsigned int livepatch_insn_len(const struct livepatch_func *func)
>>>  {
>>>      if ( !func->new_addr )
>>> -        return func->new_size;
>>> +        return func->new_size - func->patch_offset;
>>>  
>>>      return ARCH_PATCH_INSN_SIZE;
>>>  }
>>
>> Don't you also need to move the call to livepatch_insn_len() in
>> arch_livepatch_apply() after func->patch_offset has been adjusted to
>> account for ENDBR presence?
> 
> Oh, yes, I definitely need to.

Actually - not quite. I need to call the function a 2nd time. And
this then also points out that is_endbr64() and is_endbr64_poison()
may overrun the range pointed to by old_ptr (which I'll take care
of at the same time).

Jan