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@xxxxxxx>
> ---
>
> 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
