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 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.  With your suggestion that's not obvious.

Thanks, Roger.