| Message ID | 772f0afc-64db-6b98-af49-bd970ac78cbb@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] livepatch: account for patch offset when applying NOP patch | expand |
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.
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
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.
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.
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
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.
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
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.
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
--- 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; }
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.