scsi: core: Query VPD size before getting full page

We currently default to 255 bytes when fetching VPD pages during discovery.
However, we have had a few devices that are known to wedge if the requested
buffer exceeds a certain size. See commit e23ccf036d5f ("[SCSI] sd: Reduce
buffer size for vpd request") which works around one example of this
problem in the SCSI disk driver.

With commit a2200cbc64bf ("scsi: core: Add sysfs attributes for VPD pages
0h and 89h") we now risk triggering the same issue in the generic midlayer
code.

The problem with the ATA VPD page in particular is that the SCSI portion of
the page is trailed by 512 bytes of verbatim ATA Identify Device
information.  However, not all controllers actually provide the additional
512 bytes and will lock up if one asks for more than the 64 bytes
containing the SCSI protocol fields.

Instead of picking a new, somewhat arbitrary, number of bytes for the VPD
buffer size, start fetching the 4-byte header for each page. The header
contains the size of the page as far as the device is concerned. We can use
the reported size to specify the correct allocation length when
subsequently fetching the full page.

The header validation is done by a new helper function scsi_get_vpd_size()
and both scsi_get_vpd_page() and scsi_get_vpd_buf() now rely on this to
query the page size.

In addition, scsi_get_vpd_page() is simplified to mirror the logic in
scsi_get_vpd_page(). This involves removing the Supported VPD Pages lookup
prior to attempting to query a page. There does not appear any evidence,
even in the oldest SCSI specs, that this step is required. We already rely
on scsi_get_vpd_page() throughout the stack and this function never
consulted the Supported VPD Pages. Since this has not caused any problems
it should be safe to remove the precondition from scsi_get_vpd_page().

Instrumented runs also revealed that the Supported VPD Pages lookup had
little effect since the device page index often was larger than the
supplied buffer size. As a result, inquiries frequently bypassed the index
check and went through the "If we ran off the end of the buffer, give us
the benefit of the doubt" code path which assumed the page was present
despite not being listed. The revised code takes both the page size
reported by the device as well as the size of the buffer provided by the
scsi_get_vpd_page() caller into account.

Link: https://lore.kernel.org/r/20220302053559.32147-3-martin.petersen@oracle.com
Fixes: a2200cbc64bf ("scsi: core: Add sysfs attributes for VPD pages 0h and 89h")
Reported-by: Maciej W. Rozycki <macro@orcam.me.uk>
Tested-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 735b6c862b4606f87bb9810c6d254dbf8b9c328d..a17392880da27b6961a113cc5812c755f2c03be9 100644 (file)
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -321,6 +321,31 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer,
        return get_unaligned_be16(&buffer[2]) + 4;
 }
 
+static int scsi_get_vpd_size(struct scsi_device *sdev, u8 page)
+{
+       unsigned char vpd_header[SCSI_VPD_HEADER_SIZE] __aligned(4);
+       int result;
+
+       /*
+        * Fetch the VPD page header to find out how big the page
+        * is. This is done to prevent problems on legacy devices
+        * which can not handle allocation lengths as large as
+        * potentially requested by the caller.
+        */
+       result = scsi_vpd_inquiry(sdev, vpd_header, page, sizeof(vpd_header));
+       if (result < 0)
+               return 0;
+
+       if (result < SCSI_VPD_HEADER_SIZE) {
+               dev_warn_once(&sdev->sdev_gendev,
+                             "%s: short VPD page 0x%02x length: %d bytes\n",
+                             __func__, page, result);
+               return 0;
+       }
+
+       return result;
+}
+
 /**
  * scsi_get_vpd_page - Get Vital Product Data from a SCSI device
  * @sdev: The device to ask
@@ -330,47 +355,38 @@ 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;
-
-       if (sdev->skip_vpd_pages)
-               goto fail;
+       int result, vpd_len;
 
-       /* Ask for all the pages supported by this device */
-       result = scsi_vpd_inquiry(sdev, buf, 0, buf_len);
-       if (result < 4)
-               goto fail;
-
-       /* If the user actually wanted this page, we can skip the rest */
-       if (page == 0)
-               return 0;
+       if (!scsi_device_supports_vpd(sdev))
+               return -EINVAL;
 
-       for (i = 4; i < min(result, buf_len); i++)
-               if (buf[i] == page)
-                       goto found;
+       vpd_len = scsi_get_vpd_size(sdev, page);
+       if (vpd_len <= 0)
+               return -EINVAL;
 
-       if (i < result && i >= buf_len)
-               /* ran off the end of the buffer, give us benefit of doubt */
-               goto found;
-       /* The device claims it doesn't support the requested page */
-       goto fail;
+       vpd_len = min(vpd_len, buf_len);
 
- found:
-       result = scsi_vpd_inquiry(sdev, buf, page, buf_len);
+       /*
+        * Fetch the actual page. Since the appropriate size was reported
+        * by the device it is now safe to ask for something bigger.
+        */
+       memset(buf, 0, buf_len);
+       result = scsi_vpd_inquiry(sdev, buf, page, vpd_len);
        if (result < 0)
-               goto fail;
+               return -EINVAL;
+       else if (result > vpd_len)
+               dev_warn_once(&sdev->sdev_gendev,
+                             "%s: VPD page 0x%02x result %d > %d bytes\n",
+                             __func__, page, result, vpd_len);
 
        return 0;
-
- fail:
-       return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 
@@ -384,9 +400,17 @@ EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
 static struct scsi_vpd *scsi_get_vpd_buf(struct scsi_device *sdev, u8 page)
 {
        struct scsi_vpd *vpd_buf;
-       int vpd_len = SCSI_VPD_PG_LEN, result;
+       int vpd_len, result;
+
+       vpd_len = scsi_get_vpd_size(sdev, page);
+       if (vpd_len <= 0)
+               return NULL;
 
 retry_pg:
+       /*
+        * Fetch the actual page. Since the appropriate size was reported
+        * by the device it is now safe to ask for something bigger.
+        */
        vpd_buf = kmalloc(sizeof(*vpd_buf) + vpd_len, GFP_KERNEL);
        if (!vpd_buf)
                return NULL;
@@ -397,6 +421,9 @@ retry_pg:
                return NULL;
        }
        if (result > vpd_len) {
+               dev_warn_once(&sdev->sdev_gendev,
+                             "%s: VPD page 0x%02x result %d > %d bytes\n",
+                             __func__, page, result, vpd_len);
                vpd_len = result;
                kfree(vpd_buf);
                goto retry_pg;

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 57e3e239a1fce64972b82dfa1c43028888edd1e9..c37cc5b3ebebb67f7a2d7a2f38580498264e3f33 100644 (file)
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -100,6 +100,10 @@ struct scsi_vpd {
        unsigned char   data[];
 };
 
+enum scsi_vpd_parameters {
+       SCSI_VPD_HEADER_SIZE = 4,
+};
+
 struct scsi_device {
        struct Scsi_Host *host;
        struct request_queue *request_queue;
@@ -141,7 +145,6 @@ struct scsi_device {
        const char * model;             /* ... after scan; point to static string */
        const char * rev;               /* ... "nullnullnullnull" before scan */
 
-#define SCSI_VPD_PG_LEN                255
        struct scsi_vpd __rcu *vpd_pg0;
        struct scsi_vpd __rcu *vpd_pg83;
        struct scsi_vpd __rcu *vpd_pg80;