linux-kernel - [PATCH v2] usb: musb: host: Fix NULL pointer dereference in SMP environment

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>] [day] [month] [year] [list]

Date:	Sat, 27 Feb 2016 17:47:29 +0800
From:	Liu Xiang <liu.xiang6@....com.cn>
To:	linux-usb@...r.kernel.org
Cc:	linux-kernel@...r.kernel.org, balbi@...com,
	gregkh@...uxfoundation.org, b-liu@...com,
	Liu Xiang <liu.xiang6@....com.cn>
Subject: [PATCH v2] usb: musb: host: Fix NULL pointer dereference in SMP environment

In multi-core SoC, if enable USB endpoint/transmit urb/disable
USB endpoint repeatedly,it can cause a NULL pointer dereference bug:

Unable to handle kernel NULL pointer dereference at virtual address 00000010
pgd = d3eb4000
[00000010] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP
Pid: 1017, comm:          mediaserver
CPU: 0    Not tainted  (3.0.101-ZXICTOS_V2.00.20_P2B4_ZTE_ZX296700_ASIC+ #2)
PC is at musb_h_disable+0xfc/0x15c
LR is at musb_h_disable+0xfc/0x15c
pc : [<c0353de8>]    lr : [<c0353de8>]    psr: 60070093
sp : d3e5bc80  ip : 00000027  fp : d3e5bca4
r10: 00000000  r9 : ffffff94  r8 : 00000080
r7 : 60070013  r6 : d45900f0  r5 : d4717720  r4 : cee2d860
r3 : 00000000  r2 : c0875628  r1 : d3e5bbc0  r0 : 0000002a
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 18c53c7d  Table: b3eb404a  DAC: 00000015
[<c00519d0>] (__dabt_svc+0x70/0xa0) from [<c0353de8>] (musb_h_disable+0xfc/0x15c)
[<c0353de8>] (musb_h_disable+0xfc/0x15c) from [<c032f160>] (usb_hcd_disable_endpoint+0x3c/0x44)
[<c032f160>] (usb_hcd_disable_endpoint+0x3c/0x44) from [<c0331504>] (usb_disable_endpoint+0x64/0x7c)
[<c0331504>] (usb_disable_endpoint+0x64/0x7c) from [<c0331564>] (usb_disable_interface+0x48/0x58)
[<c0331564>] (usb_disable_interface+0x48/0x58) from [<c0331bcc>] (usb_set_interface+0x158/0x274)
[<c0331bcc>] (usb_set_interface+0x158/0x274) from [<c03a8a30>] (uvc_video_enable+0x78/0x9c)
[<c03a8a30>] (uvc_video_enable+0x78/0x9c) from [<c03a66c4>] (uvc_v4l2_do_ioctl+0xd08/0x12ec)
[<c03a66c4>] (uvc_v4l2_do_ioctl+0xd08/0x12ec) from [<c036c044>] (video_usercopy+0x144/0x500)
[<c036c044>] (video_usercopy+0x144/0x500) from [<c03a5444>] (uvc_v4l2_ioctl+0x2c/0x74)
[<c03a5444>] (uvc_v4l2_ioctl+0x2c/0x74) from [<c036b208>] (v4l2_ioctl+0x94/0x154)
[<c036b208>] (v4l2_ioctl+0x94/0x154) from [<c0128628>] (do_vfs_ioctl+0x84/0x5ac)
[<c0128628>] (do_vfs_ioctl+0x84/0x5ac) from [<c0128bc8>] (sys_ioctl+0x78/0x80)
[<c0128bc8>] (sys_ioctl+0x78/0x80) from [<c0052000>] (ret_fast_syscall+0x0/0x30)

Considering the following execution sequence:
CPU0                           CPU1
musb_urb_dequeue
 spin_lock_irqsave(&musb->lock, flags);
  ready = qh->is_ready;
   qh->is_ready = 0;
    musb_giveback(musb, urb, 0);
     spin_unlock(&musb->lock);
                               zx296702_musb_interrupt
                                musb_interrupt
                                 spin_lock_irqsave(&musb->lock, flags);
                                  musb_advance_schedule
                                   ready = qh->is_ready;
                                    qh->is_ready = 0;
                                     musb_giveback(musb, urb, status);
                                      spin_unlock(&musb->lock);
      spin_lock(&musb->lock);
       qh->is_ready = ready;
        spin_unlock_irqrestore(&musb->lock, flags);
                                       spin_lock(&musb->lock);
                                        qh->is_ready = ready;

When musb_urb_dequeue is called finally, the qh->is_ready has already
been set to 0 in error.Thus the recycling qh job in musb_urb_dequeue
can not be done. That results in accessing empty qh in musb_h_disable.
So the qh in musb_h_disable should be checked.If the qh is emtpy,
then recycle it and go to exit directly.

---
v2:
 - Modify the urb empty condition
---

Signed-off-by: Liu Xiang <liu.xiang6@....com.cn>
---
 drivers/usb/musb/musb_host.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 795a45b..8445db3 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2515,7 +2515,12 @@ musb_h_disable(struct usb_hcd *hcd, struct usb_host_endpoint *hep)
 	qh->is_ready = 0;
 	if (musb_ep_get_qh(qh->hw_ep, is_in) == qh) {
 		urb = next_urb(qh);
-
+		if (!urb) {
+			qh->hep->hcpriv = NULL;
+			list_del(&qh->ring);
+			kfree(qh);
+			goto exit;
+		}
 		/* make software (then hardware) stop ASAP */
 		if (!urb->unlinked)
 			urb->status = -ESHUTDOWN;
-- 
1.9.1