UBSAN whinge in ihci-hub.c

UBSAN whinge in ihci-hub.c

[Valdis Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 6346 bytes --]

So, not content in the amount of breakage I generate already, I
compiled with UBSAN enabled...

The immediately relevant part:

[    2.418576] ================================================================================
[    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
[    2.418582] index -1 is out of range for type 'u32 [1]'

The code there:

    875         u32 __iomem     *status_reg = &ehci->regs->port_status[
    876                                 (wIndex & 0xff) - 1];
    877         u32 __iomem     *hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
    878         u32             temp, temp1, status;

I'm guessing that the only reason that port_status[] didn't throw an error
because that's declared as 'u32 port_status[0]' with a 'u32 reserved3[9]'
behind it, while it's 'u32 hostpc[1]'.   So we have (possibly) 2 bugs:

1) hostpc should possibly be a 'u32 hostpc[0]'  I'd attach a patch, except
I'm low on caffeine and unsure if the 'u32 reserved5[16]' that follows needs
to be a [17] to compensate.  Either that, or port_status[] and hostpc[]
should *both* be explicitly sized so range-checking works better.

2) We need to figure out who passed a 0 wIndex down the stack, resulting in
the busted indexing...

The entire splat:

[    2.418567] hub 1-0:1.0: USB hub found
[    2.418576] ================================================================================
[    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
[    2.418582] index -1 is out of range for type 'u32 [1]'
[    2.418587] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.6.0-next-20160517-00001-gede618fce89c-dirty #279
[    2.418590] Hardware name: Dell Inc. Latitude E6530/07Y85M, BIOS A17 08/19/2015
[    2.418599]  0000000000000000 00000000ac1ab326 ffff88022ca232e8 ffffffffaa690aea
[    2.418605]  ffff88022ca23300 00000000ac1ab326 ffff88022ca23310 ffffffffffffffff
[    2.418613]  ffff88022ca23300 ffffffffaa7087ad ffffffffabd5aee0 ffff88022ca23358
[    2.418615] Call Trace:
[    2.418628]  [<ffffffffaa690aea>] dump_stack+0x7b/0xd1
[    2.418633]  [<ffffffffaa7087ad>] ubsan_epilogue+0xd/0x40
[    2.418639]  [<ffffffffaa708ec5>] __ubsan_handle_out_of_bounds+0x75/0xa0
[    2.418644]  [<ffffffffaa00312d>] ? syscall_slow_exit_work+0x1ed/0x310
[    2.418649]  [<ffffffffaa98b7b8>] ? usb_hcd_submit_urb+0x368/0xe00
[    2.418655]  [<ffffffffaa9b4dac>] ehci_hub_control+0xd9c/0xdc0
[    2.418662]  [<ffffffffaa98bd01>] usb_hcd_submit_urb+0x8b1/0xe00
[    2.418667]  [<ffffffffaa98da01>] usb_submit_urb+0x2e1/0x8f0
[    2.418672]  [<ffffffffaa132a72>] ? __init_waitqueue_head+0x52/0xa0
[    2.418677]  [<ffffffffaa98eced>] usb_start_wait_urb+0x7d/0x130
[    2.418682]  [<ffffffffaa98ee7c>] usb_control_msg+0xdc/0x120
[    2.418691]  [<ffffffffaa9861a9>] hub_probe+0x4e9/0x1110
[    2.418696]  [<ffffffffab077f27>] ? _raw_spin_unlock_irqrestore+0x87/0x90
[    2.418701]  [<ffffffffaa0f8bda>] ? preempt_count_sub+0x4a/0x90
[    2.418706]  [<ffffffffab077f14>] ? _raw_spin_unlock_irqrestore+0x74/0x90
[    2.418711]  [<ffffffffaa995369>] usb_probe_interface+0x139/0x3e0
[    2.418717]  [<ffffffffaa86b011>] driver_probe_device+0x131/0x3c0
[    2.418723]  [<ffffffffaa86b463>] __device_attach_driver+0xc3/0x180
[    2.418728]  [<ffffffffaa86b3a0>] ? __driver_attach+0x100/0x100
[    2.418732]  [<ffffffffaa867f1d>] bus_for_each_drv+0x8d/0x100
[    2.418737]  [<ffffffffaa86ad98>] __device_attach+0xe8/0x170
[    2.418742]  [<ffffffffaa86b583>] device_initial_probe+0x13/0x20
[    2.418746]  [<ffffffffaa869a67>] bus_probe_device+0xe7/0x150
[    2.418750]  [<ffffffffaa8669cb>] device_add+0x49b/0x690
[    2.418756]  [<ffffffffaa991f2b>] usb_set_configuration+0x5bb/0xc80
[    2.418762]  [<ffffffffaa9a4856>] generic_probe+0x36/0xa0
[    2.418766]  [<ffffffffaa9951eb>] usb_probe_device+0x3b/0x80
[    2.418770]  [<ffffffffaa86b011>] driver_probe_device+0x131/0x3c0
[    2.418775]  [<ffffffffaa86b463>] __device_attach_driver+0xc3/0x180
[    2.418779]  [<ffffffffaa86b3a0>] ? __driver_attach+0x100/0x100
[    2.418783]  [<ffffffffaa867f1d>] bus_for_each_drv+0x8d/0x100
[    2.418788]  [<ffffffffaa86ad98>] __device_attach+0xe8/0x170
[    2.418793]  [<ffffffffaa86b583>] device_initial_probe+0x13/0x20
[    2.418797]  [<ffffffffaa869a67>] bus_probe_device+0xe7/0x150
[    2.418801]  [<ffffffffaa8669cb>] device_add+0x49b/0x690
[    2.418807]  [<ffffffffaa9828c9>] usb_new_device+0x319/0x970
[    2.418812]  [<ffffffffaa98a01b>] usb_add_hcd+0x67b/0xa40
[    2.418817]  [<ffffffffaa9a86d3>] usb_hcd_pci_probe+0x4c3/0x770
[    2.418822]  [<ffffffffaa1464c6>] ? trace_hardirqs_on_caller+0x16/0x2c0
[    2.418827]  [<ffffffffaa0f8bda>] ? preempt_count_sub+0x4a/0x90
[    2.418832]  [<ffffffffaa9c1256>] ehci_pci_probe+0x36/0x40
[    2.418837]  [<ffffffffaa71e5fc>] pci_device_probe+0xdc/0x180
[    2.418842]  [<ffffffffaa86b011>] driver_probe_device+0x131/0x3c0
[    2.418846]  [<ffffffffaa86b359>] __driver_attach+0xb9/0x100
[    2.418851]  [<ffffffffaa86b2a0>] ? driver_probe_device+0x3c0/0x3c0
[    2.418855]  [<ffffffffaa867e0a>] bus_for_each_dev+0x8a/0xf0
[    2.418860]  [<ffffffffaa86a537>] driver_attach+0x27/0x50
[    2.418864]  [<ffffffffaa869e16>] bus_add_driver+0x116/0x2b0
[    2.418868]  [<ffffffffaa86bb6f>] driver_register+0x9f/0x160
[    2.418873]  [<ffffffffaa71d44f>] __pci_register_driver+0x8f/0xe0
[    2.418879]  [<ffffffffac37d5db>] ? ehci_hcd_init+0x90/0x90
[    2.418885]  [<ffffffffac37d640>] ehci_pci_init+0x65/0x67
[    2.418890]  [<ffffffffaa00043f>] do_one_initcall+0x5f/0x210
[    2.418896]  [<ffffffffac320848>] kernel_init_freeable+0x33d/0x3d4
[    2.418903]  [<ffffffffab069e6f>] kernel_init+0xf/0x120
[    2.418907]  [<ffffffffab07897f>] ret_from_fork+0x1f/0x40
[    2.418911]  [<ffffffffab069e60>] ? rest_init+0x170/0x170
[    2.418915] ================================================================================
[    2.418934] hub 1-0:1.0: 2 ports detected
[    2.419850] ehci-pci 0000:00:1d.0: EHCI Host Controller
[    2.419993] ehci-pci 0000:00:1d.0: new USB bus registered, assigned bus number 2
[    2.420031] ehci-pci 0000:00:1d.0: debug port 2
[    2.423961] ehci-pci 0000:00:1d.0: cache line size of 64 is not supported
[    2.423998] ehci-pci 0000:00:1d.0: irq 21, io mem 0xf7737000
[    2.430045] ehci-pci 0000:00:1d.0: USB 2.0 started, EHCI 1.00


[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

Re: UBSAN whinge in ihci-hub.c

[Greg Kroah-Hartman]

On Tue, May 17, 2016 at 05:52:40PM -0400, Valdis Kletnieks wrote:
> So, not content in the amount of breakage I generate already, I
> compiled with UBSAN enabled...
> 
> The immediately relevant part:
> 
> [    2.418576] ================================================================================
> [    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> [    2.418582] index -1 is out of range for type 'u32 [1]'

<snip>

It's a known bug in ubsan, please see the linux-usb archives for the
details :(

Re: UBSAN whinge in ihci-hub.c

[Andrey Ryabinin]

2016-05-18 1:16 GMT+03:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> On Tue, May 17, 2016 at 05:52:40PM -0400, Valdis Kletnieks wrote:
>> So, not content in the amount of breakage I generate already, I
>> compiled with UBSAN enabled...
>>
>> The immediately relevant part:
>>
>> [    2.418576] ================================================================================
>> [    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
>> [    2.418582] index -1 is out of range for type 'u32 [1]'
>
> <snip>
>
> It's a known bug in ubsan,

It's not a bug.  int *p = &a[-1] is undefined behavior. It doesn't
matter whether that pointer dereferenced or not.

Re: UBSAN whinge in ihci-hub.c

[Oliver Neukum]

On Wed, 2016-05-18 at 10:40 +0300, Andrey Ryabinin wrote:
> 2016-05-18 1:16 GMT+03:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> > On Tue, May 17, 2016 at 05:52:40PM -0400, Valdis Kletnieks wrote:
> >> So, not content in the amount of breakage I generate already, I
> >> compiled with UBSAN enabled...
> >>
> >> The immediately relevant part:
> >>
> >> [    2.418576] ================================================================================
> >> [    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> >> [    2.418582] index -1 is out of range for type 'u32 [1]'
> >
> > <snip>
> >
> > It's a known bug in ubsan,
> 
> It's not a bug.  int *p = &a[-1] is undefined behavior. It doesn't
> matter whether that pointer dereferenced or not.

That is a bold statement. Pointer arithmetic is defined. How can
the computation of an address be undefined behavior while it is
not used?

	Regards
		Oliver

Re: UBSAN whinge in ihci-hub.c

[Andrey Ryabinin]

2016-05-18 11:18 GMT+03:00 Oliver Neukum <oneukum@suse.com>:
> On Wed, 2016-05-18 at 10:40 +0300, Andrey Ryabinin wrote:
>> 2016-05-18 1:16 GMT+03:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>> > On Tue, May 17, 2016 at 05:52:40PM -0400, Valdis Kletnieks wrote:
>> >> So, not content in the amount of breakage I generate already, I
>> >> compiled with UBSAN enabled...
>> >>
>> >> The immediately relevant part:
>> >>
>> >> [    2.418576] ================================================================================
>> >> [    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
>> >> [    2.418582] index -1 is out of range for type 'u32 [1]'
>> >
>> > <snip>
>> >
>> > It's a known bug in ubsan,
>>
>> It's not a bug.  int *p = &a[-1] is undefined behavior. It doesn't
>> matter whether that pointer dereferenced or not.
>
> That is a bold statement. Pointer arithmetic is defined. How can
> the computation of an address be undefined behavior while it is
> not used?

It's defined only if pointer points to array element or one-past-end
element. Everything else is undefined.

$ 6.5.6.8
   "If both the pointer operand and the result point to elements of
the same array object,
     or one past the last element of the array object, the evaluation
shall not produce an overflow;
     otherwise, the behavior is undefined."

Here is a good example of how bad this could be -
https://lwn.net/Articles/278137/

So, in case of ehci_hub_control(), gcc is allowed to assume that
wIndex is never 0, and
"optimize" away !wIndex check from this code:

   if (!wIndex || wIndex > ports)
        goto error;

Re: UBSAN whinge in ihci-hub.c

[Oliver Neukum]

On Wed, 2016-05-18 at 12:16 +0300, Andrey Ryabinin wrote:
> 2016-05-18 11:18 GMT+03:00 Oliver Neukum <oneukum@suse.com>:
> > On Wed, 2016-05-18 at 10:40 +0300, Andrey Ryabinin wrote:
> >> 2016-05-18 1:16 GMT+03:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >> > On Tue, May 17, 2016 at 05:52:40PM -0400, Valdis Kletnieks wrote:
> >> >> So, not content in the amount of breakage I generate already, I
> >> >> compiled with UBSAN enabled...
> >> >>
> >> >> The immediately relevant part:
> >> >>
> >> >> [    2.418576] ================================================================================
> >> >> [    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> >> >> [    2.418582] index -1 is out of range for type 'u32 [1]'
> >> >
> >> > <snip>
> >> >
> >> > It's a known bug in ubsan,
> >>
> >> It's not a bug.  int *p = &a[-1] is undefined behavior. It doesn't
> >> matter whether that pointer dereferenced or not.
> >
> > That is a bold statement. Pointer arithmetic is defined. How can
> > the computation of an address be undefined behavior while it is
> > not used?
> 
> It's defined only if pointer points to array element or one-past-end
> element. Everything else is undefined.
> 
> $ 6.5.6.8
>    "If both the pointer operand and the result point to elements of
> the same array object,
>      or one past the last element of the array object, the evaluation
> shall not produce an overflow;
>      otherwise, the behavior is undefined."

But we do not care whether the calculation overflows. We don't use it
at all in those cases.

	Regards
		Oliver

Re: UBSAN whinge in ihci-hub.c

[Andrey Ryabinin]

2016-05-18 13:19 GMT+03:00 Oliver Neukum <oneukum@suse.com>:
> On Wed, 2016-05-18 at 12:16 +0300, Andrey Ryabinin wrote:
>> 2016-05-18 11:18 GMT+03:00 Oliver Neukum <oneukum@suse.com>:
>> > On Wed, 2016-05-18 at 10:40 +0300, Andrey Ryabinin wrote:
>> >> 2016-05-18 1:16 GMT+03:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>> >> > On Tue, May 17, 2016 at 05:52:40PM -0400, Valdis Kletnieks wrote:
>> >> >> So, not content in the amount of breakage I generate already, I
>> >> >> compiled with UBSAN enabled...
>> >> >>
>> >> >> The immediately relevant part:
>> >> >>
>> >> >> [    2.418576] ================================================================================
>> >> >> [    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
>> >> >> [    2.418582] index -1 is out of range for type 'u32 [1]'
>> >> >
>> >> > <snip>
>> >> >
>> >> > It's a known bug in ubsan,
>> >>
>> >> It's not a bug.  int *p = &a[-1] is undefined behavior. It doesn't
>> >> matter whether that pointer dereferenced or not.
>> >
>> > That is a bold statement. Pointer arithmetic is defined. How can
>> > the computation of an address be undefined behavior while it is
>> > not used?
>>
>> It's defined only if pointer points to array element or one-past-end
>> element. Everything else is undefined.
>>
>> $ 6.5.6.8
>>    "If both the pointer operand and the result point to elements of
>> the same array object,
>>      or one past the last element of the array object, the evaluation
>> shall not produce an overflow;
>>      otherwise, the behavior is undefined."
>
> But we do not care whether the calculation overflows. We don't use it
> at all in those cases.
>

This doesn't make it defined. Also that pointer is unused only if gcc
doesn't optimize away '!wIndex' check.
If it does,  we may actually use it.

Re: UBSAN whinge in ihci-hub.c

[Alan Stern]

On Wed, 18 May 2016, Andrey Ryabinin wrote:

> 2016-05-18 13:19 GMT+03:00 Oliver Neukum <oneukum@suse.com>:
> > On Wed, 2016-05-18 at 12:16 +0300, Andrey Ryabinin wrote:
> >> 2016-05-18 11:18 GMT+03:00 Oliver Neukum <oneukum@suse.com>:
> >> > On Wed, 2016-05-18 at 10:40 +0300, Andrey Ryabinin wrote:
> >> >> 2016-05-18 1:16 GMT+03:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> >> >> > On Tue, May 17, 2016 at 05:52:40PM -0400, Valdis Kletnieks wrote:
> >> >> >> So, not content in the amount of breakage I generate already, I
> >> >> >> compiled with UBSAN enabled...
> >> >> >>
> >> >> >> The immediately relevant part:
> >> >> >>
> >> >> >> [    2.418576] ================================================================================
> >> >> >> [    2.418579] UBSAN: Undefined behaviour in drivers/usb/host/ehci-hub.c:877:47
> >> >> >> [    2.418582] index -1 is out of range for type 'u32 [1]'
> >> >> >
> >> >> > <snip>
> >> >> >
> >> >> > It's a known bug in ubsan,
> >> >>
> >> >> It's not a bug.  int *p = &a[-1] is undefined behavior. It doesn't
> >> >> matter whether that pointer dereferenced or not.
> >> >
> >> > That is a bold statement. Pointer arithmetic is defined. How can
> >> > the computation of an address be undefined behavior while it is
> >> > not used?
> >>
> >> It's defined only if pointer points to array element or one-past-end
> >> element. Everything else is undefined.
> >>
> >> $ 6.5.6.8
> >>    "If both the pointer operand and the result point to elements of
> >> the same array object,
> >>      or one past the last element of the array object, the evaluation
> >> shall not produce an overflow;
> >>      otherwise, the behavior is undefined."
> >
> > But we do not care whether the calculation overflows. We don't use it
> > at all in those cases.
> >
> 
> This doesn't make it defined. Also that pointer is unused only if gcc
> doesn't optimize away '!wIndex' check.
> If it does,  we may actually use it.

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.

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,14 +872,17 @@ 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, *hostpc_reg;
 	u32		temp, temp1, status;
 	unsigned long	flags;
 	int		retval = 0;
 	unsigned	selector;
 
+	temp = wIndex & 0xff;
+	temp -= (temp > 0);
+	status_reg = &ehci->regs->port_status[temp];
+	hostpc_reg = &ehci->regs->hostpc[temp];
+
 	/*
 	 * FIXME:  support SetPortFeatures USB_PORT_FEAT_INDICATOR.
 	 * HCS_INDICATOR may say we can change LEDs to off/amber/green.

Re: UBSAN whinge in ihci-hub.c

[Andrey Ryabinin]

2016-05-18 17:40 GMT+03:00 Alan Stern <stern@rowland.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;



> 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,14 +872,17 @@ 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, *hostpc_reg;
>         u32             temp, temp1, status;
>         unsigned long   flags;
>         int             retval = 0;
>         unsigned        selector;
>
> +       temp = wIndex & 0xff;
> +       temp -= (temp > 0);
> +       status_reg = &ehci->regs->port_status[temp];
> +       hostpc_reg = &ehci->regs->hostpc[temp];
> +
>         /*
>          * FIXME:  support SetPortFeatures USB_PORT_FEAT_INDICATOR.
>          * HCS_INDICATOR may say we can change LEDs to off/amber/green.
>

Re: UBSAN whinge in ihci-hub.c

[Alan Stern]

On Wed, 18 May 2016, Andrey Ryabinin wrote:

> 2016-05-18 17:40 GMT+03:00 Alan Stern <stern@rowland.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?

How about this patch?

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;

Re: UBSAN whinge in ihci-hub.c

[Andrey Ryabinin]

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

Re: UBSAN whinge in ihci-hub.c

[Alan Stern]

On Wed, 18 May 2016, Andrey Ryabinin wrote:

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

Re: UBSAN whinge in ihci-hub.c

[Andrey Ryabinin]

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

Re: UBSAN whinge in ihci-hub.c

[Alan Stern]

On Thu, 19 May 2016, Andrey Ryabinin wrote:

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

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

Thanks for your comments,

Alan Stern

RE: UBSAN whinge in ihci-hub.c

[David Laight]

From: Andrey Ryabinin
> Sent: 18 May 2016 13:21
...
> >> $ 6.5.6.8
> >>    "If both the pointer operand and the result point to elements of
> >> the same array object,
> >>      or one past the last element of the array object, the evaluation
> >> shall not produce an overflow;
> >>      otherwise, the behavior is undefined."
> >
> > But we do not care whether the calculation overflows. We don't use it
> > at all in those cases.
> >
> 
> This doesn't make it defined. Also that pointer is unused only if gcc
> doesn't optimize away '!wIndex' check.
> If it does,  we may actually use it.

The compiler is allowed to generate the pointer and load it into
a 'pointer register'. On a hardware that has fat pointers and where
the hardware validates the bounds (&foo - 1) can fault.

The most recent hardware that does that is probably a vax.
Although I believe amd64 will fault if you load a suitable invalid
value (not a valid pointer) into the fs/gs offset registers.

	David