| Message ID | 2df6d890-9d91-62cc-8057-3d50f1501ad5@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | livepatch: account for patch offset when applying NOP patch | expand |
On Wed, Mar 30, 2022 at 10:03:11AM +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(). > > Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > 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. > > --- 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; > } Don't you also need to move the call to livepatch_insn_len() in arch_livepatch_apply() after func->patch_offset has been adjusted to account for ENDBR presence? Thanks, Roger.
On 30.03.2022 12:19, Roger Pau Monné wrote: > On Wed, Mar 30, 2022 at 10:03:11AM +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(). >> >> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> 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. >> >> --- 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; >> } > > Don't you also need to move the call to livepatch_insn_len() in > arch_livepatch_apply() after func->patch_offset has been adjusted to > account for ENDBR presence? Oh, yes, I definitely need to. Jan
On 30.03.2022 12:43, Jan Beulich wrote: > On 30.03.2022 12:19, Roger Pau Monné wrote: >> On Wed, Mar 30, 2022 at 10:03:11AM +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(). >>> >>> Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> 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. >>> >>> --- 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; >>> } >> >> Don't you also need to move the call to livepatch_insn_len() in >> arch_livepatch_apply() after func->patch_offset has been adjusted to >> account for ENDBR presence? > > Oh, yes, I definitely need to. Actually - not quite. I need to call the function a 2nd time. And this then also points out that is_endbr64() and is_endbr64_poison() may overrun the range pointed to by old_ptr (which I'll take care of at the same time). Jan
--- 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(). Fixes: 6974c75180f1 ("xen/x86: Livepatch: support patching CET-enhanced functions") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- 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.