Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581054 > unrolled thread

usb: storage: suspicious code

Started by"Gustavo A. R. Silva" <garsilva@embeddedor.com>
First post2017-02-15 06:10 +0100
Last post2017-02-21 00:40 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  usb: storage: suspicious code "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-02-15 06:10 +0100
    Re: [usb-storage] usb: storage: suspicious code Oliver Neukum <oneukum@suse.com> - 2017-02-15 08:10 +0100
      Re: [usb-storage] usb: storage: suspicious code "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-02-15 08:50 +0100
        [PATCH] usb: storage: add missing pre-increment to variable "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-02-15 09:10 +0100
          Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment  to variable Alan Stern <stern@rowland.harvard.edu> - 2017-02-15 16:30 +0100
            Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment  to variable "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-02-21 00:40 +0100

#1581054 — usb: storage: suspicious code

From"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date2017-02-15 06:10 +0100
Subjectusb: storage: suspicious code
Message-ID<tb00N-3Ky-1@gated-at.bofh.it>
Hello,

I ran into the following piece of code at drivers/usb/storage/jumpshot.c:305 (linux-next), and it seems a little bit suspicious:

// read the result.  apparently the bulk write can complete
// before the jumpshot drive is finished writing.  so we loop
// here until we get a good return code
waitcount = 0;
do {
	result = jumpshot_get_status(us);
	if (result != USB_STOR_TRANSPORT_GOOD) {
        	// I have not experimented to find the smallest value.
		//
		msleep(50);
	}
	} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));

	if (result != USB_STOR_TRANSPORT_GOOD)
        	usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");

Variable 'waitcount' is never updated inside the do-while loop. So, either it isn't needed at all or line 316 should be modified (++waitcount < 10)

In case 'waitcount' isn't needed, lines 318 and 319 should be removed.

Can someone help me to clarify this so I can write a patch to fix this code?

Thank you
--
Gustavo A. R. Silva

[toc] | [next] | [standalone]

#1581088 — Re: [usb-storage] usb: storage: suspicious code

FromOliver Neukum <oneukum@suse.com>
Date2017-02-15 08:10 +0100
SubjectRe: [usb-storage] usb: storage: suspicious code
Message-ID<tb1SV-50Q-7@gated-at.bofh.it>
In reply to#1581054
Am Dienstag, den 14.02.2017, 23:06 -0600 schrieb Gustavo A. R. Silva:

Hi,

> waitcount = 0;
> do {
>         result = jumpshot_get_status(us);
>         if (result != USB_STOR_TRANSPORT_GOOD) {
>                 // I have not experimented to find the smallest
> value.
>                 //
>                 msleep(50);
>         }
>         } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount <
> 10));
> 
>         if (result != USB_STOR_TRANSPORT_GOOD)
>                 usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad
> write!?\n");
> 
> Variable 'waitcount' is never updated inside the do-while loop. So,
> either it isn't needed at all or line 316 should be modified
> (++waitcount < 10)

you are correct. Waitcount needs to be incremented.

	HTH
		Oliver

[toc] | [prev] | [next] | [standalone]

#1581100 — Re: [usb-storage] usb: storage: suspicious code

From"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date2017-02-15 08:50 +0100
SubjectRe: [usb-storage] usb: storage: suspicious code
Message-ID<tb2vE-5gZ-11@gated-at.bofh.it>
In reply to#1581088
Hi Oliver,

Quoting Oliver Neukum <oneukum@suse.com>:

> Am Dienstag, den 14.02.2017, 23:06 -0600 schrieb Gustavo A. R. Silva:
>
> Hi,
>
>> waitcount = 0;
>> do {
>>         result = jumpshot_get_status(us);
>>         if (result != USB_STOR_TRANSPORT_GOOD) {
>>                 // I have not experimented to find the smallest
>> value.
>>                 //
>>                 msleep(50);
>>         }
>>         } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount <
>> 10));
>>
>>         if (result != USB_STOR_TRANSPORT_GOOD)
>>                 usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad
>> write!?\n");
>>
>> Variable 'waitcount' is never updated inside the do-while loop. So,
>> either it isn't needed at all or line 316 should be modified
>> (++waitcount < 10)
>
> you are correct. Waitcount needs to be incremented.
>

Thanks for clarifying, I'll send a patch shortly.

--
Gustavo A. R. Silva

[toc] | [prev] | [next] | [standalone]

#1581106 — [PATCH] usb: storage: add missing pre-increment to variable

From"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date2017-02-15 09:10 +0100
Subject[PATCH] usb: storage: add missing pre-increment to variable
Message-ID<tb2OZ-5Ei-9@gated-at.bofh.it>
In reply to#1581100
Add missing pre-increment to 'waitcount' variable used in do-while loop.

Addresses-Coverity-ID: 1011631
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/storage/jumpshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
index 011e527..a26c4bb 100644
--- a/drivers/usb/storage/jumpshot.c
+++ b/drivers/usb/storage/jumpshot.c
@@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
 				//
 				msleep(50); 
 			}
-		} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
+		} while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
 
 		if (result != USB_STOR_TRANSPORT_GOOD)
 			usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1581402 — Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable

FromAlan Stern <stern@rowland.harvard.edu>
Date2017-02-15 16:30 +0100
SubjectRe: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable
Message-ID<tb9GP-1II-37@gated-at.bofh.it>
In reply to#1581106
On Wed, 15 Feb 2017, Gustavo A. R. Silva wrote:

> Add missing pre-increment to 'waitcount' variable used in do-while loop.
> 
> Addresses-Coverity-ID: 1011631
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/usb/storage/jumpshot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
> index 011e527..a26c4bb 100644
> --- a/drivers/usb/storage/jumpshot.c
> +++ b/drivers/usb/storage/jumpshot.c
> @@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
>  				//
>  				msleep(50); 
>  			}
> -		} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
> +		} while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
>  
>  		if (result != USB_STOR_TRANSPORT_GOOD)
>  			usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");
> 

This has already been reported and fixed.  See

	http://marc.info/?l=linux-usb&m=148604164024557&w=2

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1584958 — Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable

From"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date2017-02-21 00:40 +0100
SubjectRe: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable
Message-ID<td5IK-2Kl-13@gated-at.bofh.it>
In reply to#1581402
Hi Alan,

Quoting Alan Stern <stern@rowland.harvard.edu>:

> On Wed, 15 Feb 2017, Gustavo A. R. Silva wrote:
>
>> Add missing pre-increment to 'waitcount' variable used in do-while loop.
>>
>> Addresses-Coverity-ID: 1011631
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>>  drivers/usb/storage/jumpshot.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
>> index 011e527..a26c4bb 100644
>> --- a/drivers/usb/storage/jumpshot.c
>> +++ b/drivers/usb/storage/jumpshot.c
>> @@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
>>  				//
>>  				msleep(50);
>>  			}
>> -		} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
>> +		} while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
>>
>>  		if (result != USB_STOR_TRANSPORT_GOOD)
>>  			usb_stor_dbg(us, "Gah!  Waitcount = 10.  Bad write!?\n");
>>
>
> This has already been reported and fixed.  See
>
> 	http://marc.info/?l=linux-usb&m=148604164024557&w=2
>

Awesome. Thanks for the info.
--
Gustavo A. R. Silva

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web