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

This requires livepatch_insn_len() to be called _after_ ->patch_offset
was set. Note however that the earlier call cannot be deleted. In fact
its result should have been used to guard the is_endbr64() /
is_endbr64_poison() invocations - add the missing check now. While
making that adjustment, also use the local variable "old_ptr"
consistently.

Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-issue livepatch_insn_len(). Fix buffer overrun.
---
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.

Note that the other use of is_endbr64() / is_endbr64_poison() in
arch_livepatch_verify_func() would need the extra check only for
cosmetic reasons, because ARCH_PATCH_INSN_SIZE > ENDBR64_LEN (5 > 4).
Hence I'm not altering the code there.

--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
      * loaded hotpatch (to avoid racing against other fixups adding/removing
      * ENDBR64 or similar instructions).
      */
-    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
+    if ( len >= ENDBR64_LEN &&
+         (is_endbr64(old_ptr) || is_endbr64_poison(old_ptr)) )
         func->patch_offset += ENDBR64_LEN;
 
+    /* This call must be re-issued once ->patch_offset has its final value. */
+    len = livepatch_insn_len(func);
+    if ( !len )
+        return;
+
     memcpy(func->opaque, old_ptr + func->patch_offset, len);
     if ( func->new_addr )
     {
--- 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 01:05:31PM +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().
> 
> This requires livepatch_insn_len() to be called _after_ ->patch_offset
> was set. Note however that the earlier call cannot be deleted. In fact
> its result should have been used to guard the is_endbr64() /
> is_endbr64_poison() invocations - add the missing check now. While
> making that adjustment, also use the local variable "old_ptr"
> consistently.
> 
> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Re-issue livepatch_insn_len(). Fix buffer overrun.
> ---
> 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.
> 
> Note that the other use of is_endbr64() / is_endbr64_poison() in
> arch_livepatch_verify_func() would need the extra check only for
> cosmetic reasons, because ARCH_PATCH_INSN_SIZE > ENDBR64_LEN (5 > 4).
> Hence I'm not altering the code there.
> 
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
>       * loaded hotpatch (to avoid racing against other fixups adding/removing
>       * ENDBR64 or similar instructions).
>       */
> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
> +    if ( len >= ENDBR64_LEN &&

Sorry, didn't realize before, but shouldn't this check be using
old_size instead of len (which is based on new_size)?

Thanks, Roger.

On 30.03.2022 19:04, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 01:05:31PM +0200,>> --- a/xen/arch/x86/livepatch.c
>> +++ b/xen/arch/x86/livepatch.c
>> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
>>       * loaded hotpatch (to avoid racing against other fixups adding/removing
>>       * ENDBR64 or similar instructions).
>>       */
>> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
>> +    if ( len >= ENDBR64_LEN &&
> 
> Sorry, didn't realize before, but shouldn't this check be using
> old_size instead of len (which is based on new_size)?

Yes and no: In principle yes, but with len == func->new_size in the NOP
case, and with arch_livepatch_verify_func() guaranteeing new_size <=
old_size, the check is still fine for that case. Plus: If new_size was
less than 4 _but_ there's an ENDBR64 at the start, what would we do? I
think there's more that needs fixing in this regard. So I guess I'll
make a v3 with this extra fix dropped and with the livepatch_insn_len()
invocation simply moved. After all the primary goal is to get the
stable trees unstuck.

Jan

On Thu, Mar 31, 2022 at 08:42:47AM +0200, Jan Beulich wrote:
> On 30.03.2022 19:04, Roger Pau Monné wrote:
> > On Wed, Mar 30, 2022 at 01:05:31PM +0200,>> --- a/xen/arch/x86/livepatch.c
> >> +++ b/xen/arch/x86/livepatch.c
> >> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
> >>       * loaded hotpatch (to avoid racing against other fixups adding/removing
> >>       * ENDBR64 or similar instructions).
> >>       */
> >> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
> >> +    if ( len >= ENDBR64_LEN &&
> > 
> > Sorry, didn't realize before, but shouldn't this check be using
> > old_size instead of len (which is based on new_size)?
> 
> Yes and no: In principle yes, but with len == func->new_size in the NOP
> case, and with arch_livepatch_verify_func() guaranteeing new_size <=
> old_size, the check is still fine for that case. Plus: If new_size was
> less than 4 _but_ there's an ENDBR64 at the start, what would we do? I
> think there's more that needs fixing in this regard. So I guess I'll
> make a v3 with this extra fix dropped and with the livepatch_insn_len()
> invocation simply moved. After all the primary goal is to get the
> stable trees unstuck.

Right, I agree to try and get the stable trees unblocked ASAP.

I think the check for ENDBR is only relevant when we are patching the
function with a jump, otherwise the new replacement code should
contain the ENDBR instruction already?

Thanks, Roger.

On 31.03.2022 10:01, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 08:42:47AM +0200, Jan Beulich wrote:
>> On 30.03.2022 19:04, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 01:05:31PM +0200,>> --- a/xen/arch/x86/livepatch.c
>>>> +++ b/xen/arch/x86/livepatch.c
>>>> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
>>>>       * loaded hotpatch (to avoid racing against other fixups adding/removing
>>>>       * ENDBR64 or similar instructions).
>>>>       */
>>>> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
>>>> +    if ( len >= ENDBR64_LEN &&
>>>
>>> Sorry, didn't realize before, but shouldn't this check be using
>>> old_size instead of len (which is based on new_size)?
>>
>> Yes and no: In principle yes, but with len == func->new_size in the NOP
>> case, and with arch_livepatch_verify_func() guaranteeing new_size <=
>> old_size, the check is still fine for that case. Plus: If new_size was
>> less than 4 _but_ there's an ENDBR64 at the start, what would we do? I
>> think there's more that needs fixing in this regard. So I guess I'll
>> make a v3 with this extra fix dropped and with the livepatch_insn_len()
>> invocation simply moved. After all the primary goal is to get the
>> stable trees unstuck.
> 
> Right, I agree to try and get the stable trees unblocked ASAP.
> 
> I think the check for ENDBR is only relevant when we are patching the
> function with a jump, otherwise the new replacement code should
> contain the ENDBR instruction already?

No, the "otherwise" case is when we're NOP-ing out code, i.e. when
there's no replacement code at all. In this case we need to leave
intact the ENDBR, and new_size being less than 4 is bogus afaict in
case there actually is an ENDBR.

Jan

On Thu, Mar 31, 2022 at 10:13:06AM +0200, Jan Beulich wrote:
> On 31.03.2022 10:01, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 08:42:47AM +0200, Jan Beulich wrote:
> >> On 30.03.2022 19:04, Roger Pau Monné wrote:
> >>> On Wed, Mar 30, 2022 at 01:05:31PM +0200,>> --- a/xen/arch/x86/livepatch.c
> >>>> +++ b/xen/arch/x86/livepatch.c
> >>>> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
> >>>>       * loaded hotpatch (to avoid racing against other fixups adding/removing
> >>>>       * ENDBR64 or similar instructions).
> >>>>       */
> >>>> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
> >>>> +    if ( len >= ENDBR64_LEN &&
> >>>
> >>> Sorry, didn't realize before, but shouldn't this check be using
> >>> old_size instead of len (which is based on new_size)?
> >>
> >> Yes and no: In principle yes, but with len == func->new_size in the NOP
> >> case, and with arch_livepatch_verify_func() guaranteeing new_size <=
> >> old_size, the check is still fine for that case. Plus: If new_size was
> >> less than 4 _but_ there's an ENDBR64 at the start, what would we do? I
> >> think there's more that needs fixing in this regard. So I guess I'll
> >> make a v3 with this extra fix dropped and with the livepatch_insn_len()
> >> invocation simply moved. After all the primary goal is to get the
> >> stable trees unstuck.
> > 
> > Right, I agree to try and get the stable trees unblocked ASAP.
> > 
> > I think the check for ENDBR is only relevant when we are patching the
> > function with a jump, otherwise the new replacement code should
> > contain the ENDBR instruction already?
> 
> No, the "otherwise" case is when we're NOP-ing out code, i.e. when
> there's no replacement code at all. In this case we need to leave
> intact the ENDBR, and new_size being less than 4 is bogus afaict in
> case there actually is an ENDBR.

Hm, so we never do in-place replacement of code, and we either
introduce a jump to the new code or otherwise the function is not to
be called anymore and hence we fill it with no-ops?

Shouldn't in the no-op filling case the call to add_nops be bounded by
old_size and salso the memcpy to old_addr?

I'm not sure I understand why we use new_size when memcpy'ing into
old_addr, or when filling the insn buffer.

Thanks, Roger.

On 31.03.2022 10:27, Roger Pau Monné wrote:
> On Thu, Mar 31, 2022 at 10:13:06AM +0200, Jan Beulich wrote:
>> On 31.03.2022 10:01, Roger Pau Monné wrote:
>>> On Thu, Mar 31, 2022 at 08:42:47AM +0200, Jan Beulich wrote:
>>>> On 30.03.2022 19:04, Roger Pau Monné wrote:
>>>>> On Wed, Mar 30, 2022 at 01:05:31PM +0200,>> --- a/xen/arch/x86/livepatch.c
>>>>>> +++ b/xen/arch/x86/livepatch.c
>>>>>> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
>>>>>>       * loaded hotpatch (to avoid racing against other fixups adding/removing
>>>>>>       * ENDBR64 or similar instructions).
>>>>>>       */
>>>>>> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
>>>>>> +    if ( len >= ENDBR64_LEN &&
>>>>>
>>>>> Sorry, didn't realize before, but shouldn't this check be using
>>>>> old_size instead of len (which is based on new_size)?
>>>>
>>>> Yes and no: In principle yes, but with len == func->new_size in the NOP
>>>> case, and with arch_livepatch_verify_func() guaranteeing new_size <=
>>>> old_size, the check is still fine for that case. Plus: If new_size was
>>>> less than 4 _but_ there's an ENDBR64 at the start, what would we do? I
>>>> think there's more that needs fixing in this regard. So I guess I'll
>>>> make a v3 with this extra fix dropped and with the livepatch_insn_len()
>>>> invocation simply moved. After all the primary goal is to get the
>>>> stable trees unstuck.
>>>
>>> Right, I agree to try and get the stable trees unblocked ASAP.
>>>
>>> I think the check for ENDBR is only relevant when we are patching the
>>> function with a jump, otherwise the new replacement code should
>>> contain the ENDBR instruction already?
>>
>> No, the "otherwise" case is when we're NOP-ing out code, i.e. when
>> there's no replacement code at all. In this case we need to leave
>> intact the ENDBR, and new_size being less than 4 is bogus afaict in
>> case there actually is an ENDBR.
> 
> Hm, so we never do in-place replacement of code, and we either
> introduce a jump to the new code or otherwise the function is not to
> be called anymore and hence we fill it with no-ops?

If it wasn't to be called anymore, it would be better to fill the
space with INT3, not NOP. I think the purpose isn't really to nop
out entire functions; it's just that the NOP testcase in the tree
does so.

> Shouldn't in the no-op filling case the call to add_nops be bounded by
> old_size and salso the memcpy to old_addr?
> 
> I'm not sure I understand why we use new_size when memcpy'ing into
> old_addr, or when filling the insn buffer.

I was wondering too - it would have seemed more natural to either
require old_size == new_size in this case, or to demand new_size == 0
matching new_addr == NULL. I'm afraid I have to rely on the livepatch
maintainers to answer your questions.

Jan

On Wed, Mar 30, 2022 at 01:05:31PM +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().
> 
> This requires livepatch_insn_len() to be called _after_ ->patch_offset
> was set. Note however that the earlier call cannot be deleted. In fact
> its result should have been used to guard the is_endbr64() /
> is_endbr64_poison() invocations - add the missing check now. While
> making that adjustment, also use the local variable "old_ptr"
> consistently.
> 
> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")

I have to admit I'm confused as to why that commit carries a Tested-by
from Arm.  Did Arm test the commit on x86 hardware?  Because that
commit only touches x86 specific code.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

FWIW, on the original implementation, I think it would have been
clearer to advance old_ptr and adjust the length?

> ---
> v2: Re-issue livepatch_insn_len(). Fix buffer overrun.
> ---
> 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.
> 
> Note that the other use of is_endbr64() / is_endbr64_poison() in
> arch_livepatch_verify_func() would need the extra check only for
> cosmetic reasons, because ARCH_PATCH_INSN_SIZE > ENDBR64_LEN (5 > 4).
> Hence I'm not altering the code there.
> 
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -157,9 +157,15 @@ void noinline arch_livepatch_apply(struc
>       * loaded hotpatch (to avoid racing against other fixups adding/removing
>       * ENDBR64 or similar instructions).
>       */
> -    if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
> +    if ( len >= ENDBR64_LEN &&
> +         (is_endbr64(old_ptr) || is_endbr64_poison(old_ptr)) )
>          func->patch_offset += ENDBR64_LEN;
>  
> +    /* This call must be re-issued once ->patch_offset has its final value. */
> +    len = livepatch_insn_len(func);
> +    if ( !len )
> +        return;
> +
>      memcpy(func->opaque, old_ptr + func->patch_offset, len);
>      if ( func->new_addr )
>      {
> --- 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;

Seeing as func->patch_offset is explicitly initialized in
arch_livepatch_apply for x86, do we also need to do the same on Arm
now that the field will be used by common code?

Maybe the initialization done in arch_livepatch_apply for x86 is not
strictly required.

Thanks, Roger.

On 30.03.2022 16:19, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 01:05:31PM +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().
>>
>> This requires livepatch_insn_len() to be called _after_ ->patch_offset
>> was set. Note however that the earlier call cannot be deleted. In fact
>> its result should have been used to guard the is_endbr64() /
>> is_endbr64_poison() invocations - add the missing check now. While
>> making that adjustment, also use the local variable "old_ptr"
>> consistently.
>>
>> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
> 
> I have to admit I'm confused as to why that commit carries a Tested-by
> from Arm.  Did Arm test the commit on x86 hardware?  Because that
> commit only touches x86 specific code.

;-)

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> FWIW, on the original implementation, I think it would have been
> clearer to advance old_ptr and adjust the length?

In my 1st attempt I had confined the change to the x86 file, but it
didn't feel right that I then also had to adjust arch_livepatch_revert().

>> ---
>> v2: Re-issue livepatch_insn_len(). Fix buffer overrun.
>> ---
>> 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.

Note how this already deals with ...

>> --- 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;
> 
> Seeing as func->patch_offset is explicitly initialized in
> arch_livepatch_apply for x86, do we also need to do the same on Arm
> now that the field will be used by common code?
> 
> Maybe the initialization done in arch_livepatch_apply for x86 is not
> strictly required.

... your remark. I'd prefer if I could get away without touching Arm
code. Hence if such initialization was needed, I think it ought to
live in common code. If this was a requirement here, I would perhaps
add a prereq patch doing the movement. My preference though would be
for that to be a follow-on, unless there's an actual reason why the
initialization has to happen; personally I think it ought to be a
requirement on patch building that this (and perhaps other) fields
start out as zero. I therefore view the initialization on x86 as a
guard against the patch getting applied a 2nd time. Yet of course it
would then also have helped (not anymore after this change) to use
= instead of += when updating ->patch_offset.

Jan

On Wed, Mar 30, 2022 at 04:55:52PM +0200, Jan Beulich wrote:
> On 30.03.2022 16:19, Roger Pau Monné wrote:
> > On Wed, Mar 30, 2022 at 01:05:31PM +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().
> >>
> >> This requires livepatch_insn_len() to be called _after_ ->patch_offset
> >> was set. Note however that the earlier call cannot be deleted. In fact
> >> its result should have been used to guard the is_endbr64() /
> >> is_endbr64_poison() invocations - add the missing check now. While
> >> making that adjustment, also use the local variable "old_ptr"
> >> consistently.
> >>
> >> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions")
> > 
> > I have to admit I'm confused as to why that commit carries a Tested-by
> > from Arm.  Did Arm test the commit on x86 hardware?  Because that
> > commit only touches x86 specific code.
> 
> ;-)
> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > FWIW, on the original implementation, I think it would have been
> > clearer to advance old_ptr and adjust the length?
> 
> In my 1st attempt I had confined the change to the x86 file, but it
> didn't feel right that I then also had to adjust arch_livepatch_revert().
> 
> >> ---
> >> v2: Re-issue livepatch_insn_len(). Fix buffer overrun.
> >> ---
> >> 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.
> 
> Note how this already deals with ...
> 
> >> --- 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;
> > 
> > Seeing as func->patch_offset is explicitly initialized in
> > arch_livepatch_apply for x86, do we also need to do the same on Arm
> > now that the field will be used by common code?
> > 
> > Maybe the initialization done in arch_livepatch_apply for x86 is not
> > strictly required.
> 
> ... your remark. I'd prefer if I could get away without touching Arm
> code. Hence if such initialization was needed, I think it ought to
> live in common code. If this was a requirement here, I would perhaps
> add a prereq patch doing the movement. My preference though would be
> for that to be a follow-on, unless there's an actual reason why the
> initialization has to happen; personally I think it ought to be a
> requirement on patch building that this (and perhaps other) fields
> start out as zero. I therefore view the initialization on x86 as a
> guard against the patch getting applied a 2nd time. Yet of course it
> would then also have helped (not anymore after this change) to use
> = instead of += when updating ->patch_offset.

Sorry, I didn't realize about your post-commit note. In which case:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.