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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Apr 08, 2022 at 10:19:54AM +0200, Roger Pau Monné wrote:
> On Thu, Apr 07, 2022 at 03:44:16PM +0000, Ross Lagerwall wrote:
> > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > Sent: Thursday, March 31, 2022 9:42 AM
> > > To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Ross 
> > > Lagerwall <ross.lagerwall@xxxxxxxxxx>; Konrad Wilk 
> > > <konrad.wilk@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei 
> > > Liu <wl@xxxxxxx>; Bjoern Doebel <doebel@xxxxxxxxx>
> > > 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 <jbeulich@xxxxxxxx>
> > > > 
> > > > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > 
> > > 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).
> 
> Sorry, maybe a naive question, but why not use old_size directly to
> overwrite with NOPs?
> 
> Is this because you could generate a patch that just removed code from
> a function and then you would ideally just overwrite with NOPs the
> section to be removed while leaving the rest of the function as-is (so
> no jump added)?
> 
> I wonder whether we exercise this functionality at all, as I would
> imagine is quite hard to come with such payload?

The original idea behind this was to do any type of changes - not just
nop but other instructions too. Aka inline assembler changes. It is not
something livepatch-build supports but you can handcraft those if you
are in a pinch.

And the test-cases just do nop as that was the easiest one to create.
> 
> Thanks, Roger.