Re: [PATCH v3 4/4] x86/livepatch: perform sanity checks on the payload exception table contents

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

In principle
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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