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 06:01:46PM +0100, Jan Beulich wrote:
> 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).

Oh, right, I was reading it wrong sorry, somehow I was (still) seeing
a pair of braces around PERDOMAIN_VIRT_START +
PERDOMAIN_ALT_VIRT_START when there are none.  Yes, I will adjust to
your suggestion.

Thanks, Roger.