linux-kernel - Re: [PATCH v3] usb: dwc3: gadget: trb

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

Date:   Fri, 19 Jul 2019 01:12:24 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     "fei.yang@...el.com" <fei.yang@...el.com>,
        "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        "john.stultz@...aro.org" <john.stultz@...aro.org>,
        "andrzej.p@...labora.com" <andrzej.p@...labora.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

Hi,

fei.yang@...el.com wrote:
> From: Fei Yang <fei.yang@...el.com>
>
> If scatter-gather operation is allowed, a large USB request is split into
> multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
> except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
> Since only the last TRB has IOC set for the whole USB request, the
> dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
> completes and all the TRBs allocated for this request are supposed to be
> reclaimed. However that is not what the current code does.
>
> dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
> following for-loop,
> 	for_each_sg(sg, s, pending, i) {
> 		trb = &dep->trb_pool[dep->trb_dequeue];
>
>                 if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>                         break;
>
>                 req->sg = sg_next(s);
>                 req->num_pending_sgs--;
>
>                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>                                 trb, event, status, chain);
>                 if (ret)
>                         break;
>         }
> but since the interrupt comes only after the last TRB completes, the
> event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
> the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
> 	if (event->status & DEPEVT_STATUS_IOC)
> 		return 1;
>
> This patch addresses the issue by checking each TRB in function
> dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
> reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
> last TRB.
>
> Signed-off-by: Fei Yang <fei.yang@...el.com>
> Cc: stable <stable@...r.kernel.org>
> ---
> v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
>     and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
> v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
>     dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
> ---
>  drivers/usb/dwc3/gadget.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 173f532..88eed49 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>  		return 1;
>  
> -	if (event->status & DEPEVT_STATUS_IOC)
> +	if (event->status & DEPEVT_STATUS_IOC && !chain)
>  		return 1;
>  
>  	return 0;
> @@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  		struct dwc3_request *req, const struct dwc3_event_depevt *event,
>  		int status)
>  {
> -	struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
> +	struct dwc3_trb *trb;
>  	struct scatterlist *sg = req->sg;
>  	struct scatterlist *s;
>  	unsigned int pending = req->num_pending_sgs;
>  	unsigned int i;
> +	int chain = false;
>  	int ret = 0;
>  
>  	for_each_sg(sg, s, pending, i) {
> @@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  
>  		req->sg = sg_next(s);
>  		req->num_pending_sgs--;
> +		if (trb->ctrl & DWC3_TRB_CTRL_CHN)
> +			chain = true;
> +		else
> +			chain = false;
>  
>  		ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
> -				trb, event, status, true);
> +				trb, event, status, chain);
>  		if (ret)
>  			break;
>  	}

There was already a fix a long time ago by Anurag. But it never made it
to the kernel mainline. You can check this out:
https://patchwork.kernel.org/patch/10640137/

Hi Felipe,

Maybe you can review and cherry-pick that patch?

Thanks,
Thinh