diff mbox series

[next] scsi: target: fix unsigned comparision with less than zero

Message ID 20190320163709.28767-1-colin.king@canonical.com (mailing list archive)
State Accepted
Commit ee26724af6c71a9db786c5809395b4853edd4d54
Headers show
Series [next] scsi: target: fix unsigned comparision with less than zero | expand

Commit Message

Colin King March 20, 2019, 4:37 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Currently an error return is being assigned to an unsigned
size_t varianle and then checked if the result is less than
zero which will always be false.  Fix this by making ret
ssize_t rather than a size_t.

Fixes: 0322913cab79 ("scsi: target: Add device product id and revision configfs attributes")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/target/target_core_configfs.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Mike Christie March 20, 2019, 5:14 p.m. UTC | #1 
On 03/20/2019 11:37 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently an error return is being assigned to an unsigned
> size_t varianle and then checked if the result is less than
> zero which will always be false.  Fix this by making ret

What kernel version was this made against?

For Martin's 5.2 queue branch, with these scsi changes it looks like
strlcpy returns a size_t. And then below it looks like we compare the
return value from that function to the buffer size and the max len of
the string we support. We do not seem to check for less than zero.


> ssize_t rather than a size_t.
> 
> Fixes: 0322913cab79 ("scsi: target: Add device product id and revision configfs attributes")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/target/target_core_configfs.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 8f3faef235b5..3fe79875b3ac 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1267,7 +1267,8 @@ static ssize_t target_wwn_vendor_id_store(struct config_item *item,
>  	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>  	unsigned char buf[INQUIRY_VENDOR_LEN + 2];
>  	char *stripped = NULL;
> -	size_t len, ret;
> +	size_t len;
> +	ssize_t ret;
>  
>  	len = strlcpy(buf, page, sizeof(buf));
>  	if (len < sizeof(buf)) {
> @@ -1322,7 +1323,8 @@ static ssize_t target_wwn_product_id_store(struct config_item *item,
>  	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>  	unsigned char buf[INQUIRY_MODEL_LEN + 2];
>  	char *stripped = NULL;
> -	size_t len, ret;
> +	size_t len;
> +	ssize_t ret;
>  
>  	len = strlcpy(buf, page, sizeof(buf));
>  	if (len < sizeof(buf)) {
> @@ -1377,7 +1379,8 @@ static ssize_t target_wwn_revision_store(struct config_item *item,
>  	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>  	unsigned char buf[INQUIRY_REVISION_LEN + 2];
>  	char *stripped = NULL;
> -	size_t len, ret;
> +	size_t len;
> +	ssize_t ret;
>  
>  	len = strlcpy(buf, page, sizeof(buf));
>  	if (len < sizeof(buf)) {
>
Colin King March 20, 2019, 5:14 p.m. UTC | #2 
On 20/03/2019 17:14, Mike Christie wrote:
> On 03/20/2019 11:37 AM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently an error return is being assigned to an unsigned
>> size_t varianle and then checked if the result is less than
>> zero which will always be false.  Fix this by making ret
> 
> What kernel version was this made against?

today's linux-next

> 
> For Martin's 5.2 queue branch, with these scsi changes it looks like
> strlcpy returns a size_t. And then below it looks like we compare the
> return value from that function to the buffer size and the max len of
> the string we support. We do not seem to check for less than zero.
> 
> 
>> ssize_t rather than a size_t.
>>
>> Fixes: 0322913cab79 ("scsi: target: Add device product id and revision configfs attributes")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/target/target_core_configfs.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>> index 8f3faef235b5..3fe79875b3ac 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -1267,7 +1267,8 @@ static ssize_t target_wwn_vendor_id_store(struct config_item *item,
>>  	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>>  	unsigned char buf[INQUIRY_VENDOR_LEN + 2];
>>  	char *stripped = NULL;
>> -	size_t len, ret;
>> +	size_t len;
>> +	ssize_t ret;
>>  
>>  	len = strlcpy(buf, page, sizeof(buf));
>>  	if (len < sizeof(buf)) {
>> @@ -1322,7 +1323,8 @@ static ssize_t target_wwn_product_id_store(struct config_item *item,
>>  	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>>  	unsigned char buf[INQUIRY_MODEL_LEN + 2];
>>  	char *stripped = NULL;
>> -	size_t len, ret;
>> +	size_t len;
>> +	ssize_t ret;
>>  
>>  	len = strlcpy(buf, page, sizeof(buf));
>>  	if (len < sizeof(buf)) {
>> @@ -1377,7 +1379,8 @@ static ssize_t target_wwn_revision_store(struct config_item *item,
>>  	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
>>  	unsigned char buf[INQUIRY_REVISION_LEN + 2];
>>  	char *stripped = NULL;
>> -	size_t len, ret;
>> +	size_t len;
>> +	ssize_t ret;
>>  
>>  	len = strlcpy(buf, page, sizeof(buf));
>>  	if (len < sizeof(buf)) {
>>
Mike Christie March 20, 2019, 5:15 p.m. UTC | #3 
On 03/20/2019 12:14 PM, Mike Christie wrote:
> On 03/20/2019 11:37 AM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently an error return is being assigned to an unsigned
>> size_t varianle and then checked if the result is less than
>> zero which will always be false.  Fix this by making ret
> 
> What kernel version was this made against?
> 
> For Martin's 5.2 queue branch, with these scsi changes it looks like
> strlcpy returns a size_t. And then below it looks like we compare the
> return value from that function to the buffer size and the max len of
> the string we support. We do not seem to check for less than zero.
> 
> 

My mistake. I was looking at len and not ret.

Patch looks ok to me.

Reviewed-by: Mike Christie <mchristi@redhat.com>
Martin K. Petersen March 21, 2019, 12:33 a.m. UTC | #4 
Colin,

> Currently an error return is being assigned to an unsigned size_t
> varianle and then checked if the result is less than zero which will
> always be false.  Fix this by making ret ssize_t rather than a size_t.

Applied to 5.2/scsi-queue, thanks!
diff mbox series

Patch

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 8f3faef235b5..3fe79875b3ac 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1267,7 +1267,8 @@  static ssize_t target_wwn_vendor_id_store(struct config_item *item,
 	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
 	unsigned char buf[INQUIRY_VENDOR_LEN + 2];
 	char *stripped = NULL;
-	size_t len, ret;
+	size_t len;
+	ssize_t ret;
 
 	len = strlcpy(buf, page, sizeof(buf));
 	if (len < sizeof(buf)) {
@@ -1322,7 +1323,8 @@  static ssize_t target_wwn_product_id_store(struct config_item *item,
 	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
 	unsigned char buf[INQUIRY_MODEL_LEN + 2];
 	char *stripped = NULL;
-	size_t len, ret;
+	size_t len;
+	ssize_t ret;
 
 	len = strlcpy(buf, page, sizeof(buf));
 	if (len < sizeof(buf)) {
@@ -1377,7 +1379,8 @@  static ssize_t target_wwn_revision_store(struct config_item *item,
 	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
 	unsigned char buf[INQUIRY_REVISION_LEN + 2];
 	char *stripped = NULL;
-	size_t len, ret;
+	size_t len;
+	ssize_t ret;
 
 	len = strlcpy(buf, page, sizeof(buf));
 	if (len < sizeof(buf)) {