[PATCH 0/1] usb: musb: nuke endpoint before setting its descriptor to NULL

[PATCH 0/1] usb: musb: nuke endpoint before setting its descriptor to NULL

[Tal Shorer]

Hello,
I compiled and installed linux-4.6-rc3 on my beagle bone black and
noticed that when I unload a gadget using f_sourcesink (namely
g_zero), a kernel panic occurs:
[   12.531504] Unable to handle kernel NULL pointer dereference at virtual address 00000005
[   12.540100] pgd = de6a4000
[   12.542984] [00000005] *pgd=9e702831, *pte=00000000, *ppte=00000000
[   12.549713] Internal error: Oops: 17 [#1] SMP ARM
[   12.554713] Modules linked in: usb_f_ss_lb g_zero(-) libcomposite musb_dsps musb_hdrc cppi41 udc_core usbcore omap_rng rng_core musb_am335x rtc_omap omap_wdt cpufreq_dt thermal_sys leds_gpio hwmon led_class
[   12.574519] CPU: 0 PID: 139 Comm: modprobe Not tainted 4.6.0-rc3 #3
[   12.581165] Hardware name: Generic AM33XX (Flattened Device Tree)
[   12.587632] task: de65e400 ti: de6ce000 task.ti: de6ce000
[   12.593391] PC is at source_sink_free_instance+0x24/0xfc [usb_f_ss_lb]
[   12.600327] LR is at source_sink_complete+0x174/0x480 [usb_f_ss_lb]
[   12.606980] pc : [<bf0d699c>]    lr : [<bf0d6d2c>]    psr: 80000093
[   12.606980] sp : de6cfe40  ip : bf0a4074  fp : 00000000
[   12.619141] r10: de685d80  r9 : dd00a000  r8 : 20000093
[   12.624688] r7 : de685d80  r6 : dd123000  r5 : 00000000  r4 : dd1804e8
[   12.631609] r3 : 00000000  r2 : bf0d2900  r1 : de5a4280  r0 : dd123000
[   12.638536] Flags: Nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   12.646196] Control: 10c5387d  Table: 9e6a4019  DAC: 00000051
[   12.652292] Process modprobe (pid: 139, stack limit = 0xde6ce218)
[   12.658758] Stack: (0xde6cfe40 to 0xde6d0000)
[   12.663409] fe40: dd1804e8 dd1804e8 de5a4280 dd123000 de685d80 20000093 dd00a000 e0b72410
[   12.672096] fe60: 00000000 bf0d6d2c 60000093 dd180010 dd180010 dd1804e8 00000001 de5a4280
[   12.680781] fe80: dd180010 dd1804e8 00000001 bf09900c c0c0965c bf0ad7ec e0b72410 dd180010
[   12.689467] fea0: dd1804e8 dd180538 dd180010 bf0b2c84 20000093 bf0adf28 dd1804e8 de685d80
[   12.698152] fec0: dd180f0c dd1804e8 c0107984 de6ce000 00000000 bf0d68f8 dd102740 de6cff00
[   12.706838] fee0: c0107984 de685d80 dd180fec bf0d7aa8 dd180f0c dd123000 de685d80 00000000
[   12.715523] ff00: 00000081 bf0d7b00 dd180fec bf0c4bec bf0ca000 bf0c4c18 de685d80 de685ddc
[   12.724209] ff20: 60000013 bf0c51d4 dd175800 dd1811b0 00000080 bf09974c bf0d2a10 dd175800
[   12.732895] ff40: bf0d2a10 bf099814 bf0d247c bf0d2ac0 000af6d0 c01c5350 00000000 657a5f67
[   12.741579] ff60: 00006f72 c0cc5d9c de65e400 00000000 de6ce000 00000000 00000000 c0155754
[   12.750264] ff80: 00000000 de6ce000 b6f4348c 000af6b0 00000001 001078bc b6f4348c 000af6b0
[   12.758949] ffa0: 00000001 c01077e0 b6f4348c 000af6b0 000af6d0 00000080 00000001 00000000
[   12.767633] ffc0: b6f4348c 000af6b0 00000001 00000081 000af638 00000000 000af6b0 00000000
[   12.776319] ffe0: b6eed3e8 beb1bb70 0001b160 b6eed3f4 a0000010 000af6d0 9fdf6861 9fdf6c61
[   12.785029] [<bf0d699c>] (source_sink_free_instance [usb_f_ss_lb]) from [<bf0d6d2c>] (source_sink_complete+0x174/0x480 [usb_f_ss_lb])
[   12.797779] [<bf0d6d2c>] (source_sink_complete [usb_f_ss_lb]) from [<bf09900c>] (usb_gadget_giveback_request+0xc/0x10 [udc_core])
[   12.810188] [<bf09900c>] (usb_gadget_giveback_request [udc_core]) from [<bf0ad7ec>] (musb_g_giveback+0x118/0x614 [musb_hdrc])
[   12.822218] [<bf0ad7ec>] (musb_g_giveback [musb_hdrc]) from [<bf0adf28>] (musb_gadget_disable+0x130/0x1d8 [musb_hdrc])
[   12.833590] [<bf0adf28>] (musb_gadget_disable [musb_hdrc]) from [<bf0d68f8>] (sourcesink_get_alt+0x3c/0x78 [usb_f_ss_lb])
[   12.845227] [<bf0d68f8>] (sourcesink_get_alt [usb_f_ss_lb]) from [<bf0d7aa8>] (disable_endpoints+0x24/0x84 [usb_f_ss_lb])
[   12.856864] [<bf0d7aa8>] (disable_endpoints [usb_f_ss_lb]) from [<bf0d7b00>] (disable_endpoints+0x7c/0x84 [usb_f_ss_lb])
[   12.868447] [<bf0d7b00>] (disable_endpoints [usb_f_ss_lb]) from [<bf0c4c18>] (config_ep_by_speed+0x28c/0x3ac [libcomposite])
[   12.880379] [<bf0c4c18>] (config_ep_by_speed [libcomposite]) from [<bf0c51d4>] (composite_disconnect+0x2c/0x54 [libcomposite])
[   12.892484] [<bf0c51d4>] (composite_disconnect [libcomposite]) from [<bf09974c>] (usb_gadget_state_work+0x90/0xf4 [udc_core])
[   12.904488] [<bf09974c>] (usb_gadget_state_work [udc_core]) from [<bf099814>] (usb_gadget_unregister_driver+0x64/0xc4 [udc_core])
[   12.916868] [<bf099814>] (usb_gadget_unregister_driver [udc_core]) from [<c01c5350>] (SyS_delete_module+0x11c/0x1e4)
[   12.928048] [<c01c5350>] (SyS_delete_module) from [<c01077e0>] (ret_fast_syscall+0x0/0x1c)
[   12.936829] Code: e5905080 e5933024 e3550002 e592a01c (e5d39005)
[   12.943314] ---[ end trace 87c865532163a167 ]---

I traced the issue to f_sourcesink trying to use a struct usb_ep's
dest field after it's set to NULL by musb_gadget.c

This patch fixes this problem by moving the clearing of ep->desc to
occur after calling the complete() callbacks for all requests.

Tal Shorer (1):
  usb: musb: gadget: nuke endpoint before setting its descriptor to NULL

 drivers/usb/musb/musb_gadget.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.5.0

[PATCH 1/1] usb: musb: gadget: nuke endpoint before setting its descriptor to NULL

[Tal Shorer]

Some functions, such as f_sourcesink, rely on an endpoint's desc
field during their requests' complete() callback, so clear it only
_after_ nuking all requests to avoid NULL pointer dereference.

Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
---
 drivers/usb/musb/musb_gadget.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 87bd578..152865b 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1164,12 +1164,12 @@ static int musb_gadget_disable(struct usb_ep *ep)
 		musb_writew(epio, MUSB_RXMAXP, 0);
 	}
 
-	musb_ep->desc = NULL;
-	musb_ep->end_point.desc = NULL;
-
 	/* abort all pending DMA and requests */
 	nuke(musb_ep, -ESHUTDOWN);
 
+	musb_ep->desc = NULL;
+	musb_ep->end_point.desc = NULL;
+
 	schedule_work(&musb->irq_work);
 
 	spin_unlock_irqrestore(&(musb->lock), flags);
-- 
2.5.0

Re: [PATCH 1/1] usb: musb: gadget: nuke endpoint before setting its descriptor to NULL

[Bin Liu]

Hi,

On Thu, Apr 14, 2016 at 07:33:43PM +0300, Tal Shorer wrote:
> Some functions, such as f_sourcesink, rely on an endpoint's desc
> field during their requests' complete() callback, so clear it only
> _after_ nuking all requests to avoid NULL pointer dereference.
> 
> Signed-off-by: Tal Shorer <tal.shorer@gmail.com>

Signed-off-by: Bin Liu <b-liu@ti.com>

Regards,
-Bin.

> ---
>  drivers/usb/musb/musb_gadget.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index 87bd578..152865b 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -1164,12 +1164,12 @@ static int musb_gadget_disable(struct usb_ep *ep)
>  		musb_writew(epio, MUSB_RXMAXP, 0);
>  	}
>  
> -	musb_ep->desc = NULL;
> -	musb_ep->end_point.desc = NULL;
> -
>  	/* abort all pending DMA and requests */
>  	nuke(musb_ep, -ESHUTDOWN);
>  
> +	musb_ep->desc = NULL;
> +	musb_ep->end_point.desc = NULL;
> +
>  	schedule_work(&musb->irq_work);
>  
>  	spin_unlock_irqrestore(&(musb->lock), flags);
> -- 
> 2.5.0
>