[PATCH v2] scsi: simplify scsi_get_vpd_page()

[PATCH v2] scsi: simplify scsi_get_vpd_page()

[Damien Le Moal]

Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
readability. Also use memchr() instead of an open coded search loop
and update the outdated kernel doc comment for this function.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---

Changes from v1:
* Keep the "found" goto and use memchr() in place of the page search
  loop, as suggested by Bart.
* Update the patch commit title and message

 drivers/scsi/scsi.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d26025cf5de3..4946d8c4f298 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -339,47 +339,43 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
  *
  * SCSI devices may optionally supply Vital Product Data.  Each 'page'
  * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
- * If the device supports this VPD page, this routine returns a pointer
- * to a buffer containing the data from that page.  The caller is
- * responsible for calling kfree() on this pointer when it is no longer
- * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
+ * If the device supports this VPD page, this routine fills @buf
+ * with the data from that page and return 0. If the VPD page is not
+ * supported or its content cannot be retrieved, -EINVAL is returned.
  */
 int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 		      int buf_len)
 {
-	int i, result;
+	int result;
 
 	if (sdev->skip_vpd_pages)
-		goto fail;
+		return -EINVAL;
 
 	/* Ask for all the pages supported by this device */
 	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
 	if (result < 4)
-		goto fail;
+		return -EINVAL;
 
 	/* If the user actually wanted this page, we can skip the rest */
 	if (page == 0)
 		return 0;
 
-	for (i = 4; i < min(result, buf_len); i++)
-		if (buf[i] == page)
-			goto found;
+	if (memchr(&buf[4], page, min(result, buf_len)))
+		goto found;
 
-	if (i < result && i >= buf_len)
-		/* ran off the end of the buffer, give us benefit of doubt */
+	/* If we ran off the end of the buffer, give us benefit of doubt */
+	if (result > buf_len)
 		goto found;
+
 	/* The device claims it doesn't support the requested page */
-	goto fail;
+	return -EINVAL;
 
  found:
 	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
 	if (result < 0)
-		goto fail;
+		return -EINVAL;
 
 	return 0;
-
- fail:
-	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 
-- 
2.31.1

Re: [PATCH v2] scsi: simplify scsi_get_vpd_page()

[Niklas Cassel]

On Fri, Aug 13, 2021 at 10:07:38AM +0900, Damien Le Moal wrote:
> Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
> readability. Also use memchr() instead of an open coded search loop
> and update the outdated kernel doc comment for this function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> 
> Changes from v1:
> * Keep the "found" goto and use memchr() in place of the page search
>   loop, as suggested by Bart.
> * Update the patch commit title and message
> 
>  drivers/scsi/scsi.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index d26025cf5de3..4946d8c4f298 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -339,47 +339,43 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>   *
>   * SCSI devices may optionally supply Vital Product Data.  Each 'page'
>   * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
> - * If the device supports this VPD page, this routine returns a pointer
> - * to a buffer containing the data from that page.  The caller is
> - * responsible for calling kfree() on this pointer when it is no longer
> - * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
> + * If the device supports this VPD page, this routine fills @buf
> + * with the data from that page and return 0. If the VPD page is not
> + * supported or its content cannot be retrieved, -EINVAL is returned.
>   */
>  int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>  		      int buf_len)
>  {
> -	int i, result;
> +	int result;
>  
>  	if (sdev->skip_vpd_pages)
> -		goto fail;
> +		return -EINVAL;
>  
>  	/* Ask for all the pages supported by this device */
>  	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
>  	if (result < 4)
> -		goto fail;
> +		return -EINVAL;
>  
>  	/* If the user actually wanted this page, we can skip the rest */
>  	if (page == 0)
>  		return 0;
>  
> -	for (i = 4; i < min(result, buf_len); i++)
> -		if (buf[i] == page)
> -			goto found;
> +	if (memchr(&buf[4], page, min(result, buf_len)))

Hello Damien,

This will try to access data outside the buffer.

When starting from index 4, you cannot search buf_len number of elements.


Kind regards,
Niklas

Re: [PATCH v2] scsi: simplify scsi_get_vpd_page()

[Damien Le Moal]

On 2021/08/19 23:42, Niklas Cassel wrote:
> On Fri, Aug 13, 2021 at 10:07:38AM +0900, Damien Le Moal wrote:
>> Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
>> readability. Also use memchr() instead of an open coded search loop
>> and update the outdated kernel doc comment for this function.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>
>> Changes from v1:
>> * Keep the "found" goto and use memchr() in place of the page search
>>   loop, as suggested by Bart.
>> * Update the patch commit title and message
>>
>>  drivers/scsi/scsi.c | 30 +++++++++++++-----------------
>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index d26025cf5de3..4946d8c4f298 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -339,47 +339,43 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>>   *
>>   * SCSI devices may optionally supply Vital Product Data.  Each 'page'
>>   * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
>> - * If the device supports this VPD page, this routine returns a pointer
>> - * to a buffer containing the data from that page.  The caller is
>> - * responsible for calling kfree() on this pointer when it is no longer
>> - * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
>> + * If the device supports this VPD page, this routine fills @buf
>> + * with the data from that page and return 0. If the VPD page is not
>> + * supported or its content cannot be retrieved, -EINVAL is returned.
>>   */
>>  int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>>  		      int buf_len)
>>  {
>> -	int i, result;
>> +	int result;
>>  
>>  	if (sdev->skip_vpd_pages)
>> -		goto fail;
>> +		return -EINVAL;
>>  
>>  	/* Ask for all the pages supported by this device */
>>  	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
>>  	if (result < 4)
>> -		goto fail;
>> +		return -EINVAL;
>>  
>>  	/* If the user actually wanted this page, we can skip the rest */
>>  	if (page == 0)
>>  		return 0;
>>  
>> -	for (i = 4; i < min(result, buf_len); i++)
>> -		if (buf[i] == page)
>> -			goto found;
>> +	if (memchr(&buf[4], page, min(result, buf_len)))
> 
> Hello Damien,
> 
> This will try to access data outside the buffer.
> 
> When starting from index 4, you cannot search buf_len number of elements.

Aouch, indeed, good catch. Sending V2 to correct that.

> 
> 
> Kind regards,
> Niklas
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] scsi: simplify scsi_get_vpd_page()

[Niklas Cassel]

On Fri, Aug 20, 2021 at 12:21:54PM +0900, Damien Le Moal wrote:
> Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
> readability and use memchr() instead of an open coded search loop.
> Also update the outdated kernel doc comment for this function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
> Changes from v1:
> * Fix memchr() use to avoid accesses beyond the buffer and result size.
> 
>  drivers/scsi/scsi.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index b241f9e3885c..6be68b3427a0 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -339,47 +339,44 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
>   *
>   * SCSI devices may optionally supply Vital Product Data.  Each 'page'
>   * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
> - * If the device supports this VPD page, this routine returns a pointer
> - * to a buffer containing the data from that page.  The caller is
> - * responsible for calling kfree() on this pointer when it is no longer
> - * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
> + * If the device supports this VPD page, this routine fills @buf
> + * with the data from that page and return 0. If the VPD page is not
> + * supported or its content cannot be retrieved, -EINVAL is returned.
>   */
>  int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
>  		      int buf_len)
>  {
> -	int i, result;
> +	int result, len;
>  
>  	if (sdev->skip_vpd_pages)
> -		goto fail;
> +		return -EINVAL;
>  
>  	/* Ask for all the pages supported by this device */
>  	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
>  	if (result < 4)
> -		goto fail;
> +		return -EINVAL;
>  
>  	/* If the user actually wanted this page, we can skip the rest */
>  	if (page == 0)
>  		return 0;
>  
> -	for (i = 4; i < min(result, buf_len); i++)
> -		if (buf[i] == page)
> -			goto found;
> +	len = min(result, buf_len);
> +	if (len > 4 && memchr(&buf[4], page, len - 4))
> +		goto found;
>  
> -	if (i < result && i >= buf_len)
> -		/* ran off the end of the buffer, give us benefit of doubt */
> +	/* If we ran off the end of the buffer, give us benefit of doubt */
> +	if (result > buf_len)
>  		goto found;
> +
>  	/* The device claims it doesn't support the requested page */
> -	goto fail;
> +	return -EINVAL;
>  
>   found:
>  	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
>  	if (result < 0)
> -		goto fail;
> +		return -EINVAL;
>  
>  	return 0;
> -
> - fail:
> -	return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
>  

(Nit: I think you forgot to bump the version. Subject for this is v2, which
is the same as the previous "scsi: simplify scsi_get_vpd_page()" patch.)

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

[PATCH v2] scsi: simplify scsi_get_vpd_page()

[Damien Le Moal]

Remove unnecessary gotos in scsi_get_vpd_page() to improve the code
readability and use memchr() instead of an open coded search loop.
Also update the outdated kernel doc comment for this function.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
Changes from v1:
* Fix memchr() use to avoid accesses beyond the buffer and result size.

 drivers/scsi/scsi.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b241f9e3885c..6be68b3427a0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -339,47 +339,44 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
  *
  * SCSI devices may optionally supply Vital Product Data.  Each 'page'
  * of VPD is defined in the appropriate SCSI document (eg SPC, SBC).
- * If the device supports this VPD page, this routine returns a pointer
- * to a buffer containing the data from that page.  The caller is
- * responsible for calling kfree() on this pointer when it is no longer
- * needed.  If we cannot retrieve the VPD page this routine returns %NULL.
+ * If the device supports this VPD page, this routine fills @buf
+ * with the data from that page and return 0. If the VPD page is not
+ * supported or its content cannot be retrieved, -EINVAL is returned.
  */
 int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 		      int buf_len)
 {
-	int i, result;
+	int result, len;
 
 	if (sdev->skip_vpd_pages)
-		goto fail;
+		return -EINVAL;
 
 	/* Ask for all the pages supported by this device */
 	result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
 	if (result < 4)
-		goto fail;
+		return -EINVAL;
 
 	/* If the user actually wanted this page, we can skip the rest */
 	if (page == 0)
 		return 0;
 
-	for (i = 4; i < min(result, buf_len); i++)
-		if (buf[i] == page)
-			goto found;
+	len = min(result, buf_len);
+	if (len > 4 && memchr(&buf[4], page, len - 4))
+		goto found;
 
-	if (i < result && i >= buf_len)
-		/* ran off the end of the buffer, give us benefit of doubt */
+	/* If we ran off the end of the buffer, give us benefit of doubt */
+	if (result > buf_len)
 		goto found;
+
 	/* The device claims it doesn't support the requested page */
-	goto fail;
+	return -EINVAL;
 
  found:
 	result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
 	if (result < 0)
-		goto fail;
+		return -EINVAL;
 
 	return 0;
-
- fail:
-	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 
-- 
2.31.1