Re: [PATCH] usb: musb: host: Fix NULL pointer dereference in SMP environment

[Felipe Balbi]

Hi,

Liu Xiang  writes:
> 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 0010
> pgd = d3eb4000
> [0010] *pgd=
> Internal error: Oops: 5 [#1] PREEMPT SMP
> Pid: 1017, comm:  mediaserver
> CPU: 0Not 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 : []lr : []psr: 60070093
> sp : d3e5bc80  ip : 0027  fp : d3e5bca4
> r10:   r9 : ff94  r8 : 0080
> r7 : 60070013  r6 : d45900f0  r5 : d4717720  r4 : cee2d860
> r3 :   r2 : c0875628  r1 : d3e5bbc0  r0 : 002a
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 18c53c7d  Table: b3eb404a  DAC: 0015
> [] (__dabt_svc+0x70/0xa0) from [] 
> (musb_h_disable+0xfc/0x15c)
> [] (musb_h_disable+0xfc/0x15c) from [] 
> (usb_hcd_disable_endpoint+0x3c/0x44)
> [] (usb_hcd_disable_endpoint+0x3c/0x44) from [] 
> (usb_disable_endpoint+0x64/0x7c)
> [] (usb_disable_endpoint+0x64/0x7c) from [] 
> (usb_disable_interface+0x48/0x58)
> [] (usb_disable_interface+0x48/0x58) from [] 
> (usb_set_interface+0x158/0x274)
> [] (usb_set_interface+0x158/0x274) from [] 
> (uvc_video_enable+0x78/0x9c)
> [] (uvc_video_enable+0x78/0x9c) from [] 
> (uvc_v4l2_do_ioctl+0xd08/0x12ec)
> [] (uvc_v4l2_do_ioctl+0xd08/0x12ec) from [] 
> (video_usercopy+0x144/0x500)
> [] (video_usercopy+0x144/0x500) from [] 
> (uvc_v4l2_ioctl+0x2c/0x74)
> [] (uvc_v4l2_ioctl+0x2c/0x74) from [] 
> (v4l2_ioctl+0x94/0x154)
> [] (v4l2_ioctl+0x94/0x154) from [] 
> (do_vfs_ioctl+0x84/0x5ac)
> [] (do_vfs_ioctl+0x84/0x5ac) from [] (sys_ioctl+0x78/0x80)
> [] (sys_ioctl+0x78/0x80) from [] 
> (ret_fast_syscall+0x0/0x30)
>
> Considering the following execution sequence:
> CPU0   CPU1
> musb_urb_dequeue
>  spin_lock_irqsave(>lock, flags);
>   ready = qh->is_ready;
>qh->is_ready = 0;
> musb_giveback(musb, urb, 0);
>  spin_unlock(>lock);
>zx296702_musb_interrupt
>   musb_interrupt
>spin_lock_irqsave(>lock, flags);
> musb_advance_schedule
>  ready = qh->is_ready;
>   qh->is_ready = 0;
>musb_giveback(musb, urb, status);
> spin_unlock(>lock);
>   spin_lock(>lock);
>qh->is_ready = ready;
> spin_unlock_irqrestore(>lock, flags);
>  spin_lock(>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.  
>
> Signed-off-by: Liu Xiang 
> ---
>  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..438f5b4 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 == NULL) {

!urb would be nicer here.

Also, adding Bin as he'll become the new MUSB maintainer.

> + qh->hep->hcpriv = NULL;
> + list_del(>ring);
> + kfree(qh);
> + goto exit;
> + }
>   /* make software (then hardware) stop ASAP */
>   if (!urb->unlinked)
>   urb->status = -ESHUTDOWN;
> -- 
> 1.9.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] usb: musb: host: Fix NULL pointer dereference in SMP environment

[Liu Xiang]

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 0010
pgd = d3eb4000
[0010] *pgd=
Internal error: Oops: 5 [#1] PREEMPT SMP
Pid: 1017, comm:  mediaserver
CPU: 0Not 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 : []lr : []psr: 60070093
sp : d3e5bc80  ip : 0027  fp : d3e5bca4
r10:   r9 : ff94  r8 : 0080
r7 : 60070013  r6 : d45900f0  r5 : d4717720  r4 : cee2d860
r3 :   r2 : c0875628  r1 : d3e5bbc0  r0 : 002a
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 18c53c7d  Table: b3eb404a  DAC: 0015
[] (__dabt_svc+0x70/0xa0) from [] 
(musb_h_disable+0xfc/0x15c)
[] (musb_h_disable+0xfc/0x15c) from [] 
(usb_hcd_disable_endpoint+0x3c/0x44)
[] (usb_hcd_disable_endpoint+0x3c/0x44) from [] 
(usb_disable_endpoint+0x64/0x7c)
[] (usb_disable_endpoint+0x64/0x7c) from [] 
(usb_disable_interface+0x48/0x58)
[] (usb_disable_interface+0x48/0x58) from [] 
(usb_set_interface+0x158/0x274)
[] (usb_set_interface+0x158/0x274) from [] 
(uvc_video_enable+0x78/0x9c)
[] (uvc_video_enable+0x78/0x9c) from [] 
(uvc_v4l2_do_ioctl+0xd08/0x12ec)
[] (uvc_v4l2_do_ioctl+0xd08/0x12ec) from [] 
(video_usercopy+0x144/0x500)
[] (video_usercopy+0x144/0x500) from [] 
(uvc_v4l2_ioctl+0x2c/0x74)
[] (uvc_v4l2_ioctl+0x2c/0x74) from [] 
(v4l2_ioctl+0x94/0x154)
[] (v4l2_ioctl+0x94/0x154) from [] (do_vfs_ioctl+0x84/0x5ac)
[] (do_vfs_ioctl+0x84/0x5ac) from [] (sys_ioctl+0x78/0x80)
[] (sys_ioctl+0x78/0x80) from [] (ret_fast_syscall+0x0/0x30)

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

Signed-off-by: Liu Xiang 
---
 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..438f5b4 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 == NULL) {
+   qh->hep->hcpriv = NULL;
+   list_del(>ring);
+   kfree(qh);
+   goto exit;
+   }
/* make software (then hardware) stop ASAP */
if (!urb->unlinked)
urb->status = -ESHUTDOWN;
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html