[PATCH v2 0/5] x86/ubsan: fix ubsan on clang + code fixes

[PATCH v2 0/5] x86/ubsan: fix ubsan on clang + code fixes

[Roger Pau Monne]

Hello,

This started as a series to fix UBSAN when using clang, and the first
patch do fix that.

Finally patches 2 to 4 fix bugs in the code highlighted by the clang UB
pointer arithmetic detection.

Patch 5 was the original goal of the series: be able to enable UBSAN for
randconfig.

Thanks, Roger.

Roger Pau Monne (5):
  x86/wait: prevent duplicated assembly labels
  x86/vga: fix mapping of the VGA text buffer
  x86/xlat: fix UB pointer arithmetic in COMPAT_ARG_XLAT_VIRT_BASE
  x86/shadow: fix UB pointer arithmetic in sh_mfn_is_a_page_table()
  kconfig/randconfig: enable UBSAN for randconfig

 xen/Kconfig                               |  4 ++++
 xen/Kconfig.debug                         |  2 +-
 xen/arch/x86/include/asm/x86_64/uaccess.h |  6 +++---
 xen/arch/x86/mm/shadow/private.h          |  3 ++-
 xen/common/wait.c                         | 10 ++++++++--
 xen/drivers/video/vga.c                   |  4 ++--
 xen/tools/kconfig/allrandom.config        |  1 -
 7 files changed, 20 insertions(+), 10 deletions(-)

-- 
2.48.1

[PATCH v2 1/5] x86/wait: prevent duplicated assembly labels

[Roger Pau Monne]

When enabling UBSAN with clang, the following error is triggered during the
build:

common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
  154 |         "push %%rbx; push %%rbp; push %%r12;"
      |         ^
<inline asm>:1:121: note: instantiated into assembly here
    1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
      |                                                                                                                                ^
common/wait.c:154:9: error: symbol '.L_skip' is already defined
  154 |         "push %%rbx; push %%rbp; push %%r12;"
      |         ^
<inline asm>:1:159: note: instantiated into assembly here
    1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
      |                                                                                                                                                                      ^
2 errors generated.

The inline assembly block in __prepare_to_wait() is duplicated, thus
leading to multiple definitions of the otherwise unique labels inside the
assembly block.  GCC extended-asm documentation notes the possibility of
duplicating asm blocks:

> Under certain circumstances, GCC may duplicate (or remove duplicates of)
> your assembly code when optimizing. This can lead to unexpected duplicate
> symbol errors during compilation if your asm code defines symbols or
> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.

Workaround the issue by latching esp to a local variable, this prevents
clang duplicating the inline asm blocks.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Use approach suggested by Jan.
---
 xen/common/wait.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/common/wait.c b/xen/common/wait.c
index cb6f5ff3c20a..9a11dccb5de5 100644
--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -124,6 +124,11 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
     struct cpu_info *cpu_info = get_cpu_info();
     struct vcpu *curr = current;
     unsigned long dummy;
+    /*
+     * Latch esp to a local variable to prevent clang from duplicating the
+     * inline assembly block when UBSAN is enabled.
+     */
+    void *esp = NULL;
 
     ASSERT(wqv->esp == NULL);
 
@@ -166,12 +171,12 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
         ".L_skip:"
         "pop %%r15; pop %%r14; pop %%r13;"
         "pop %%r12; pop %%rbp; pop %%rbx"
-        : "=&S" (wqv->esp), "=&c" (dummy), "=&D" (dummy)
+        : "=&S" (esp), "=&c" (dummy), "=&D" (dummy)
         : "0" (0), "1" (cpu_info), "2" (wqv->stack),
           [sz] "i" (PAGE_SIZE)
         : "memory", "rax", "rdx", "r8", "r9", "r10", "r11" );
 
-    if ( unlikely(wqv->esp == NULL) )
+    if ( unlikely(esp == NULL) )
     {
         gdprintk(XENLOG_ERR, "Stack too large in %s\n", __func__);
         domain_crash(curr->domain);
@@ -179,6 +184,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
         for ( ; ; )
             do_softirq();
     }
+    wqv->esp = esp;
 }
 
 static void __finish_wait(struct waitqueue_vcpu *wqv)
-- 
2.48.1

Re: [PATCH v2 1/5] x86/wait: prevent duplicated assembly labels

[Andrew Cooper]

On 18/03/2025 9:19 am, Roger Pau Monne wrote:
> When enabling UBSAN with clang, the following error is triggered during the
> build:
>
> common/wait.c:154:9: error: symbol '.L_wq_resume' is already defined
>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>       |         ^
> <inline asm>:1:121: note: instantiated into assembly here
>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>       |                                                                                                                                ^
> common/wait.c:154:9: error: symbol '.L_skip' is already defined
>   154 |         "push %%rbx; push %%rbp; push %%r12;"
>       |         ^
> <inline asm>:1:159: note: instantiated into assembly here
>     1 |         push %rbx; push %rbp; push %r12;push %r13; push %r14; push %r15;sub %esp,%ecx;cmp $4096, %ecx;ja .L_skip;mov %rsp,%rsi;.L_wq_resume: rep movsb;mov %rsp,%rsi;.L_skip:pop %r15; pop %r14; pop %r13;pop %r12; pop %rbp; pop %rbx
>       |                                                                                                                                                                      ^
> 2 errors generated.
>
> The inline assembly block in __prepare_to_wait() is duplicated, thus
> leading to multiple definitions of the otherwise unique labels inside the
> assembly block.  GCC extended-asm documentation notes the possibility of
> duplicating asm blocks:
>
>> Under certain circumstances, GCC may duplicate (or remove duplicates of)
>> your assembly code when optimizing. This can lead to unexpected duplicate
>> symbol errors during compilation if your asm code defines symbols or
>> labels. Using ‘%=’ (see AssemblerTemplate) may help resolve this problem.
> Workaround the issue by latching esp to a local variable, this prevents
> clang duplicating the inline asm blocks.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH v2 2/5] x86/vga: fix mapping of the VGA text buffer

[Roger Pau Monne]

The call to ioremap_wc() in video_init() will always fail, because
video_init() is called ahead of vm_init_type(), and so the underlying
__vmap() call will fail to allocate the linear address space.

Fix by reverting to the previous behavior and using ioremap() for the VGA
text buffer.

Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - No not attempt to adjust the directmap VGA text buffer mappings to be
   WC, just revert to the previous usage of UC- mappings for the whole VGA
   hole.
---
 xen/drivers/video/vga.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
index b4d018326128..ee6cf0a7079a 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -71,7 +71,7 @@ void __init video_init(void)
     {
     case XEN_VGATYPE_TEXT_MODE_3:
         if ( page_is_ram_type(paddr_to_pfn(0xB8000), RAM_TYPE_CONVENTIONAL) ||
-             ((video = ioremap_wc(0xB8000, 0x8000)) == NULL) )
+             ((video = ioremap(0xB8000, 0x8000)) == NULL) )
             return;
         outw(0x200a, 0x3d4); /* disable cursor */
         columns = vga_console_info.u.text_mode_3.columns;
@@ -158,7 +158,7 @@ void __init video_endboot(void)
         if ( !vgacon_keep )
         {
             memset(video, 0, columns * lines * 2);
-            iounmap(video);
+            /* No iounmap(), as it's a directmap mapping. */
             video = ZERO_BLOCK_PTR;
         }
         break;
-- 
2.48.1

Re: [PATCH v2 2/5] x86/vga: fix mapping of the VGA text buffer

[Andrew Cooper]

On 18/03/2025 9:19 am, Roger Pau Monne wrote:
> The call to ioremap_wc() in video_init() will always fail, because
> video_init() is called ahead of vm_init_type(), and so the underlying
> __vmap() call will fail to allocate the linear address space.
>
> Fix by reverting to the previous behavior and using ioremap() for the VGA
> text buffer.
>
> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This is somewhat ugly.

ioremap() isn't really any better than ioremap_wc(); this only works
because plain ioremap() has a special case for the bottom 1M where it
does nothing and exits.

What's the consequence of ioremap_wc() failing?  Presumably nothing out
on the screen when using legacy text mode?

From that point of view, this is an improvement I suppose.

~Andrew

Re: [PATCH v2 2/5] x86/vga: fix mapping of the VGA text buffer

[Jan Beulich]

On 18.03.2025 14:11, Andrew Cooper wrote:
> On 18/03/2025 9:19 am, Roger Pau Monne wrote:
>> The call to ioremap_wc() in video_init() will always fail, because
>> video_init() is called ahead of vm_init_type(), and so the underlying
>> __vmap() call will fail to allocate the linear address space.
>>
>> Fix by reverting to the previous behavior and using ioremap() for the VGA
>> text buffer.
>>
>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This is somewhat ugly.
> 
> ioremap() isn't really any better than ioremap_wc(); this only works
> because plain ioremap() has a special case for the bottom 1M where it
> does nothing and exits.

And this is exactly why I screwed up back then. Imo we would be better
off moving to using __va() directly here. Otherwise the same mistake
could be made again by someone (perhaps even me) noticing the less
efficient ioremap(), when ioremap_wc() really would be wanted.

Jan

Re: [PATCH v2 2/5] x86/vga: fix mapping of the VGA text buffer

[Roger Pau Monné]

On Tue, Mar 18, 2025 at 03:28:32PM +0100, Jan Beulich wrote:
> On 18.03.2025 14:11, Andrew Cooper wrote:
> > On 18/03/2025 9:19 am, Roger Pau Monne wrote:
> >> The call to ioremap_wc() in video_init() will always fail, because
> >> video_init() is called ahead of vm_init_type(), and so the underlying
> >> __vmap() call will fail to allocate the linear address space.
> >>
> >> Fix by reverting to the previous behavior and using ioremap() for the VGA
> >> text buffer.
> >>
> >> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > This is somewhat ugly.
> > 
> > ioremap() isn't really any better than ioremap_wc(); this only works
> > because plain ioremap() has a special case for the bottom 1M where it
> > does nothing and exits.
> 
> And this is exactly why I screwed up back then. Imo we would be better
> off moving to using __va() directly here. Otherwise the same mistake
> could be made again by someone (perhaps even me) noticing the less
> efficient ioremap(), when ioremap_wc() really would be wanted.

I can switch to using __va(), that's fine.  I guess we should then
remove the special casing for the low 1MB in ioremap()?

Thanks, Roger.

Re: [PATCH v2 2/5] x86/vga: fix mapping of the VGA text buffer

[Jan Beulich]

On 18.03.2025 16:31, Roger Pau Monné wrote:
> On Tue, Mar 18, 2025 at 03:28:32PM +0100, Jan Beulich wrote:
>> On 18.03.2025 14:11, Andrew Cooper wrote:
>>> On 18/03/2025 9:19 am, Roger Pau Monne wrote:
>>>> The call to ioremap_wc() in video_init() will always fail, because
>>>> video_init() is called ahead of vm_init_type(), and so the underlying
>>>> __vmap() call will fail to allocate the linear address space.
>>>>
>>>> Fix by reverting to the previous behavior and using ioremap() for the VGA
>>>> text buffer.
>>>>
>>>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> This is somewhat ugly.
>>>
>>> ioremap() isn't really any better than ioremap_wc(); this only works
>>> because plain ioremap() has a special case for the bottom 1M where it
>>> does nothing and exits.
>>
>> And this is exactly why I screwed up back then. Imo we would be better
>> off moving to using __va() directly here. Otherwise the same mistake
>> could be made again by someone (perhaps even me) noticing the less
>> efficient ioremap(), when ioremap_wc() really would be wanted.
> 
> I can switch to using __va(), that's fine.  I guess we should then
> remove the special casing for the low 1MB in ioremap()?

I'm not sure we can - DMI and ACPI might use that, and who knows what
else. But of course if you're sure nothing depends on that anymore ...

Jan

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

[Roger Pau Monne]

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(-)

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);
-- 
2.48.1

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

[Andrew Cooper]

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>

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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.

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

[PATCH v2 4/5] x86/shadow: fix UB pointer arithmetic in sh_mfn_is_a_page_table()

[Roger Pau Monne]

UBSAN complains with:

UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30
pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0
[...]
Xen call trace:
   [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
   [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100
   [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350
   [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450
   [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15

Fix by moving the call to mfn_to_page() after the check of whether the
passed gmfn is valid.  This avoid the call to mfn_to_page() with an
INVALID_MFN parameter.

While there make the page local variable const, it's not modified by the
function.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/mm/shadow/private.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h
index a5fc3a7676eb..cef9dbef2e77 100644
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -512,13 +512,14 @@ static inline unsigned long __backpointer(const struct page_info *sp)
 static inline int
 sh_mfn_is_a_page_table(mfn_t gmfn)
 {
-    struct page_info *page = mfn_to_page(gmfn);
+    const struct page_info *page;
     struct domain *owner;
     unsigned long type_info;
 
     if ( !mfn_valid(gmfn) )
         return 0;
 
+    page = mfn_to_page(gmfn);
     owner = page_get_owner(page);
     if ( owner && shadow_mode_refcounts(owner)
          && (page->count_info & PGC_shadowed_pt) )
-- 
2.48.1

Re: [PATCH v2 4/5] x86/shadow: fix UB pointer arithmetic in sh_mfn_is_a_page_table()

[Andrew Cooper]

On 18/03/2025 9:19 am, Roger Pau Monne wrote:
> UBSAN complains with:
>
> UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30
> pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0
> [...]
> Xen call trace:
>    [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
>    [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100
>    [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350
>    [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450
>    [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15

Something is definitely wonky in this backtrace.

>
> Fix by moving the call to mfn_to_page() after the check of whether the
> passed gmfn is valid.  This avoid the call to mfn_to_page() with an
> INVALID_MFN parameter.
>
> While there make the page local variable const, it's not modified by the
> function.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Whatever is wonky in the backtrace isn't related to this patch, so
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace
does want fixing.

Re: [PATCH v2 4/5] x86/shadow: fix UB pointer arithmetic in sh_mfn_is_a_page_table()

[Jan Beulich]

On 18.03.2025 13:53, Andrew Cooper wrote:
> On 18/03/2025 9:19 am, Roger Pau Monne wrote:
>> UBSAN complains with:
>>
>> UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30
>> pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0
>> [...]
>> Xen call trace:
>>    [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
>>    [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100
>>    [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350
>>    [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450
>>    [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15
> 
> Something is definitely wonky in this backtrace.
> 
>>
>> Fix by moving the call to mfn_to_page() after the check of whether the
>> passed gmfn is valid.  This avoid the call to mfn_to_page() with an
>> INVALID_MFN parameter.
>>
>> While there make the page local variable const, it's not modified by the
>> function.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Whatever is wonky in the backtrace isn't related to this patch, so
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace
> does want fixing.

Right, but the fix may need to be in the tool chain. I'd be curious what
the symbol table looks like that this was created from. Roger, was this
linked with GNU ld or LLVM? Are the filename anomalies also visible in
the corresponding xen-syms.map?

Jan

Re: [PATCH v2 4/5] x86/shadow: fix UB pointer arithmetic in sh_mfn_is_a_page_table()

[Roger Pau Monné]

On Tue, Mar 18, 2025 at 03:36:45PM +0100, Jan Beulich wrote:
> On 18.03.2025 13:53, Andrew Cooper wrote:
> > On 18/03/2025 9:19 am, Roger Pau Monne wrote:
> >> UBSAN complains with:
> >>
> >> UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30
> >> pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0
> >> [...]
> >> Xen call trace:
> >>    [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
> >>    [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100
> >>    [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350
> >>    [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450
> >>    [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15
> > 
> > Something is definitely wonky in this backtrace.
> > 
> >>
> >> Fix by moving the call to mfn_to_page() after the check of whether the
> >> passed gmfn is valid.  This avoid the call to mfn_to_page() with an
> >> INVALID_MFN parameter.
> >>
> >> While there make the page local variable const, it's not modified by the
> >> function.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > Whatever is wonky in the backtrace isn't related to this patch, so
> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace
> > does want fixing.
> 
> Right, but the fix may need to be in the tool chain. I'd be curious what
> the symbol table looks like that this was created from. Roger, was this
> linked with GNU ld or LLVM? Are the filename anomalies also visible in
> the corresponding xen-syms.map?

It's with LLVM LD, it's this issue:

https://lore.kernel.org/xen-devel/20220505142137.51306-1-roger.pau@citrix.com/

I need to refresh that patch and resend.

Sorry, I got so used to those wonky filenames in the backtraces that I
no longer notice.

Thanks, Roger.

Re: [PATCH v2 4/5] x86/shadow: fix UB pointer arithmetic in sh_mfn_is_a_page_table()

[Roger Pau Monné]

On Tue, Mar 18, 2025 at 12:53:30PM +0000, Andrew Cooper wrote:
> On 18/03/2025 9:19 am, Roger Pau Monne wrote:
> > UBSAN complains with:
> >
> > UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30
> > pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0
> > [...]
> > Xen call trace:
> >    [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
> >    [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100
> >    [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350
> >    [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450
> >    [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15
> 
> Something is definitely wonky in this backtrace.

Oh, yes, it's a TODO I have pending when using LLVM LD.  I sent a fix
very long time ago, but it was quite ugly.

> >
> > Fix by moving the call to mfn_to_page() after the check of whether the
> > passed gmfn is valid.  This avoid the call to mfn_to_page() with an
> > INVALID_MFN parameter.
> >
> > While there make the page local variable const, it's not modified by the
> > function.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Whatever is wonky in the backtrace isn't related to this patch, so
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace
> does want fixing.

I can get the proper backtrace using clang + GNU LD.

Thanks, Roger.

[PATCH v2 5/5] kconfig/randconfig: enable UBSAN for randconfig

[Roger Pau Monne]

Introduce an additional Kconfig check to only offer the option if the
compiler supports -fsanitize=undefined.

We no longer use Travis CI, so the original motivation for not enabling
UBSAN might no longer present.  Regardless, the option won't be present in
the first place if the compiler doesn't support -fsanitize=undefined.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/Kconfig                        | 4 ++++
 xen/Kconfig.debug                  | 2 +-
 xen/tools/kconfig/allrandom.config | 1 -
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 72fdb8376087..2128f0ccfc0b 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -37,6 +37,10 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
 config CC_SPLIT_SECTIONS
 	bool
 
+# Compiler supports -fsanitize=undefined
+config CC_HAS_UBSAN
+	def_bool $(cc-option,-fsanitize=undefined)
+
 # Set code alignment.
 #
 # Allow setting on a boolean basis, and then convert such selection to an
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index c4a8d86912e0..f7cc5ffaabd7 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -98,7 +98,7 @@ config SCRUB_DEBUG
 
 config UBSAN
 	bool "Undefined behaviour sanitizer"
-	depends on HAS_UBSAN
+	depends on HAS_UBSAN && CC_HAS_UBSAN
 	help
 	  Enable undefined behaviour sanitizer. It uses compiler to insert code
 	  snippets so that undefined behaviours in C are detected during runtime.
diff --git a/xen/tools/kconfig/allrandom.config b/xen/tools/kconfig/allrandom.config
index 76f74320b5b0..c7753ac4addb 100644
--- a/xen/tools/kconfig/allrandom.config
+++ b/xen/tools/kconfig/allrandom.config
@@ -1,4 +1,3 @@
 # Explicit option choices not subject to regular RANDCONFIG
 
 CONFIG_GCOV_FORMAT_AUTODETECT=y
-CONFIG_UBSAN=n
-- 
2.48.1

Re: [PATCH v2 5/5] kconfig/randconfig: enable UBSAN for randconfig

[Andrew Cooper]

On 18/03/2025 9:19 am, Roger Pau Monne wrote:
> Introduce an additional Kconfig check to only offer the option if the
> compiler supports -fsanitize=undefined.
>
> We no longer use Travis CI, so the original motivation for not enabling
> UBSAN might no longer present.  Regardless, the option won't be present in
> the first place if the compiler doesn't support -fsanitize=undefined.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I can't remember quite what it was, but it was very early days and I
recall the toolchain cared about libubsan which wasn't in the Travis
environment, even though the end build didn't need it.

Either way, it's long obsolete now.