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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

Thanks, Roger.