> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, March 31, 2022 9:42 AM
> To: Roger Pau Monne <roger....@citrix.com>
> Cc: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>; Ross
> Lagerwall <ross.lagerw...@citrix.com>; Konrad Wilk <konrad.w...@oracle.com>;
> Andrew Cooper <andrew.coop...@citrix.com>; Wei Liu <w...@xen.org>; Bjoern
> Doebel <doe...@amazon.de>
> Subject: Re: [PATCH v3] livepatch: account for patch offset when applying NOP
> patch
>
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments
> unless you have verified the sender and know the content is safe.
>
> On 31.03.2022 10:21, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 08:49:46AM +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.
> >>
> >> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced
> >> functions")
> >> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> >
> > Reviewed-by: Roger Pau Monné <roger....@citrix.com>
>
> Thanks.
>
> As a note to the livepatch maintainers: I'm going to put this in
> without further waiting for suitable acks. Just in case I'll put
> it on the 4.16 branch only for starters, to see that it actually
> helps there (it's unusual to put something on the stable
> branches before it having passed the push gate to master).
Thanks (was on PTO and away from email).
>
> > Albeit I don't think I understand how the in-place patching is done. I
> > would expect the !func->new_addr branch of the if in
> > arch_livepatch_apply to fill the insn buffer with the in-place
> > replacement instructions, but I only see the buffer getting filled
> > with nops. I'm likely missing something (not that this patch changes
> > any of this).
>
> Well, as per the v2 thread: There's no in-place patching except
> to NOP out certain insns.
Correct. FWIW I'm not really aware of a valid use case for this
>
> > I'm also having trouble figuring out how we assert that the len value
> > (which is derived from new_size if !new_addr) is not greater than
> > LIVEPATCH_OPAQUE_SIZE, which is the limit of the insn buffer. Maybe
> > that's already checked elsewhere.
>
> That's what my 3rd post-commit-message remark was (partly) about.
old_size specifies the length of the existing function to be patched.
If new_addr is zero (NOP case), then new_size specifies the number of
bytes to overwrite with NOP. That's why new_size is used as the memcpy
length (minus patch offset). It is checked against the size of the insn
buffer in arch_livepatch_verify_func(). I think the code is correct as is
but I could be missing something.
Ross
