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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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".
> 
> 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