Re: [PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
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é
>>
>> In principle
>> Reviewed-by: Jan Beulich
>> 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
Re: [PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
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é
>
> In principle
> Reviewed-by: Jan Beulich
> 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.
Re: [PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
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é
In principle
Reviewed-by: Jan Beulich
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
[PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents
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é
---
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(+)
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;
--
2.44.0
