Re: UBSAN whinge in ihci-hub.c

From: Alan Stern
Date: Thu May 19 2016 - 16:11:09 EST


On Thu, 19 May 2016, Andrey Ryabinin wrote:

> 2016-05-18 22:28 GMT+03:00 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
> > On Wed, 18 May 2016, Andrey Ryabinin wrote:
> >
> >> 2016-05-18 19:09 GMT+03:00 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
> >> > On Wed, 18 May 2016, Andrey Ryabinin wrote:
> >> >
> >> >> 2016-05-18 17:40 GMT+03:00 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>:
> >> >>
> >> >> > All right, I'm getting very tired of all these bug reports. Besides,
> >> >> > Andrey has a point: Unless you're Linus, arguing against the C standard
> >> >> > is futile. (Even though the language dialect used in the kernel is not
> >> >> > standard C.)
> >> >> >
> >> >> > Does this patch make UBSAN happy? The runtime overhead is minimal.
> >> >> >
> >> >>
> >> >> It does. However, you could fool ubsan way more easy:
> >> >> u32 __iomem *hostpc_reg = ehci->regs->hostpc +
> >> >> (wIndex & 0xff) - 1;
> >
> > This probably should be considered to be a bug in UBSAN. It ought to
> > treat pointer addition the same as index addition.
> >
>
> It's more like a missing feature. UBSAN doesn't guarantee that every possible
> UB will be detected.
>
>
> >>
> >> So it silences UBSAN, but still undefined.
> >> I think it's up to you to decide - more code churn or undefined behavior.
> >
> > Well, I don't want the compiler to eliminate code that's necessary.
> >
> > On the other hand, it's not clear how much we need to worry about the
> > standard. After all, zero-length arrays are a GNU extension to C.
> > Since the array objects in question are defined like this:
> >
> > u32 port_status[0]; /* up to N_PORTS */
> >
> > it's hard to guess what the compiler will think about out-of-bounds
> > pointer values.
> >
> > Maybe the best thing to do is eliminate the underflow while leaving the
> > calculation unchanged. What does UBSAN think about this? Does it
> > dislike -1 as an index value as much as it dislikes -1u?
>
> Type of doesn't change anything here.

Okay, then I'll go back to the more verbose but guaranteed safe patch.

Thanks for your comments,

Alan Stern