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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 18 May 2016 20:15:41 +0300
From:	Andrey Ryabinin <ryabinin.a.a@...il.com>
To:	Alan Stern <stern@...land.harvard.edu>
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

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;
>
> 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.

> 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
> @@ -872,9 +872,10 @@ 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];
> +       u32 __iomem     *status_reg = ehci->regs->port_status +
> +                               ((wIndex & 0xff) - 1);
> +       u32 __iomem     *hostpc_reg = ehci->regs->hostpc +
> +                               ((wIndex & 0xff) - 1);
>         u32             temp, temp1, status;
>         unsigned long   flags;
>         int             retval = 0;
>