Re: [PATCH v2 3/5] x86/xlat: fix UB pointer arithmetic in COMPAT_ARG_XLAT_VIRT_BASE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2025 17:47, Roger Pau Monné wrote:
> On Tue, Mar 18, 2025 at 04:50:58PM +0100, Jan Beulich wrote:
>> On 18.03.2025 16:35, Roger Pau Monné wrote:
>>> On Tue, Mar 18, 2025 at 03:33:03PM +0100, Jan Beulich wrote:
>>>> On 18.03.2025 10:19, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/include/asm/x86_64/uaccess.h
>>>>> +++ b/xen/arch/x86/include/asm/x86_64/uaccess.h
>>>>> @@ -9,9 +9,9 @@
>>>>>   * a secondary mapping installed, which needs to be used for such 
>>>>> accesses in
>>>>>   * the PV case, and will also be used for HVM to avoid extra 
>>>>> conditionals.
>>>>>   */
>>>>> -#define COMPAT_ARG_XLAT_VIRT_BASE ((void *)ARG_XLAT_START(current) + \
>>>>> -                                   (PERDOMAIN_ALT_VIRT_START - \
>>>>> -                                    PERDOMAIN_VIRT_START))
>>>>> +#define COMPAT_ARG_XLAT_VIRT_BASE ((void *)ARG_XLAT_START(current) - \
>>>>> +                                   (PERDOMAIN_VIRT_START - \
>>>>> +                                    PERDOMAIN_ALT_VIRT_START))
>>>>
>>>> Aren't we then (still) dependent on ordering between PERDOMAIN_VIRT_START
>>>> and PERDOMAIN_ALT_VIRT_START? Would
>>>>
>>>> #define COMPAT_ARG_XLAT_VIRT_BASE ((void *)ARG_XLAT_START(current) - \
>>>>                                    PERDOMAIN_VIRT_START + \
>>>>                                    PERDOMAIN_ALT_VIRT_START)
>>>>
>>>> perhaps be less fragile?
>>>
>>> PERDOMAIN_{ALT_,}VIRT_START are unsigned long, so this might work.
>>>
>>> Note however that even with your suggestion we are still dependant on
>>> ARG_XLAT_START(v) > PERDOMAIN_ALT_VIRT_START, or else the '-' won't
>>> work.  I think I prefer my proposed version, because it's clear that
>>> PERDOMAIN_VIRT_START, ARG_XLAT_START(current) >
>>> PERDOMAIN_ALT_VIRT_START.
>>
>> What makes that clear? Can't we move PERDOMAIN_ALT_VIRT_START pretty
>> much at will?
> 
> We would need to adjust the calculations here again, if
> PERDOMAIN_ALT_VIRT_START > PERDOMAIN_VIRT_START the subtraction would
> lead to an underflow, and would also be UB pointer arithmetic?

With

#define ARG_XLAT_VIRT_START      PERDOMAIN_VIRT_SLOT(2)

I can't see how subtracting PERDOMAIN_VIRT_START could lead to an underflow.
The idea of the expression suggested is to first subtract the area base (no
underflow) and then add the other area's base (no overflow).

Jan