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

Commit Message

Jan Beulich March 31, 2022, 6:49 a.m. UTC

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 <[e-mail]>
---
v3: Drop 1st livepatch_insn_len(). Drop buffer overrun fix.
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.

I think there's more fallout from the cited commit, but that'll need to
wait.

Comments

Roger Pau Monné March 31, 2022, 8:21 a.m. UTC | #1

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 <[e-mail]>

Reviewed-by: Roger Pau Monné <[e-mail]>

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

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.

Thanks, Roger.

Jan Beulich March 31, 2022, 8:42 a.m. UTC | #2

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 <[e-mail]>
> 
> Reviewed-by: Roger Pau Monné <[e-mail]>

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

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

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

Jan

Ross Lagerwall April 7, 2022, 3:44 p.m. UTC | #3

> From: Jan Beulich <[e-mail]>
> Sent: Thursday, March 31, 2022 9:42 AM
> To: Roger Pau Monne <[e-mail]>
> Cc: [e-mail] <[e-mail]>; Ross Lagerwall <[e-mail]>; Konrad Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Wei Liu <[e-mail]>; Bjoern Doebel <[e-mail]>
> 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 <[e-mail]>
> > 
> > Reviewed-by: Roger Pau Monné <[e-mail]>
> 
> 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

Roger Pau Monné April 8, 2022, 8:19 a.m. UTC | #4

On Thu, Apr 07, 2022 at 03:44:16PM +0000, Ross Lagerwall wrote:
> > From: Jan Beulich <[e-mail]>
> > Sent: Thursday, March 31, 2022 9:42 AM
> > To: Roger Pau Monne <[e-mail]>
> > Cc: [e-mail] <[e-mail]>; Ross Lagerwall <[e-mail]>; Konrad Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Wei Liu <[e-mail]>; Bjoern Doebel <[e-mail]>
> > 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 <[e-mail]>
> > > 
> > > Reviewed-by: Roger Pau Monné <[e-mail]>
> > 
> > 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?

Thanks, Roger.

Konrad Rzeszutek Wilk April 8, 2022, 12:56 p.m. UTC | #5

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 <[e-mail]>
> > > Sent: Thursday, March 31, 2022 9:42 AM
> > > To: Roger Pau Monne <[e-mail]>
> > > Cc: [e-mail] <[e-mail]>; Ross Lagerwall <[e-mail]>; Konrad Wilk <[e-mail]>; Andrew Cooper <[e-mail]>; Wei Liu <[e-mail]>; Bjoern Doebel <[e-mail]>
> > > 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 <[e-mail]>
> > > > 
> > > > Reviewed-by: Roger Pau Monné <[e-mail]>
> > > 
> > > 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.

Patch

--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -145,9 +145,6 @@  void noinline arch_livepatch_apply(struc
 
     func->patch_offset = 0;
     old_ptr = func->old_addr;
-    len = livepatch_insn_len(func);
-    if ( !len )
-        return;
 
     /*
      * CET hotpatching support: We may have functions starting with an ENDBR64
@@ -160,6 +157,11 @@  void noinline arch_livepatch_apply(struc
     if ( is_endbr64(old_ptr) || is_endbr64_poison(func->old_addr) )
         func->patch_offset += ENDBR64_LEN;
 
+    /* This call must be done with ->patch_offset already set. */
+    len = livepatch_insn_len(func);
+    if ( !len )
+        return;
+
     memcpy(func->opaque, old_ptr + func->patch_offset, len);
     if ( func->new_addr )
     {
--- 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;
 
     return ARCH_PATCH_INSN_SIZE;
 }