[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 5/7] x86/ioremap: prevent additions against the NULL pointer



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@xxxxxxxxxx>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, 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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.