[PATCH] usb: gadget: net2272: assert for a valid dma request

[PATCH] usb: gadget: net2272: assert for a valid dma request

[trix]

From: Tom Rix <trix@redhat.com>

clang static analysis flags this representive problem

net2272.c:1541:8: warning: Dereference of null pointer
    if ((req->req.length % ep->ep.maxpacket != 0) ||
         ^~~~~~~~~~~~~~~
This is mostly not a problem.

In net2272_handle_dma(), even though every path through
the routine dereferences req, it is set to NULL when the
ep->queue is empty.  If the eq->queue was ever empty this
would have oops.

So the else statement should not be needed.  If it is,
flag it as a BUG.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/usb/gadget/udc/net2272.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
index 44d1ea2307bb..8301723a8b1a 100644
--- a/drivers/usb/gadget/udc/net2272.c
+++ b/drivers/usb/gadget/udc/net2272.c
@@ -1506,17 +1506,16 @@ static int net2272_stop(struct usb_gadget *_gadget)
 static void
 net2272_handle_dma(struct net2272_ep *ep)
 {
-	struct net2272_request *req;
+	struct net2272_request *req = NULL;
 	unsigned len;
 	int status;
 
 	if (!list_empty(&ep->queue))
 		req = list_entry(ep->queue.next,
 				struct net2272_request, queue);
-	else
-		req = NULL;
 
 	dev_vdbg(ep->dev->dev, "handle_dma %s req %p\n", ep->ep.name, req);
+	BUG_ON(!req);
 
 	/* Ensure DREQ is de-asserted */
 	net2272_write(ep->dev, DMAREQ,
-- 
2.18.1

Re: [PATCH] usb: gadget: net2272: assert for a valid dma request

[Alan Stern]

On Sun, Aug 30, 2020 at 11:36:46AM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> clang static analysis flags this representive problem
> 
> net2272.c:1541:8: warning: Dereference of null pointer
>     if ((req->req.length % ep->ep.maxpacket != 0) ||
>          ^~~~~~~~~~~~~~~
> This is mostly not a problem.
> 
> In net2272_handle_dma(), even though every path through
> the routine dereferences req, it is set to NULL when the
> ep->queue is empty.  If the eq->queue was ever empty this
> would have oops.
> 
> So the else statement should not be needed.  If it is,
> flag it as a BUG.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>

This patch really seems to be overkill.  In particular, Linus Torvalds 
feels very strongly that people should not add BUG or BUG_ON calls 
except in extreme situations -- which this isn't.

> ---
>  drivers/usb/gadget/udc/net2272.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
> index 44d1ea2307bb..8301723a8b1a 100644
> --- a/drivers/usb/gadget/udc/net2272.c
> +++ b/drivers/usb/gadget/udc/net2272.c
> @@ -1506,17 +1506,16 @@ static int net2272_stop(struct usb_gadget *_gadget)
>  static void
>  net2272_handle_dma(struct net2272_ep *ep)
>  {
> -	struct net2272_request *req;
> +	struct net2272_request *req = NULL;
>  	unsigned len;
>  	int status;
>  
>  	if (!list_empty(&ep->queue))
>  		req = list_entry(ep->queue.next,
>  				struct net2272_request, queue);
> -	else
> -		req = NULL;
>  
>  	dev_vdbg(ep->dev->dev, "handle_dma %s req %p\n", ep->ep.name, req);
> +	BUG_ON(!req);

There's no point in adding this.  If the function goes on to dereference 
a NULL pointer, you'll get the same effect in any case -- an oops.

If you want to make the point that req had better not be NULL, just get 
rid of the "if" test entirely.  You could even change the list_entry to 
list_first_entry.

Alan Stern

Re: [PATCH] usb: gadget: net2272: assert for a valid dma request

[Tom Rix]

On 8/30/20 2:22 PM, Alan Stern wrote:
> On Sun, Aug 30, 2020 at 11:36:46AM -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> clang static analysis flags this representive problem
>>
>> net2272.c:1541:8: warning: Dereference of null pointer
>>     if ((req->req.length % ep->ep.maxpacket != 0) ||
>>          ^~~~~~~~~~~~~~~
>> This is mostly not a problem.
>>
>> In net2272_handle_dma(), even though every path through
>> the routine dereferences req, it is set to NULL when the
>> ep->queue is empty.  If the eq->queue was ever empty this
>> would have oops.
>>
>> So the else statement should not be needed.  If it is,
>> flag it as a BUG.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
> This patch really seems to be overkill.  In particular, Linus Torvalds 
> feels very strongly that people should not add BUG or BUG_ON calls 
> except in extreme situations -- which this isn't.
>
>> ---
>>  drivers/usb/gadget/udc/net2272.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
>> index 44d1ea2307bb..8301723a8b1a 100644
>> --- a/drivers/usb/gadget/udc/net2272.c
>> +++ b/drivers/usb/gadget/udc/net2272.c
>> @@ -1506,17 +1506,16 @@ static int net2272_stop(struct usb_gadget *_gadget)
>>  static void
>>  net2272_handle_dma(struct net2272_ep *ep)
>>  {
>> -	struct net2272_request *req;
>> +	struct net2272_request *req = NULL;
>>  	unsigned len;
>>  	int status;
>>  
>>  	if (!list_empty(&ep->queue))
>>  		req = list_entry(ep->queue.next,
>>  				struct net2272_request, queue);
>> -	else
>> -		req = NULL;
>>  
>>  	dev_vdbg(ep->dev->dev, "handle_dma %s req %p\n", ep->ep.name, req);
>> +	BUG_ON(!req);
> There's no point in adding this.  If the function goes on to dereference 
> a NULL pointer, you'll get the same effect in any case -- an oops.
>
> If you want to make the point that req had better not be NULL, just get 
> rid of the "if" test entirely.  You could even change the list_entry to 
> list_first_entry.

Since nothing is really going to change, drop this patch.

Tom

> Alan Stern
>

Re: [PATCH] usb: gadget: net2272: assert for a valid dma request

[Greg KH]

On Sun, Aug 30, 2020 at 11:36:46AM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> clang static analysis flags this representive problem
> 
> net2272.c:1541:8: warning: Dereference of null pointer
>     if ((req->req.length % ep->ep.maxpacket != 0) ||
>          ^~~~~~~~~~~~~~~
> This is mostly not a problem.
> 
> In net2272_handle_dma(), even though every path through
> the routine dereferences req, it is set to NULL when the
> ep->queue is empty.  If the eq->queue was ever empty this
> would have oops.
> 
> So the else statement should not be needed.  If it is,
> flag it as a BUG.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/usb/gadget/udc/net2272.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
> index 44d1ea2307bb..8301723a8b1a 100644
> --- a/drivers/usb/gadget/udc/net2272.c
> +++ b/drivers/usb/gadget/udc/net2272.c
> @@ -1506,17 +1506,16 @@ static int net2272_stop(struct usb_gadget *_gadget)
>  static void
>  net2272_handle_dma(struct net2272_ep *ep)
>  {
> -	struct net2272_request *req;
> +	struct net2272_request *req = NULL;
>  	unsigned len;
>  	int status;
>  
>  	if (!list_empty(&ep->queue))
>  		req = list_entry(ep->queue.next,
>  				struct net2272_request, queue);
> -	else
> -		req = NULL;
>  
>  	dev_vdbg(ep->dev->dev, "handle_dma %s req %p\n", ep->ep.name, req);
> +	BUG_ON(!req);

No, fix this properly, don't paper over the issue by crashing (as Alan
said, never add a BUG_ON() for something that you can recover from, and
really, never add a BUG_ON() call at all, that's just being a lazy
programmer.)

If this can cause a NULL dereference later on, than correctly handle the
error here please.

thanks,

greg k-h