| Message ID | 20250318091904.52903-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ubsan: fix ubsan on clang + code fixes | expand |
On 18/03/2025 9:19 am, Roger Pau Monne wrote: > UBSAN complains with: > > UBSAN: Undefined behaviour in common/compat/memory.c:90:9 > pointer operation overflowed ffff820080000000 to 0000020080000000 > [...] > Xen call trace: > [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0 > [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100 > [<ffff82d0402a6359>] F lib/xxhash64.c#compat_memory_op+0xf1/0x4d20 > [<ffff82d04041545d>] F lib/xxhash64.c#hvm_memory_op+0x55/0xe0 > [<ffff82d040416280>] F lib/xxhash64.c#hvm_hypercall+0xae8/0x21b0 > [<ffff82d0403b25ca>] F lib/xxhash64.c#svm_vmexit_handler+0x1252/0x2450 > [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15 > > Adjust the calculations in COMPAT_ARG_XLAT_VIRT_BASE to subtract from the > per-domain area to obtain the mirrored linear address in the 4th slot, > instead of overflowing the per-domain linear address. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
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? Jan
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.
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? Jan > With your suggestion that's not obvious. > > Thanks, Roger.
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.
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
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.
diff --git a/xen/arch/x86/include/asm/x86_64/uaccess.h b/xen/arch/x86/include/asm/x86_64/uaccess.h index c6fa3fd381bc..f933707e109f 100644 --- 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)) #define COMPAT_ARG_XLAT_SIZE (2*PAGE_SIZE) struct vcpu; int setup_compat_arg_xlat(struct vcpu *v);
UBSAN complains with: UBSAN: Undefined behaviour in common/compat/memory.c:90:9 pointer operation overflowed ffff820080000000 to 0000020080000000 [...] Xen call trace: [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0 [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100 [<ffff82d0402a6359>] F lib/xxhash64.c#compat_memory_op+0xf1/0x4d20 [<ffff82d04041545d>] F lib/xxhash64.c#hvm_memory_op+0x55/0xe0 [<ffff82d040416280>] F lib/xxhash64.c#hvm_hypercall+0xae8/0x21b0 [<ffff82d0403b25ca>] F lib/xxhash64.c#svm_vmexit_handler+0x1252/0x2450 [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15 Adjust the calculations in COMPAT_ARG_XLAT_VIRT_BASE to subtract from the per-domain area to obtain the mirrored linear address in the 4th slot, instead of overflowing the per-domain linear address. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/include/asm/x86_64/uaccess.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)