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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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

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

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

Thanks, Roger.