[PATCH v2] usb/core: usb_alloc_dev(): fix setting of ->portnum

[PATCH v2] usb/core: usb_alloc_dev(): fix setting of ->portnum

[Nicolai Stange]

With commit 69bec7259853 ("USB: core: let USB device know device node"),
the port1 argument of usb_alloc_dev() gets overwritten as follows:

  ... usb_alloc_dev(..., unsigned port1)
  {
    ...
    if (!parent->parent) {
      port1 = usb_hcd_find_raw_port_number(..., port1);
    }
    ...
  }

Later on, this now overwritten port1 gets assigned to ->portnum:

  dev->portnum = port1;

However, since xhci_find_raw_port_number() isn't idempotent, the
aforementioned commit causes a number of KASAN splats like the following:

  BUG: KASAN: slab-out-of-bounds in xhci_find_raw_port_number+0x98/0x170
                                       at addr ffff8801d9311670
  Read of size 8 by task kworker/2:1/87
  [...]
  Workqueue: usb_hub_wq hub_event
   0000000000000188 000000005814b877 ffff8800cba17588 ffffffff8191447e
   0000000041b58ab3 ffffffff82a03209 ffffffff819143a2 ffffffff82a252f4
   ffff8801d93115e0 0000000000000188 ffff8801d9311628 ffff8800cba17588
  Call Trace:
   [<ffffffff8191447e>] dump_stack+0xdc/0x15e
   [<ffffffff819143a2>] ? _atomic_dec_and_lock+0xa2/0xa2
   [<ffffffff814e2cd1>] ? print_section+0x61/0xb0
   [<ffffffff814e4939>] print_trailer+0x179/0x2c0
   [<ffffffff814f0d84>] object_err+0x34/0x40
   [<ffffffff814f4388>] kasan_report_error+0x2f8/0x8b0
   [<ffffffff814eb91e>] ? __slab_alloc+0x5e/0x90
   [<ffffffff812178c0>] ? __lock_is_held+0x90/0x130
   [<ffffffff814f5091>] kasan_report+0x71/0xa0
   [<ffffffff814ec082>] ? kmem_cache_alloc_trace+0x212/0x560
   [<ffffffff81d99468>] ? xhci_find_raw_port_number+0x98/0x170
   [<ffffffff814f33d4>] __asan_load8+0x64/0x70
   [<ffffffff81d99468>] xhci_find_raw_port_number+0x98/0x170
   [<ffffffff81db0105>] xhci_setup_addressable_virt_dev+0x235/0xa10
   [<ffffffff81d9ea51>] xhci_setup_device+0x3c1/0x1430
   [<ffffffff8121cddd>] ? trace_hardirqs_on+0xd/0x10
   [<ffffffff81d9fac0>] ? xhci_setup_device+0x1430/0x1430
   [<ffffffff81d9fad3>] xhci_address_device+0x13/0x20
   [<ffffffff81d2081a>] hub_port_init+0x55a/0x1550
   [<ffffffff81d28705>] hub_event+0xef5/0x24d0
   [<ffffffff81d27810>] ? hub_port_debounce+0x2f0/0x2f0
   [<ffffffff8195e1ee>] ? debug_object_deactivate+0x1be/0x270
   [<ffffffff81210203>] ? print_rt_rq+0x53/0x2d0
   [<ffffffff8121657d>] ? trace_hardirqs_off+0xd/0x10
   [<ffffffff8226acfb>] ? _raw_spin_unlock_irqrestore+0x5b/0x60
   [<ffffffff81250000>] ? irq_domain_set_hwirq_and_chip+0x30/0xb0
   [<ffffffff81256339>] ? debug_lockdep_rcu_enabled+0x39/0x40
   [<ffffffff812178c0>] ? __lock_is_held+0x90/0x130
   [<ffffffff81196877>] process_one_work+0x567/0xec0
  [...]

Afterwards, xhci reports some functional errors:

  xhci_hcd 0000:00:14.0: ERROR: unexpected setup address command completion
                                code 0x11.
  xhci_hcd 0000:00:14.0: ERROR: unexpected setup address command completion
                                code 0x11.
  usb 4-3: device not accepting address 2, error -22

Fix this by not overwriting the port1 argument in usb_alloc_dev(), but
storing the raw port number as required by OF in an additional variable,
raw_port.

Fixes: 69bec7259853 ("USB: core: let USB device know device node")
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 Applicable to linux-next-20160317

 Changes to v1:
 - Initialize raw_port with port1

 drivers/usb/core/usb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index ffa5cf1..dcb85e3 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -424,6 +424,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
 	struct usb_device *dev;
 	struct usb_hcd *usb_hcd = bus_to_hcd(bus);
 	unsigned root_hub = 0;
+	unsigned raw_port = port1;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
@@ -498,11 +499,11 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
 
 		if (!parent->parent) {
 			/* device under root hub's port */
-			port1 = usb_hcd_find_raw_port_number(usb_hcd,
+			raw_port = usb_hcd_find_raw_port_number(usb_hcd,
 				port1);
 		}
 		dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
-				port1);
+				raw_port);
 
 		/* hub driver sets up TT records */
 	}
-- 
2.7.2

Re: [PATCH v2] usb/core: usb_alloc_dev(): fix setting of ->portnum

[Alan Stern]

On Thu, 17 Mar 2016, Nicolai Stange wrote:

> With commit 69bec7259853 ("USB: core: let USB device know device node"),
> the port1 argument of usb_alloc_dev() gets overwritten as follows:
> 
>   ... usb_alloc_dev(..., unsigned port1)
>   {
>     ...
>     if (!parent->parent) {
>       port1 = usb_hcd_find_raw_port_number(..., port1);
>     }
>     ...
>   }
> 
> Later on, this now overwritten port1 gets assigned to ->portnum:
> 
>   dev->portnum = port1;
> 
> However, since xhci_find_raw_port_number() isn't idempotent, the
> aforementioned commit causes a number of KASAN splats like the following:

...

> Fix this by not overwriting the port1 argument in usb_alloc_dev(), but
> storing the raw port number as required by OF in an additional variable,
> raw_port.
> 
> Fixes: 69bec7259853 ("USB: core: let USB device know device node")
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
>  Applicable to linux-next-20160317
> 
>  Changes to v1:
>  - Initialize raw_port with port1

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH v2] usb/core: usb_alloc_dev(): fix setting of ->portnum

[Peter Chen]

On Fri, Mar 18, 2016 at 10:13:34AM -0400, Alan Stern wrote:
> On Thu, 17 Mar 2016, Nicolai Stange wrote:
> 
> > With commit 69bec7259853 ("USB: core: let USB device know device node"),
> > the port1 argument of usb_alloc_dev() gets overwritten as follows:
> > 
> >   ... usb_alloc_dev(..., unsigned port1)
> >   {
> >     ...
> >     if (!parent->parent) {
> >       port1 = usb_hcd_find_raw_port_number(..., port1);
> >     }
> >     ...
> >   }
> > 
> > Later on, this now overwritten port1 gets assigned to ->portnum:
> > 
> >   dev->portnum = port1;
> > 
> > However, since xhci_find_raw_port_number() isn't idempotent, the
> > aforementioned commit causes a number of KASAN splats like the following:
> 
> ...
> 
> > Fix this by not overwriting the port1 argument in usb_alloc_dev(), but
> > storing the raw port number as required by OF in an additional variable,
> > raw_port.
> > 
> > Fixes: 69bec7259853 ("USB: core: let USB device know device node")
> > Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> > ---
> >  Applicable to linux-next-20160317
> > 
> >  Changes to v1:
> >  - Initialize raw_port with port1
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 

Thanks for fixing it in time, you are right. My patch would let the
device logical port number equals to device raw port number, but it
is wrong for xhci.

-- 

Best Regards,
Peter Chen