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: <[email protected]>
Fixes: 5bc38d33a5a1 ("usb: cdnsp: Fixes issue with redundant Status Stage")
Signed-off-by: Pawel Laszczak <[email protected]>
---
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
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: <[email protected]>
> Fixes: 5bc38d33a5a1 ("usb: cdnsp: Fixes issue with redundant Status Stage")
> Signed-off-by: Pawel Laszczak <[email protected]>
> ---
> 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
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
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
>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: <[email protected]>
>> Fixes: 5bc38d33a5a1 ("usb: cdnsp: Fixes issue with redundant Status
>> Stage")
>> Signed-off-by: Pawel Laszczak <[email protected]>
>> ---
>> 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