On 3/17/2015 2:54 AM, Mian Yousaf Kaukab wrote:
> From: Gregory Herrero <gregory.herrero@xxxxxxxxx>
>
> During urb_enqueue, if the urb can't be queued to the endpoint,
> the urb is freed without any spinlock protection.
> This leads to memory corruption when concurrent urb_dequeue try to free
> same urb->hcpriv.
> Thus, ensure the whole urb_enqueue in spinlocked.
>
> Signed-off-by: Gregory Herrero <gregory.herrero@xxxxxxxxx>
> ---
> drivers/usb/dwc2/hcd.c | 9 ++-------
> drivers/usb/dwc2/hcd_queue.c | 6 ------
> 2 files changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 23a8f2f..fd2ad25 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -354,7 +354,6 @@ static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
> gfp_t mem_flags)
> {
> struct dwc2_qtd *qtd;
> - unsigned long flags;
> u32 intr_mask;
> int retval;
> int dev_speed;
> @@ -405,11 +404,9 @@ static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
> */
> return 0;
>
> - spin_lock_irqsave(&hsotg->lock, flags);
> tr_type = dwc2_hcd_select_transactions(hsotg);
> if (tr_type != DWC2_TRANSACTION_NONE)
> dwc2_hcd_queue_transactions(hsotg, tr_type);
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> }
>
> return 0;
> @@ -2504,7 +2501,6 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
>
> spin_lock_irqsave(&hsotg->lock, flags);
> retval = usb_hcd_link_urb_to_ep(hcd, urb);
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> if (retval)
> goto fail1;
Spinlock not unlocked in this condition.
>
> @@ -2513,17 +2509,16 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> goto fail2;
>
> if (alloc_bandwidth) {
> - spin_lock_irqsave(&hsotg->lock, flags);
> dwc2_allocate_bus_bandwidth(hcd,
> dwc2_hcd_get_ep_bandwidth(hsotg, ep),
> urb);
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> }
>
> + spin_unlock_irqrestore(&hsotg->lock, flags);
> +
> return 0;
>
> fail2:
> - spin_lock_irqsave(&hsotg->lock, flags);
> dwc2_urb->priv = NULL;
> usb_hcd_unlink_urb_from_ep(hcd, urb);
> spin_unlock_irqrestore(&hsotg->lock, flags);
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index bb97838..02103b66 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -777,7 +777,6 @@ int dwc2_hcd_qtd_add(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
> struct dwc2_qh **qh, gfp_t mem_flags)
Document spinlock must be held.
> {
> struct dwc2_hcd_urb *urb = qtd->urb;
> - unsigned long flags;
> int allocated = 0;
> int retval;
>
> @@ -792,7 +791,6 @@ int dwc2_hcd_qtd_add(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
> allocated = 1;
> }
>
> - spin_lock_irqsave(&hsotg->lock, flags);
>
Extra newline introduced here.
> retval = dwc2_hcd_qh_add(hsotg, *qh);
> if (retval)
> @@ -800,7 +798,6 @@ int dwc2_hcd_qtd_add(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
>
> qtd->qh = *qh;
> list_add_tail(&qtd->qtd_list_entry, &(*qh)->qtd_list);
> - spin_unlock_irqrestore(&hsotg->lock, flags);
>
> return 0;
>
> @@ -817,10 +814,7 @@ fail:
> qtd_list_entry)
> dwc2_hcd_qtd_unlink_and_free(hsotg, qtd2, qh_tmp);
>
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> dwc2_hcd_qh_free(hsotg, qh_tmp);
> - } else {
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> }
>
> return retval;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
