This was reported by clang UBSAN as:

UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
applying zero offset to null pointer
[...]
Xen call trace:
    [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
    [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
    [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
    [<ffff82d0406c3728>] F video_init+0xd0/0x180
    [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
    [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
    [<ffff82d04020482e>] F __high_start+0x8e/0x90

Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
pointer from __vmap() is NULL.

Fixes: d0d4635d034f ('implement vmap()')
Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/dmi_scan.c | 7 +++++--
 xen/arch/x86/mm.c       | 6 ++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index 2fcc485295eb..a05492037519 100644
--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -112,6 +112,7 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
 {
     mfn_t mfn = _mfn(PFN_DOWN(addr));
     unsigned int offs = PAGE_OFFSET(addr);
+    void *va;
 
     if ( addr + len <= MB(1) )
         return __va(addr);
@@ -119,8 +120,10 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
     if ( system_state < SYS_STATE_boot )
         return __acpi_map_table(addr, len);
 
-    return __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
-                  VMAP_DEFAULT) + offs;
+    va = __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
+	        VMAP_DEFAULT);
+
+    return va ? va + offs : NULL;
 }
 
 static void __init bt_iounmap(const void *ptr, unsigned int len)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index bfdc8fb01949..03b8319f7a9d 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6277,7 +6277,9 @@ void __iomem *ioremap(paddr_t pa, size_t len)
         unsigned int offs = pa & (PAGE_SIZE - 1);
         unsigned int nr = PFN_UP(offs + len);
 
-        va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_UCMINUS, VMAP_DEFAULT) + offs;
+        va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_UCMINUS, VMAP_DEFAULT);
+        if ( va )
+            va += offs;
     }
 
     return (void __force __iomem *)va;
@@ -6294,7 +6296,7 @@ void __iomem *__init ioremap_wc(paddr_t pa, size_t len)
 
     va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_WC, VMAP_DEFAULT);
 
-    return (void __force __iomem *)(va + offs);
+    return (void __force __iomem *)(va ? va + offs : NULL);
 }
 
 int create_perdomain_mapping(struct domain *d, unsigned long va,
-- 
2.48.1

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> This was reported by clang UBSAN as:
>
> UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
> applying zero offset to null pointer
> [...]
> Xen call trace:
>     [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
>     [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
>     [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
>     [<ffff82d0406c3728>] F video_init+0xd0/0x180
>     [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
>     [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
>     [<ffff82d04020482e>] F __high_start+0x8e/0x90
>
> Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
> pointer from __vmap() is NULL.
>
> Fixes: d0d4635d034f ('implement vmap()')
> Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one style fix.

It's unfortunate, because C23 makes this one case (add 0 to NULL
pointer) explicitly well defined to avoid corner cases like this.  Oh well.

> diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> index 2fcc485295eb..a05492037519 100644
> --- a/xen/arch/x86/dmi_scan.c
> +++ b/xen/arch/x86/dmi_scan.c
> @@ -119,8 +120,10 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
>      if ( system_state < SYS_STATE_boot )
>          return __acpi_map_table(addr, len);
>  
> -    return __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> -                  VMAP_DEFAULT) + offs;
> +    va = __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> +	        VMAP_DEFAULT);

You've got mixed tabs/spaces here.

~Andrew

On Thu, Mar 13, 2025 at 05:21:13PM +0000, Andrew Cooper wrote:
> On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> > This was reported by clang UBSAN as:
> >
> > UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
> > applying zero offset to null pointer
> > [...]
> > Xen call trace:
> >     [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
> >     [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
> >     [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
> >     [<ffff82d0406c3728>] F video_init+0xd0/0x180
> >     [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
> >     [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
> >     [<ffff82d04020482e>] F __high_start+0x8e/0x90
> >
> > Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
> > pointer from __vmap() is NULL.
> >
> > Fixes: d0d4635d034f ('implement vmap()')
> > Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
> > Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one style fix.
> 
> It's unfortunate, because C23 makes this one case (add 0 to NULL
> pointer) explicitly well defined to avoid corner cases like this.  Oh well.

Interesting, so they added a new type (nullptr_t) that has a single
possible value (nullptr), and hence arithmetic operations against it
always result in nullptr.  That's helpful to prevent this kind of
bugs.

> > diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> > index 2fcc485295eb..a05492037519 100644
> > --- a/xen/arch/x86/dmi_scan.c
> > +++ b/xen/arch/x86/dmi_scan.c
> > @@ -119,8 +120,10 @@ static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
> >      if ( system_state < SYS_STATE_boot )
> >          return __acpi_map_table(addr, len);
> >  
> > -    return __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> > -                  VMAP_DEFAULT) + offs;
> > +    va = __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
> > +	        VMAP_DEFAULT);
> 
> You've got mixed tabs/spaces here.

Thanks, vim autodetection is a bit confused with this file because it
uses both hard and soft tabs, fixed now.

Roger.

On 14/03/2025 8:43 am, Roger Pau Monné wrote:
> On Thu, Mar 13, 2025 at 05:21:13PM +0000, Andrew Cooper wrote:
>> On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
>>> This was reported by clang UBSAN as:
>>>
>>> UBSAN: Undefined behaviour in arch/x86/mm.c:6297:40
>>> applying zero offset to null pointer
>>> [...]
>>> Xen call trace:
>>>     [<ffff82d040303662>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0
>>>     [<ffff82d040304aa3>] F __ubsan_handle_pointer_overflow+0xcb/0x100
>>>     [<ffff82d0406ebbc0>] F ioremap_wc+0xc8/0xe0
>>>     [<ffff82d0406c3728>] F video_init+0xd0/0x180
>>>     [<ffff82d0406ab6f5>] F console_init_preirq+0x3d/0x220
>>>     [<ffff82d0406f1876>] F __start_xen+0x68e/0x5530
>>>     [<ffff82d04020482e>] F __high_start+0x8e/0x90
>>>
>>> Fix bt_ioremap() and ioremap{,_wc}() to not add the offset if the returned
>>> pointer from __vmap() is NULL.
>>>
>>> Fixes: d0d4635d034f ('implement vmap()')
>>> Fixes: f390941a92f1 ('x86/DMI: fix table mapping when one lives above 1Mb')
>>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one style fix.
>>
>> It's unfortunate, because C23 makes this one case (add 0 to NULL
>> pointer) explicitly well defined to avoid corner cases like this.  Oh well.
> Interesting, so they added a new type (nullptr_t) that has a single
> possible value (nullptr), and hence arithmetic operations against it
> always result in nullptr.  That's helpful to prevent this kind of
> bugs.

nullptr_t is unrelated.  That's for _Generic() and friends.

I'm struggling to find the reference to NULL + 0 being made well
defined.  It was in the context of library implementations of memset()/etc.

Nevertheless, we've got to cope with it, given our current -std.

~Andrew