linux-kernel - Re: [PATCH] usb: dwc3: gadget: Replace list_for_each_entry

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]

Message-ID: <20210508034551.GA2728@nchen>
Date:   Sat, 8 May 2021 11:45:51 +0800
From:   Peter Chen <peter.chen@...nel.org>
To:     Wesley Cheng <wcheng@...eaurora.org>
Cc:     balbi@...nel.org, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        jackp@...eaurora.org
Subject: Re: [PATCH] usb: dwc3: gadget: Replace list_for_each_entry_safe() if
 using giveback

On 21-05-07 11:42:03, Wesley Cheng wrote:
> The list_for_each_entry_safe() macro saves the current item (n) and
> the item after (n+1), so that n can be safely removed without
> corrupting the list.  However, when traversing the list and removing
> items using gadget giveback, the DWC3 lock is briefly released,

I see dwc3_gadget_del_and_unmap_request remove the list, the lock is
still held there. Am I something wrong?

Peter
       
> allowing other routines to execute.  There is a situation where while
> items are being removed from the cancelled_list using
> dwc3_gadget_ep_cleanup_cancelled_requests(), the pullup disable
> routine is running in parallel (due to UDC unbind).  As the cleanup
> routine removes n, and the pullup disable removes n+1, once the
> cleanup retakes the DWC3 lock, it references a request who was already
> removed/handled.  With list debug enabled, this leads to a panic.
> Ensure all instances of the macro are replaced where gadget giveback
> is used.
> 
> Fixes: d4f1afe5e896 ("usb: dwc3: gadget: move requests to cancelled_list")
> Signed-off-by: Wesley Cheng <wcheng@...eaurora.org>
> ---
>  drivers/usb/dwc3/gadget.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index dd80e5c..efa939b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1737,10 +1737,10 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r
>  static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
>  {
>  	struct dwc3_request		*req;
> -	struct dwc3_request		*tmp;
>  	struct dwc3			*dwc = dep->dwc;
>  
> -	list_for_each_entry_safe(req, tmp, &dep->cancelled_list, list) {
> +	while (!list_empty(&dep->cancelled_list)) {
> +		req = next_request(&dep->cancelled_list);
>  		dwc3_gadget_ep_skip_trbs(dep, req);
>  		switch (req->status) {
>  		case DWC3_REQUEST_STATUS_DISCONNECTED:
> @@ -2935,11 +2935,11 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
>  		const struct dwc3_event_depevt *event, int status)
>  {
>  	struct dwc3_request	*req;
> -	struct dwc3_request	*tmp;
>  
> -	list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
> +	while (!list_empty(&dep->started_list)) {
>  		int ret;
>  
> +		req = next_request(&dep->started_list);
>  		ret = dwc3_gadget_ep_cleanup_completed_request(dep, event,
>  				req, status);
>  		if (ret)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 

Thanks,
Peter Chen