* [PATCH v2] livepatch: account for patch offset when applying NOP patch
@ 2022-03-30 11:05 Jan Beulich
2022-03-30 14:19 ` Roger Pau Monné
2022-03-30 17:04 ` Roger Pau Monné
0 siblings, 2 replies; 10+ messages in thread
From: Jan Beulich @ 2022-03-30 11:05 UTC (permalink / raw)
To: xen-devel
Cc: Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Roger Pau Monné,
Bjoern Doebel
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")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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 that the other use of is_endbr64() / is_endbr64_poison() in
arch_livepatch_verify_func() would need the extra check only for
cosmetic reasons, because ARCH_PATCH_INSN_SIZE > ENDBR64_LEN (5 > 4).
Hence I'm not altering the code there.
--- 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 &&
+ (is_endbr64(old_ptr) || is_endbr64_poison(old_ptr)) )
func->patch_offset += ENDBR64_LEN;
+ /* This call must be re-issued once ->patch_offset has its final value. */
+ 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;
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-30 11:05 [PATCH v2] livepatch: account for patch offset when applying NOP patch Jan Beulich
@ 2022-03-30 14:19 ` Roger Pau Monné
2022-03-30 14:55 ` Jan Beulich
2022-03-30 17:04 ` Roger Pau Monné
1 sibling, 1 reply; 10+ messages in thread
From: Roger Pau Monné @ 2022-03-30 14:19 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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@suse.com>
FWIW, on the original implementation, I think it would have been
clearer to advance old_ptr and adjust the length?
> ---
> 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 that the other use of is_endbr64() / is_endbr64_poison() in
> arch_livepatch_verify_func() would need the extra check only for
> cosmetic reasons, because ARCH_PATCH_INSN_SIZE > ENDBR64_LEN (5 > 4).
> Hence I'm not altering the code there.
>
> --- 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 &&
> + (is_endbr64(old_ptr) || is_endbr64_poison(old_ptr)) )
> func->patch_offset += ENDBR64_LEN;
>
> + /* This call must be re-issued once ->patch_offset has its final value. */
> + 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;
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.
Thanks, Roger.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-30 14:19 ` Roger Pau Monné
@ 2022-03-30 14:55 ` Jan Beulich
2022-03-30 15:02 ` Roger Pau Monné
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2022-03-30 14:55 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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@suse.com>
>
> 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.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-30 14:55 ` Jan Beulich
@ 2022-03-30 15:02 ` Roger Pau Monné
0 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monné @ 2022-03-30 15:02 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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@suse.com>
> >
> > 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@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-30 11:05 [PATCH v2] livepatch: account for patch offset when applying NOP patch Jan Beulich
2022-03-30 14:19 ` Roger Pau Monné
@ 2022-03-30 17:04 ` Roger Pau Monné
2022-03-31 6:42 ` Jan Beulich
1 sibling, 1 reply; 10+ messages in thread
From: Roger Pau Monné @ 2022-03-30 17:04 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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 that the other use of is_endbr64() / is_endbr64_poison() in
> arch_livepatch_verify_func() would need the extra check only for
> cosmetic reasons, because ARCH_PATCH_INSN_SIZE > ENDBR64_LEN (5 > 4).
> Hence I'm not altering the code there.
>
> --- 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)?
Thanks, Roger.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-30 17:04 ` Roger Pau Monné
@ 2022-03-31 6:42 ` Jan Beulich
2022-03-31 8:01 ` Roger Pau Monné
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2022-03-31 6:42 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-31 6:42 ` Jan Beulich
@ 2022-03-31 8:01 ` Roger Pau Monné
2022-03-31 8:13 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Roger Pau Monné @ 2022-03-31 8:01 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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?
Thanks, Roger.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-31 8:01 ` Roger Pau Monné
@ 2022-03-31 8:13 ` Jan Beulich
2022-03-31 8:27 ` Roger Pau Monné
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2022-03-31 8:13 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-31 8:13 ` Jan Beulich
@ 2022-03-31 8:27 ` Roger Pau Monné
2022-03-31 8:36 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Roger Pau Monné @ 2022-03-31 8:27 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: account for patch offset when applying NOP patch
2022-03-31 8:27 ` Roger Pau Monné
@ 2022-03-31 8:36 ` Jan Beulich
0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2022-03-31 8:36 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, Ross Lagerwall, Konrad Wilk, Andrew Cooper, Wei Liu,
Bjoern Doebel
On 31.03.2022 10:27, Roger Pau Monné wrote:
> 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?
If it wasn't to be called anymore, it would be better to fill the
space with INT3, not NOP. I think the purpose isn't really to nop
out entire functions; it's just that the NOP testcase in the tree
does so.
> 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.
I was wondering too - it would have seemed more natural to either
require old_size == new_size in this case, or to demand new_size == 0
matching new_addr == NULL. I'm afraid I have to rely on the livepatch
maintainers to answer your questions.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-03-31 8:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-30 11:05 [PATCH v2] livepatch: account for patch offset when applying NOP patch Jan Beulich
2022-03-30 14:19 ` Roger Pau Monné
2022-03-30 14:55 ` Jan Beulich
2022-03-30 15:02 ` Roger Pau Monné
2022-03-30 17:04 ` Roger Pau Monné
2022-03-31 6:42 ` Jan Beulich
2022-03-31 8:01 ` Roger Pau Monné
2022-03-31 8:13 ` Jan Beulich
2022-03-31 8:27 ` Roger Pau Monné
2022-03-31 8:36 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.