linux-kernel - Re: [PATCH v3] usb: dwc3: gadget: Move null pinter check after window closed

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: <Yn4E0XHlsPjaEgMw@kroah.com>
Date:   Fri, 13 May 2022 09:12:17 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Albert Wang <albertccwang@...gle.com>
Cc:     balbi@...nel.org, quic_jackp@...cinc.com, badhri@...gle.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] usb: dwc3: gadget: Move null pinter check after
 window closed

On Fri, May 13, 2022 at 02:57:09PM +0800, Albert Wang wrote:
> After inspecting further, we do see the locking is implicit, with the
> main gotcha being the unlock/re-lock.

This sentance makes no sense at all.

Who is "we"?  What is the gotcha?  What is the subject of the sentance?
What is going on?

> That creates a window for a race to happen.

What is "that"?

> This change moves the NULL check to be adjacent to where
> to it's used and after the window is "closed".

What is "this"?

Please read Documentation/process/submitting-patches.rst for how to
properly write a changelog text so that others can understand what is
going on.

thanks,

greg k-h


> 
> Fixes: 26288448120b ("usb: dwc3: gadget: Fix null pointer exception")
> Signed-off-by: Albert Wang <albertccwang@...gle.com>
> ---
>  v3: Add change log to be compliant with the canonical patch format
>  v2: Remove redundant 'else' and add additional comments and more
>      descriptive commit text
> 
>  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 19477f4bbf54..fda58951cf27 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3366,14 +3366,19 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
>  	struct dwc3		*dwc = dep->dwc;
>  	bool			no_started_trb = true;
>  
> -	if (!dep->endpoint.desc)
> -		return no_started_trb;
> -
> +	/*
> +	 * This function eventually leads to dwc3_giveback() which unlocks
> +	 * the dwc->lock and relocks afterwards. This actually creates a
> +	 * a window for a race to happen.

What race?  Why mention it here?  Why not fix it instead of documenting
it?

this comment does not make sense, sorry.

thanks,

greg k-h