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]

Message-ID: <CAPAsAGxQPS2TwmxJx9cPpEUoUEkxMjfw+nfOTTiqHXfJK4h65g@mail.gmail.com>
Date:	Thu, 19 May 2016 19:29:02 +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 22:28 GMT+03:00 Alan Stern <stern@...land.harvard.edu>:
> 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.
>

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.


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