| Message ID | 20240423131249.29825-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | livepatch: minor bug fixes and improvements | expand |
On 23.04.2024 15:12, Roger Pau Monne wrote: > Ensure the entries of a payload exception table only apply to text regions in > the payload itself. Since the payload exception table needs to be loaded and > active even before a patch is applied (because hooks might already rely on it), > make sure the exception table (if any) only contains fixups for the payload > text section. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> In principle Reviewed-by: Jan Beulich <jbeulich@suse.com> Still two comments: > --- a/xen/arch/x86/extable.c > +++ b/xen/arch/x86/extable.c > @@ -228,3 +228,21 @@ unsigned long asmlinkage search_pre_exception_table(struct cpu_user_regs *regs) > } > return fixup; > } > + > +#ifdef CONFIG_LIVEPATCH > +bool extable_is_between_bounds(const struct exception_table_entry *ex_start, s/between/in/ or even s/is_between/in/? "Between", to me at least, reads very much like meaning "exclusive at both ends". > + const struct exception_table_entry *ex_end, > + const void *start, const void *end) > +{ > + for ( ; ex_start < ex_end; ex_start++ ) > + { > + const void *addr = (void *)ex_addr(ex_start); > + const void *cont = (void *)ex_cont(ex_start); Might be nicer to use _p() here, or not do the comparisons with pointers, but instead with unsigned long-s. Jan
On Tue, Apr 23, 2024 at 03:51:31PM +0200, Jan Beulich wrote: > On 23.04.2024 15:12, Roger Pau Monne wrote: > > Ensure the entries of a payload exception table only apply to text regions in > > the payload itself. Since the payload exception table needs to be loaded and > > active even before a patch is applied (because hooks might already rely on it), > > make sure the exception table (if any) only contains fixups for the payload > > text section. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > In principle > Reviewed-by: Jan Beulich <jbeulich@suse.com> > Still two comments: > > > --- a/xen/arch/x86/extable.c > > +++ b/xen/arch/x86/extable.c > > @@ -228,3 +228,21 @@ unsigned long asmlinkage search_pre_exception_table(struct cpu_user_regs *regs) > > } > > return fixup; > > } > > + > > +#ifdef CONFIG_LIVEPATCH > > +bool extable_is_between_bounds(const struct exception_table_entry *ex_start, > > s/between/in/ or even s/is_between/in/? "Between", to me at least, reads > very much like meaning "exclusive at both ends". Oh, OK, I don't associate any boundary inclusion with 'between' or 'in'. The result is shorter, so I like it. > > + const struct exception_table_entry *ex_end, > > + const void *start, const void *end) > > +{ > > + for ( ; ex_start < ex_end; ex_start++ ) > > + { > > + const void *addr = (void *)ex_addr(ex_start); > > + const void *cont = (void *)ex_cont(ex_start); > > Might be nicer to use _p() here, or not do the comparisons with pointers, but > instead with unsigned long-s. No strong opinion regarding whether to use unsigned longs or pointers. I've used pointers because I think the function parameters should be pointers, and that avoided doing a cast in the comparison with obfuscates it (or introducing yet another local variable). I can switch to _p(), that's indeed better. Let me know if you have a strong opinion for using unsigned longs, otherwise my preference would be to leave it with pointers. Thanks, Roger.
On 23.04.2024 16:31, Roger Pau Monné wrote: > On Tue, Apr 23, 2024 at 03:51:31PM +0200, Jan Beulich wrote: >> On 23.04.2024 15:12, Roger Pau Monne wrote: >>> Ensure the entries of a payload exception table only apply to text regions in >>> the payload itself. Since the payload exception table needs to be loaded and >>> active even before a patch is applied (because hooks might already rely on it), >>> make sure the exception table (if any) only contains fixups for the payload >>> text section. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> >> In principle >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> Still two comments: >> >>> --- a/xen/arch/x86/extable.c >>> +++ b/xen/arch/x86/extable.c >>> @@ -228,3 +228,21 @@ unsigned long asmlinkage search_pre_exception_table(struct cpu_user_regs *regs) >>> } >>> return fixup; >>> } >>> + >>> +#ifdef CONFIG_LIVEPATCH >>> +bool extable_is_between_bounds(const struct exception_table_entry *ex_start, >> >> s/between/in/ or even s/is_between/in/? "Between", to me at least, reads >> very much like meaning "exclusive at both ends". > > Oh, OK, I don't associate any boundary inclusion with 'between' or > 'in'. The result is shorter, so I like it. > >>> + const struct exception_table_entry *ex_end, >>> + const void *start, const void *end) >>> +{ >>> + for ( ; ex_start < ex_end; ex_start++ ) >>> + { >>> + const void *addr = (void *)ex_addr(ex_start); >>> + const void *cont = (void *)ex_cont(ex_start); >> >> Might be nicer to use _p() here, or not do the comparisons with pointers, but >> instead with unsigned long-s. > > No strong opinion regarding whether to use unsigned longs or pointers. > I've used pointers because I think the function parameters should be > pointers, and that avoided doing a cast in the comparison with > obfuscates it (or introducing yet another local variable). > > I can switch to _p(), that's indeed better. > > Let me know if you have a strong opinion for using unsigned longs, > otherwise my preference would be to leave it with pointers. Especially if you want to stick to pointer function arguments - no, no strong opinion. Jan
diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c index 8415cd1fa249..9e91e8234e71 100644 --- a/xen/arch/x86/extable.c +++ b/xen/arch/x86/extable.c @@ -228,3 +228,21 @@ unsigned long asmlinkage search_pre_exception_table(struct cpu_user_regs *regs) } return fixup; } + +#ifdef CONFIG_LIVEPATCH +bool extable_is_between_bounds(const struct exception_table_entry *ex_start, + const struct exception_table_entry *ex_end, + const void *start, const void *end) +{ + for ( ; ex_start < ex_end; ex_start++ ) + { + const void *addr = (void *)ex_addr(ex_start); + const void *cont = (void *)ex_cont(ex_start); + + if ( addr < start || addr >= end || cont < start || cont >= end ) + return false; + } + + return true; +} +#endif diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h index 48b684c19d44..0dad61e21a9c 100644 --- a/xen/arch/x86/include/asm/uaccess.h +++ b/xen/arch/x86/include/asm/uaccess.h @@ -426,5 +426,9 @@ extern unsigned long search_exception_table(const struct cpu_user_regs *regs, extern void sort_exception_tables(void); extern void sort_exception_table(struct exception_table_entry *start, const struct exception_table_entry *stop); +extern bool extable_is_between_bounds( + const struct exception_table_entry *ex_start, + const struct exception_table_entry *ex_end, + const void *start, const void *end); #endif /* __X86_UACCESS_H__ */ diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 36cf4bee8b8a..67b6815d87ac 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -912,6 +912,15 @@ static int prepare_payload(struct payload *payload, s = sec->load_addr; e = sec->load_addr + sec->sec->sh_size; + if ( !extable_is_between_bounds(s, e, payload->text_addr, + payload->text_addr + payload->text_size) ) + { + printk(XENLOG_ERR LIVEPATCH + "%s: Invalid exception table with out of bounds entries\n", + elf->name); + return -EINVAL; + } + sort_exception_table(s ,e); region->ex = s;
Ensure the entries of a payload exception table only apply to text regions in the payload itself. Since the payload exception table needs to be loaded and active even before a patch is applied (because hooks might already rely on it), make sure the exception table (if any) only contains fixups for the payload text section. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - New in this version. --- xen/arch/x86/extable.c | 18 ++++++++++++++++++ xen/arch/x86/include/asm/uaccess.h | 4 ++++ xen/common/livepatch.c | 9 +++++++++ 3 files changed, 31 insertions(+)