[PATCH] usb: cdnsp: Fixes error: uninitialized symbol 'len'

[PATCH] usb: cdnsp: Fixes error: uninitialized symbol 'len'

[Pawel Laszczak]

The patch 5bc38d33a5a1: "usb: cdnsp: Fixes issue with redundant
Status Stage" leads to the following Smatch static checker warning:

  drivers/usb/cdns3/cdnsp-ep0.c:470 cdnsp_setup_analyze()
  error: uninitialized symbol 'len'.

cc: <stable@vger.kernel.org>
Fixes: 5bc38d33a5a1 ("usb: cdnsp: Fixes issue with redundant Status Stage")
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/cdnsp-ep0.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c
index d63d5d92f255..f317d3c84781 100644
--- a/drivers/usb/cdns3/cdnsp-ep0.c
+++ b/drivers/usb/cdns3/cdnsp-ep0.c
@@ -414,7 +414,7 @@ static int cdnsp_ep0_std_request(struct cdnsp_device *pdev,
 void cdnsp_setup_analyze(struct cdnsp_device *pdev)
 {
 	struct usb_ctrlrequest *ctrl = &pdev->setup;
-	int ret = 0;
+	int ret = -EINVAL;
 	u16 len;
 
 	trace_cdnsp_ctrl_req(ctrl);
@@ -424,7 +424,6 @@ void cdnsp_setup_analyze(struct cdnsp_device *pdev)
 
 	if (pdev->gadget.state == USB_STATE_NOTATTACHED) {
 		dev_err(pdev->dev, "ERR: Setup detected in unattached state\n");
-		ret = -EINVAL;
 		goto out;
 	}
 
-- 
2.34.1

Re: [PATCH] usb: cdnsp: Fixes error: uninitialized symbol 'len'

[Greg KH]

On Fri, Mar 31, 2023 at 05:06:00AM -0400, Pawel Laszczak wrote:
> The patch 5bc38d33a5a1: "usb: cdnsp: Fixes issue with redundant
> Status Stage" leads to the following Smatch static checker warning:
> 
>   drivers/usb/cdns3/cdnsp-ep0.c:470 cdnsp_setup_analyze()
>   error: uninitialized symbol 'len'.

Are you sure this is correct?

> 
> cc: <stable@vger.kernel.org>
> Fixes: 5bc38d33a5a1 ("usb: cdnsp: Fixes issue with redundant Status Stage")
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/cdnsp-ep0.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c
> index d63d5d92f255..f317d3c84781 100644
> --- a/drivers/usb/cdns3/cdnsp-ep0.c
> +++ b/drivers/usb/cdns3/cdnsp-ep0.c
> @@ -414,7 +414,7 @@ static int cdnsp_ep0_std_request(struct cdnsp_device *pdev,
>  void cdnsp_setup_analyze(struct cdnsp_device *pdev)
>  {
>  	struct usb_ctrlrequest *ctrl = &pdev->setup;
> -	int ret = 0;
> +	int ret = -EINVAL;
>  	u16 len;
>  
>  	trace_cdnsp_ctrl_req(ctrl);
> @@ -424,7 +424,6 @@ void cdnsp_setup_analyze(struct cdnsp_device *pdev)
>  
>  	if (pdev->gadget.state == USB_STATE_NOTATTACHED) {
>  		dev_err(pdev->dev, "ERR: Setup detected in unattached state\n");
> -		ret = -EINVAL;

That's a nice change, but I don't see the original error here that you
are saying this change fixes.

What am I missing?

thanks,

greg k-h

Re: [PATCH] usb: cdnsp: Fixes error: uninitialized symbol 'len'

[Oliver Neukum]

On 05.04.23 19:23, Greg KH wrote:
> On Fri, Mar 31, 2023 at 05:06:00AM -0400, Pawel Laszczak wrote:

>>   {
>>   	struct usb_ctrlrequest *ctrl = &pdev->setup;
>> -	int ret = 0;
>> +	int ret = -EINVAL;
>>   	u16 len;
>>   
>>   	trace_cdnsp_ctrl_req(ctrl);
>> @@ -424,7 +424,6 @@ void cdnsp_setup_analyze(struct cdnsp_device *pdev)
>>   
>>   	if (pdev->gadget.state == USB_STATE_NOTATTACHED) {
>>   		dev_err(pdev->dev, "ERR: Setup detected in unattached state\n");
>> -		ret = -EINVAL;
> 
> That's a nice change, but I don't see the original error here that you
> are saying this change fixes.
> 
> What am I missing?

The function has this check at its beginning:

        if (!pdev->gadget_driver)
                 goto out;

ret is initialized to 0 and len is uninitialized.
The jump goes to:

out:
         if (ret < 0)
                 cdnsp_ep0_stall(pdev);
         else if (!len && pdev->ep0_stage != CDNSP_STATUS_STAGE)
                 cdnsp_status_stage(pdev);


The compiler (and an analysis tool) can determine that len will be
evaluated in an uninitialized state. Setting ret to something
negative prevents that. I must say this is convoluted, even though
it is correct.

	HTH
		Oliver

Re: [PATCH] usb: cdnsp: Fixes error: uninitialized symbol 'len'

[Greg KH]

On Wed, Apr 05, 2023 at 07:41:53PM +0200, Oliver Neukum wrote:
> On 05.04.23 19:23, Greg KH wrote:
> > On Fri, Mar 31, 2023 at 05:06:00AM -0400, Pawel Laszczak wrote:
> 
> > >   {
> > >   	struct usb_ctrlrequest *ctrl = &pdev->setup;
> > > -	int ret = 0;
> > > +	int ret = -EINVAL;
> > >   	u16 len;
> > >   	trace_cdnsp_ctrl_req(ctrl);
> > > @@ -424,7 +424,6 @@ void cdnsp_setup_analyze(struct cdnsp_device *pdev)
> > >   	if (pdev->gadget.state == USB_STATE_NOTATTACHED) {
> > >   		dev_err(pdev->dev, "ERR: Setup detected in unattached state\n");
> > > -		ret = -EINVAL;
> > 
> > That's a nice change, but I don't see the original error here that you
> > are saying this change fixes.
> > 
> > What am I missing?
> 
> The function has this check at its beginning:
> 
>        if (!pdev->gadget_driver)
>                 goto out;

Argh, I missed this at the top of the function.  I was looking further
down, sorry for the noise.

I'll go queue this up now, thanks.

greg k-h

RE: [PATCH] usb: cdnsp: Fixes error: uninitialized symbol 'len'

[Pawel Laszczak]

>On Fri, Mar 31, 2023 at 05:06:00AM -0400, Pawel Laszczak wrote:
>> The patch 5bc38d33a5a1: "usb: cdnsp: Fixes issue with redundant Status
>> Stage" leads to the following Smatch static checker warning:
>>
>>   drivers/usb/cdns3/cdnsp-ep0.c:470 cdnsp_setup_analyze()
>>   error: uninitialized symbol 'len'.
>
>Are you sure this is correct?

Yes, I'm sure. 

>
>>
>> cc: <stable@vger.kernel.org>
>> Fixes: 5bc38d33a5a1 ("usb: cdnsp: Fixes issue with redundant Status
>> Stage")
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>  drivers/usb/cdns3/cdnsp-ep0.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c
>> b/drivers/usb/cdns3/cdnsp-ep0.c index d63d5d92f255..f317d3c84781
>> 100644
>> --- a/drivers/usb/cdns3/cdnsp-ep0.c
>> +++ b/drivers/usb/cdns3/cdnsp-ep0.c
>> @@ -414,7 +414,7 @@ static int cdnsp_ep0_std_request(struct
>> cdnsp_device *pdev,  void cdnsp_setup_analyze(struct cdnsp_device
>> *pdev)  {
>>  	struct usb_ctrlrequest *ctrl = &pdev->setup;
>> -	int ret = 0;
>> +	int ret = -EINVAL;
>>  	u16 len;
>>
>>  	trace_cdnsp_ctrl_req(ctrl);
>> @@ -424,7 +424,6 @@ void cdnsp_setup_analyze(struct cdnsp_device
>> *pdev)
>>
>>  	if (pdev->gadget.state == USB_STATE_NOTATTACHED) {
>>  		dev_err(pdev->dev, "ERR: Setup detected in unattached
>state\n");
>> -		ret = -EINVAL;
>
>That's a nice change, but I don't see the original error here that you are saying
>this change fixes.
>
>What am I missing?

The fixed patch is:
Commit:  5bc38d33a5a1209fd4de65101d1ae8255ea12c6e
And here you have the link to linux-next tree to this patch:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?id=5bc38d33a5a1209fd4de65101d1ae8255ea12c6e

I send this fix as v2 for patch "usb: cdnsp: Fixes issue with redundant Status Stage" but it was to late and you recommended me  to send this as separate patch.

Thanks and Regards,
Pawel