Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> 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