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

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

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.