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.
> > Really? That's a lot simpler. But will it also fool gcc? That is,
> > will it prevent gcc from optimizing away the !wIndex tests below?
> >
>
> This only fools ubsan, but it's still undefined behavior => checks
> could be optimized away,
> but it seems that current gcc(5.3.0) doesn't do this yet:
>
> $ cat test.c
> int a[10];
>
> int test(int i) {
> int *p = &a[i & 0xff - 1];
>
> if (!i)
> return 100;
> else
> return *p + 10;
> }
>
> $ gcc -O3 -c test.c
> $ objdump -d test.o
>
>
> 0000000000000000 <test>:
> 0: 85 ff test %edi,%edi
> 2: b8 64 00 00 00 mov $0x64,%eax
> 7: 75 07 jne 10 <test+0x10>
> 9: f3 c3 repz retq
> b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
> 10: 81 e7 fe 00 00 00 and $0xfe,%edi
> 16: 8b 04 bd 00 00 00 00 mov 0x0(,%rdi,4),%eax
> 1d: 83 c0 0a add $0xa,%eax
> 20: c3 retq
>
>
> > How about this patch?
> >
>
> 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?
Alan Stern
Index: usb-4.x/drivers/usb/host/ehci-hub.c
===================================================================
--- usb-4.x.orig/drivers/usb/host/ehci-hub.c
+++ usb-4.x/drivers/usb/host/ehci-hub.c
@@ -873,8 +873,9 @@ int ehci_hub_control(
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
int ports = HCS_N_PORTS (ehci->hcs_params);
u32 __iomem *status_reg = &ehci->regs->port_status[
- (wIndex & 0xff) - 1];
- u32 __iomem *hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
+ ((int) wIndex & 0xff) - 1];
+ u32 __iomem *hostpc_reg = &ehci->regs->hostpc[
+ ((int) wIndex & 0xff) - 1];
u32 temp, temp1, status;
unsigned long flags;
int retval = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
