From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751665AbcBWMdX (ORCPT ); Tue, 23 Feb 2016 07:33:23 -0500 Received: from mail-pf0-f172.google.com ([209.85.192.172]:35064 "EHLO mail-pf0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751190AbcBWMdV (ORCPT ); Tue, 23 Feb 2016 07:33:21 -0500 From: Felipe Balbi To: Liu Xiang , linux-usb@vger.kernel.org, Bin Liu Cc: linux-kernel@vger.kernel.org, balbi@ti.com, gregkh@linuxfoundation.org, Liu Xiang Subject: Re: [PATCH] usb: musb: host: Fix NULL pointer dereference in SMP environment In-Reply-To: <1455899913-3847-1-git-send-email-liu.xiang6@zte.com.cn> References: <1455899913-3847-1-git-send-email-liu.xiang6@zte.com.cn> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/25.0.50.2 (x86_64-pc-linux-gnu) Date: Tue, 23 Feb 2016 06:32:45 +0200 Message-ID: <874md0kpw2.fsf@ti.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Liu Xiang writes: > In multi-core SoC, if enable USB endpoint/transmit urb/disable=20 > USB endpoint repeatedly,it can cause a NULL pointer dereference bug: > > Unable to handle kernel NULL pointer dereference at virtual address 00000= 010 > pgd =3D d3eb4000 > [00000010] *pgd=3D00000000 > 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 : [] lr : [] 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 > [] (__dabt_svc+0x70/0xa0) from [] (musb_h_disable+0xf= c/0x15c) > [] (musb_h_disable+0xfc/0x15c) from [] (usb_hcd_disab= le_endpoint+0x3c/0x44) > [] (usb_hcd_disable_endpoint+0x3c/0x44) from [] (usb_= disable_endpoint+0x64/0x7c) > [] (usb_disable_endpoint+0x64/0x7c) from [] (usb_disa= ble_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_us= ercopy+0x144/0x500) > [] (video_usercopy+0x144/0x500) from [] (uvc_v4l2_ioc= tl+0x2c/0x74) > [] (uvc_v4l2_ioctl+0x2c/0x74) from [] (v4l2_ioctl+0x9= 4/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+0x= 0/0x30) > > Considering the following execution sequence: > CPU0 CPU1 > musb_urb_dequeue > spin_lock_irqsave(&musb->lock, flags); > ready =3D qh->is_ready; > qh->is_ready =3D 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 =3D qh->is_ready; > qh->is_ready =3D 0; > musb_giveback(musb, urb, status); > spin_unlock(&musb->lock); > spin_lock(&musb->lock); > qh->is_ready =3D ready; > spin_unlock_irqrestore(&musb->lock, flags); > spin_lock(&musb->lock); > qh->is_ready =3D ready; > > When musb_urb_dequeue is called finally, the qh->is_ready has already=20 > been set to 0 in error.Thus the recycling qh job in musb_urb_dequeue=20 > 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,=20 > then recycle it and go to exit directly.=09=09 > > 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_hos= t_endpoint *hep) > qh->is_ready =3D 0; > if (musb_ep_get_qh(qh->hw_ep, is_in) =3D=3D qh) { > urb =3D next_urb(qh); > - > + if (urb =3D=3D NULL) { !urb would be nicer here. Also, adding Bin as he'll become the new MUSB maintainer. > + qh->hep->hcpriv =3D NULL; > + list_del(&qh->ring); > + kfree(qh); > + goto exit; > + } > /* make software (then hardware) stop ASAP */ > if (!urb->unlinked) > urb->status =3D -ESHUTDOWN; > --=20 > 1.9.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWy+DuAAoJEIaOsuA1yqRE0mEP/i4MldauvOTkq/zTXwurOqlG 1Tzr8eIrHX4P48iePGTE01QQsX3whyVWpzqAA2D4SC/aV1kdJachpnXFM8R4o0jd GwLLJgO6kLUpyscltbiW8ex04r4i+0pSnvGKlyoizPCcktMVmIJ/15W5GA4Z3lkH lH0zKGQx/JStX2YHwgSOqigPuma8+KCxeQ/CzyEWEd7mFHk2/3GRPhMvKqVbNcrs TCDTX77fhUrYVcp0y4wzJOQJJ78447pYVzMOyNxxETYmvt9h/9T6YgyV0QbOLuTD VSehMoYBvG7NSUUIqR764AvNm4xzR9Zw70kieEwZf7kJNWkadVX20GT60mHCwvIe HxiVN/h2qxgkXTUjZejZ3CjyJi6AkpDiQEI21mtvQCAzQkW5TFP9p6q7jpDxPGDk ETrdtzGe/SwI92kPeF8E5Aag/atSTdWzrfxaqoWc7qpbeuvCEY1gE0o0YcuSy1Qo EYB6s9QwWRIWnUg7HxwYeq1WmwE4w+ppXXfNbTVZtZulVwSrUwrdvl3RZ6juji6X MnCwluSy1iCxB6w+7kp3miNBnJ/nsP3+Y1cHZwHDpJQNrmX61h2+XVz20W+hRLyz 0VKc+uRxK3wVH1R+VxLmqbXucG3z0J3ERUlIU2qI7N2l3XqPmStNPtc90NaWmGs7 O1tBAPLP9CXH0GhAREte =kJOh -----END PGP SIGNATURE----- --=-=-=--