| Message ID | 20210813010738.1204219-1-damien.lemoal@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] scsi: simplify scsi_get_vpd_page() | expand |
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
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 >
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);
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(-)