linux-kernel - Re: UBSAN whinge in ihci-hub.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 May 2016 15:28:16 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Andrey Ryabinin <ryabinin.a.a@...il.com>
cc:	Oliver Neukum <oneukum@...e.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	LKML <linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	Valdis Kletnieks <Valdis.Kletnieks@...edu>
Subject: Re: UBSAN whinge in ihci-hub.c

On Wed, 18 May 2016, Andrey Ryabinin wrote:

> 2016-05-18 19:09 GMT+03:00 Alan Stern <stern@...land.harvard.edu>:
> > On Wed, 18 May 2016, Andrey Ryabinin wrote:
> >
> >> 2016-05-18 17:40 GMT+03:00 Alan Stern <stern@...land.harvard.edu>:
> >>
> >> > 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;