[PATCHv3 0/2] PCI: Safe VPD access

[PATCHv3 0/2] PCI: Safe VPD access

[Hannes Reinecke]

PCI VPD suffers from two problems: it has a very rudimentary interface
and it relies on correctly formatted data. And essentially it provides
a direct channel into the card hardware. In other words, plenty of
chances to mess things up.
With the original implementation we would just read the VPD space,
blissfully ignorant of the data formatting of the VPD data. This
would work if the VPD space happens to be properly implemented.
However, I've had several reports where a simple 'cat' on the vpd
attribute would return garbage (if you're lucky), trigger a timeout
with a kernel warning (which actually triggered this patchset), or
hang your machine (if you're really unlucky).
So this patchset validates the VPD data, setting the VPD attribute to
the correct size or disabling VPD access altogether if no valid data
is found.

Hannes Reinecke (2):
  pci: Update VPD definitions
  pci: Update VPD size with correct length

 drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  | 27 +++++++++++++++++++++++--
 2 files changed, 82 insertions(+), 2 deletions(-)

-- 
1.8.5.6

[PATCH 1/2] pci: Update VPD definitions

[Hannes Reinecke]

The 'end' tag is actually 0x0f, it's the representation as a
small resource data type tag that's 0x78 (ie shifted by 3).
This patch also adds helper functions to extract the resource
data type tags for both large and small resource data types.

Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 include/linux/pci.h | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6ae25aa..2fe2b5c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1836,12 +1836,13 @@ bool pci_acs_path_enabled(struct pci_dev *start,
 #define PCI_VPD_LRDT_RW_DATA		PCI_VPD_LRDT_ID(PCI_VPD_LTIN_RW_DATA)
 
 /* Small Resource Data Type Tag Item Names */
-#define PCI_VPD_STIN_END		0x78	/* End */
+#define PCI_VPD_STIN_END		0x0f	/* End */
 
-#define PCI_VPD_SRDT_END		PCI_VPD_STIN_END
+#define PCI_VPD_SRDT_END		(PCI_VPD_STIN_END << 3)
 
 #define PCI_VPD_SRDT_TIN_MASK		0x78
 #define PCI_VPD_SRDT_LEN_MASK		0x07
+#define PCI_VPD_LRDT_TIN_MASK		0x7f
 
 #define PCI_VPD_LRDT_TAG_SIZE		3
 #define PCI_VPD_SRDT_TAG_SIZE		1
@@ -1865,6 +1866,17 @@ static inline u16 pci_vpd_lrdt_size(const u8 *lrdt)
 }
 
 /**
+ * pci_vpd_lrdt_tag - Extracts the Large Resource Data Type Tag Item
+ * @lrdt: Pointer to the beginning of the Large Resource Data Type tag
+ *
+ * Returns the extracted Large Resource Data Type Tag item.
+ */
+static inline u16 pci_vpd_lrdt_tag(const u8 *lrdt)
+{
+    return (u16)(lrdt[0] & PCI_VPD_LRDT_TIN_MASK);
+}
+
+/**
  * pci_vpd_srdt_size - Extracts the Small Resource Data Type length
  * @lrdt: Pointer to the beginning of the Small Resource Data Type tag
  *
@@ -1876,6 +1888,17 @@ static inline u8 pci_vpd_srdt_size(const u8 *srdt)
 }
 
 /**
+ * pci_vpd_srdt_tag - Extracts the Small Resource Data Type Tag Item
+ * @lrdt: Pointer to the beginning of the Small Resource Data Type tag
+ *
+ * Returns the extracted Small Resource Data Type Tag Item.
+ */
+static inline u8 pci_vpd_srdt_tag(const u8 *srdt)
+{
+	return ((*srdt) & PCI_VPD_SRDT_TIN_MASK) >> 3;
+}
+
+/**
  * pci_vpd_info_field_size - Extracts the information field length
  * @lrdt: Pointer to the beginning of an information field header
  *
-- 
1.8.5.6

[PATCH 2/2] pci: Update VPD size with correct length

[Hannes Reinecke]

PCI-2.2 VPD entries have a maximum size of 32k, but might actually
be smaller than that. To figure out the actual size one has to read
the VPD area until the 'end marker' is reached.
Trying to read VPD data beyond that marker results in 'interesting'
effects, from simple read errors to crashing the card. And to make
matters worse not every PCI card implements this properly, leaving
us with no 'end' marker or even completely invalid data.
This path modifies the size of the VPD attribute to the available
size, and disables the VPD attribute altogether if no valid data
could be read.

Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 59ac36f..0a647b1 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -475,6 +475,56 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
 	.release = pci_vpd_pci22_release,
 };
 
+/**
+ * pci_vpd_size - determine actual size of Vital Product Data
+ * @dev:	pci device struct
+ * @old_size:	current assumed size, also maximum allowed size
+ *
+ */
+static size_t
+pci_vpd_pci22_size(struct pci_dev *dev)
+{
+	size_t off = 0;
+	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
+
+	while (off < PCI_VPD_PCI22_SIZE &&
+	       pci_read_vpd(dev, off, 1, header) == 1) {
+		unsigned char tag;
+
+		if (header[0] & PCI_VPD_LRDT) {
+			/* Large Resource Data Type Tag */
+			tag = pci_vpd_lrdt_tag(header);
+			/* Only read length from known tag items */
+			if ((tag == PCI_VPD_LTIN_ID_STRING) ||
+			    (tag == PCI_VPD_LTIN_RO_DATA) ||
+			    (tag == PCI_VPD_LTIN_RW_DATA)) {
+				if (pci_read_vpd(dev, off+1, 2,
+						 &header[1]) != 2)
+					return off + 1;
+				off += PCI_VPD_LRDT_TAG_SIZE +
+					pci_vpd_lrdt_size(header);
+			}
+		} else {
+			/* Short Resource Data Type Tag */
+			off += PCI_VPD_SRDT_TAG_SIZE +
+				pci_vpd_srdt_size(header);
+			tag = pci_vpd_srdt_tag(header);
+		}
+		if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
+			return off;
+		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
+		    (tag != PCI_VPD_LTIN_RO_DATA) &&
+		    (tag != PCI_VPD_LTIN_RW_DATA)) {
+			dev_dbg(&dev->dev,
+				"invalid %s vpd tag %02x at offset %zu.",
+				(header[0] & PCI_VPD_LRDT) ? "large" : "short",
+				tag, off);
+			break;
+		}
+	}
+	return 0;
+}
+
 int pci_vpd_pci22_init(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd;
@@ -497,6 +547,13 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	vpd->cap = cap;
 	vpd->busy = false;
 	dev->vpd = &vpd->base;
+	vpd->base.len = pci_vpd_pci22_size(dev);
+	if (vpd->base.len == 0) {
+		dev_dbg(&dev->dev, "Disabling VPD access.");
+		dev->vpd = NULL;
+		kfree(vpd);
+		return -ENXIO;
+	}
 	return 0;
 }
 
-- 
1.8.5.6

RE: [PATCH 1/2] pci: Update VPD definitions

[Seymour, Shane M]

> The 'end' tag is actually 0x0f, it's the representation as a
> small resource data type tag that's 0x78 (ie shifted by 3).
> This patch also adds helper functions to extract the resource
> data type tags for both large and small resource data types.
>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Tested-by: Shane Seymour <shane.seymour@hpe.com>

RE: [PATCH 2/2] pci: Update VPD size with correct length

[Seymour, Shane M]

Tested with a HP AE311-60001 PCIe card. It used to repeat the same VPD every 4k
for 32k now only the 154 bytes are returned and lspci -vvvv reports that the data up
to and including the end and that the check sum is good:

...
        Capabilities: [74] Vital Product Data
                Product Name: PCI-Express 4Gb Fibre Channel HBA
                Read-only fields:
                        [PN] Part number: AE311-60001
                        [EC] Engineering changes: C-4830
                        [SN] Serial number: CN80847W3U
                        [V0] Vendor specific: PW=15W
                        [V2] Vendor specific: 4847
                        [MN] Manufacture ID: 50 58 32 35 31 30 34 30 31 2d 37 30 20 20 42
                        [V1] Vendor specific: 02.22
                        [V3] Vendor specific: 05.03.15
                        [V4] Vendor specific: 03.13
                        [V5] Vendor specific: 02.03
                        [YA] Asset tag: NA
                        [RV] Reserved: checksum good, 0 byte(s) reserved
                End
...
---
Tested-by: Shane Seymour <shane.seymour@hpe.com>

Re: [PATCH 2/2] pci: Update VPD size with correct length

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

Hi Hannes,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.4-rc5 next-20151217]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/pci-Update-VPD-definitions/20151217-160050
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   lib/crc32.c:148: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:148: warning: Excess function parameter 'tab' description in 'crc32_le_generic'
   lib/crc32.c:293: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:293: warning: Excess function parameter 'tab' description in 'crc32_be_generic'
   lib/crc32.c:1: warning: no structured comments found
>> drivers/pci/access.c:486: warning: Excess function parameter 'old_size' description in 'pci_vpd_pci22_size'

vim +486 drivers/pci/access.c

   470	}
   471	
   472	static const struct pci_vpd_ops pci_vpd_f0_ops = {
   473		.read = pci_vpd_f0_read,
   474		.write = pci_vpd_f0_write,
   475		.release = pci_vpd_pci22_release,
   476	};
   477	
   478	/**
   479	 * pci_vpd_size - determine actual size of Vital Product Data
   480	 * @dev:	pci device struct
   481	 * @old_size:	current assumed size, also maximum allowed size
   482	 *
   483	 */
   484	static size_t
   485	pci_vpd_pci22_size(struct pci_dev *dev)
 > 486	{
   487		size_t off = 0;
   488		unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
   489	
   490		while (off < PCI_VPD_PCI22_SIZE &&
   491		       pci_read_vpd(dev, off, 1, header) == 1) {
   492			unsigned char tag;
   493	
   494			if (header[0] & PCI_VPD_LRDT) {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6125 bytes --]

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Alexander Duyck]

On Wed, Dec 16, 2015 at 11:59 PM, Hannes Reinecke <hare@suse.de> wrote:
> PCI-2.2 VPD entries have a maximum size of 32k, but might actually
> be smaller than that. To figure out the actual size one has to read
> the VPD area until the 'end marker' is reached.
> Trying to read VPD data beyond that marker results in 'interesting'
> effects, from simple read errors to crashing the card. And to make
> matters worse not every PCI card implements this properly, leaving
> us with no 'end' marker or even completely invalid data.
> This path modifies the size of the VPD attribute to the available
> size, and disables the VPD attribute altogether if no valid data
> could be read.
>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 59ac36f..0a647b1 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -475,6 +475,56 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
>         .release = pci_vpd_pci22_release,
>  };
>
> +/**
> + * pci_vpd_size - determine actual size of Vital Product Data
> + * @dev:       pci device struct
> + * @old_size:  current assumed size, also maximum allowed size
> + *

"old_siz"e was dropped so you can remove this line.

> + */
> +static size_t
> +pci_vpd_pci22_size(struct pci_dev *dev)
> +{
> +       size_t off = 0;
> +       unsigned char header[1+2];      /* 1 byte tag, 2 bytes length */
> +
> +       while (off < PCI_VPD_PCI22_SIZE &&
> +              pci_read_vpd(dev, off, 1, header) == 1) {
> +               unsigned char tag;
> +

The offset comparison is probably redundant.  There is already a check
in pci_vpd_pci22_read that will check the offset and return -EINVAL if
we have exceeded vpd->base.len.  As such you can probably just do the
pci_read_vpd comparison and drop the offset length entirely.

> +               if (header[0] & PCI_VPD_LRDT) {
> +                       /* Large Resource Data Type Tag */
> +                       tag = pci_vpd_lrdt_tag(header);
> +                       /* Only read length from known tag items */
> +                       if ((tag == PCI_VPD_LTIN_ID_STRING) ||
> +                           (tag == PCI_VPD_LTIN_RO_DATA) ||
> +                           (tag == PCI_VPD_LTIN_RW_DATA)) {
> +                               if (pci_read_vpd(dev, off+1, 2,
> +                                                &header[1]) != 2)
> +                                       return off + 1;
> +                               off += PCI_VPD_LRDT_TAG_SIZE +
> +                                       pci_vpd_lrdt_size(header);
> +                       }
> +               } else {
> +                       /* Short Resource Data Type Tag */
> +                       off += PCI_VPD_SRDT_TAG_SIZE +
> +                               pci_vpd_srdt_size(header);
> +                       tag = pci_vpd_srdt_tag(header);
> +               }
> +               if (tag == PCI_VPD_STIN_END)    /* End tag descriptor */
> +                       return off;
> +               if ((tag != PCI_VPD_LTIN_ID_STRING) &&
> +                   (tag != PCI_VPD_LTIN_RO_DATA) &&
> +                   (tag != PCI_VPD_LTIN_RW_DATA)) {
> +                       dev_dbg(&dev->dev,
> +                               "invalid %s vpd tag %02x at offset %zu.",
> +                               (header[0] & PCI_VPD_LRDT) ? "large" : "short",
> +                               tag, off);
> +                       break;
> +               }
> +       }
> +       return 0;
> +}
> +
>  int pci_vpd_pci22_init(struct pci_dev *dev)
>  {
>         struct pci_vpd_pci22 *vpd;
> @@ -497,6 +547,13 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>         vpd->cap = cap;
>         vpd->busy = false;
>         dev->vpd = &vpd->base;
> +       vpd->base.len = pci_vpd_pci22_size(dev);
> +       if (vpd->base.len == 0) {
> +               dev_dbg(&dev->dev, "Disabling VPD access.");
> +               dev->vpd = NULL;
> +               kfree(vpd);
> +               return -ENXIO;
> +       }
>         return 0;
>  }

It looks like this still doesn't address the VPD_REF_F0 issue I
mentioned earlier.  We don't need to compute the length for each
function we only need to do it once.  I would recommend modifying
things so that you set vpd->base.len to 0 if the VPD_REF_F0 flag is
set.

Also I wouldn't delete the vpd configuration if the length is not
correct as that will likely break several quirks that already exist
that are setting the length.  Also there is no need to return an
error, the fact is the part has VPD but we cannot determine the length
as such the correct solution is to leave it at 0.  We can leave that
for a quirk to sort out later if needed.  You could probably move the
dev_dbg message to just before the return 0 in the pci_vpd_pci22_size
call and drop the entire if statement in the init function.

- Alex

Re: [PATCH 2/2] pci: Update VPD size with correct length

[Hannes Reinecke]

On 12/17/2015 06:13 PM, Alexander Duyck wrote:
> On Wed, Dec 16, 2015 at 11:59 PM, Hannes Reinecke <hare@suse.de> wrote:
>> PCI-2.2 VPD entries have a maximum size of 32k, but might actually
>> be smaller than that. To figure out the actual size one has to read
>> the VPD area until the 'end marker' is reached.
>> Trying to read VPD data beyond that marker results in 'interesting'
>> effects, from simple read errors to crashing the card. And to make
>> matters worse not every PCI card implements this properly, leaving
>> us with no 'end' marker or even completely invalid data.
>> This path modifies the size of the VPD attribute to the available
>> size, and disables the VPD attribute altogether if no valid data
>> could be read.
>>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Cc: Bjorn Helgaas <helgaas@kernel.org>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/pci/access.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index 59ac36f..0a647b1 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -475,6 +475,56 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
>>          .release = pci_vpd_pci22_release,
>>   };
>>
>> +/**
>> + * pci_vpd_size - determine actual size of Vital Product Data
>> + * @dev:       pci device struct
>> + * @old_size:  current assumed size, also maximum allowed size
>> + *
>
> "old_siz"e was dropped so you can remove this line.
>
>> + */
>> +static size_t
>> +pci_vpd_pci22_size(struct pci_dev *dev)
>> +{
>> +       size_t off = 0;
>> +       unsigned char header[1+2];      /* 1 byte tag, 2 bytes length */
>> +
>> +       while (off < PCI_VPD_PCI22_SIZE &&
>> +              pci_read_vpd(dev, off, 1, header) == 1) {
>> +               unsigned char tag;
>> +
>
> The offset comparison is probably redundant.  There is already a check
> in pci_vpd_pci22_read that will check the offset and return -EINVAL if
> we have exceeded vpd->base.len.  As such you can probably just do the
> pci_read_vpd comparison and drop the offset length entirely.
>
Indeed it does. Will be doing so.

>> +               if (header[0] & PCI_VPD_LRDT) {
>> +                       /* Large Resource Data Type Tag */
>> +                       tag = pci_vpd_lrdt_tag(header);
>> +                       /* Only read length from known tag items */
>> +                       if ((tag == PCI_VPD_LTIN_ID_STRING) ||
>> +                           (tag == PCI_VPD_LTIN_RO_DATA) ||
>> +                           (tag == PCI_VPD_LTIN_RW_DATA)) {
>> +                               if (pci_read_vpd(dev, off+1, 2,
>> +                                                &header[1]) != 2)
>> +                                       return off + 1;
>> +                               off += PCI_VPD_LRDT_TAG_SIZE +
>> +                                       pci_vpd_lrdt_size(header);
>> +                       }
>> +               } else {
>> +                       /* Short Resource Data Type Tag */
>> +                       off += PCI_VPD_SRDT_TAG_SIZE +
>> +                               pci_vpd_srdt_size(header);
>> +                       tag = pci_vpd_srdt_tag(header);
>> +               }
>> +               if (tag == PCI_VPD_STIN_END)    /* End tag descriptor */
>> +                       return off;
>> +               if ((tag != PCI_VPD_LTIN_ID_STRING) &&
>> +                   (tag != PCI_VPD_LTIN_RO_DATA) &&
>> +                   (tag != PCI_VPD_LTIN_RW_DATA)) {
>> +                       dev_dbg(&dev->dev,
>> +                               "invalid %s vpd tag %02x at offset %zu.",
>> +                               (header[0] & PCI_VPD_LRDT) ? "large" : "short",
>> +                               tag, off);
>> +                       break;
>> +               }
>> +       }
>> +       return 0;
>> +}
>> +
>>   int pci_vpd_pci22_init(struct pci_dev *dev)
>>   {
>>          struct pci_vpd_pci22 *vpd;
>> @@ -497,6 +547,13 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>>          vpd->cap = cap;
>>          vpd->busy = false;
>>          dev->vpd = &vpd->base;
>> +       vpd->base.len = pci_vpd_pci22_size(dev);
>> +       if (vpd->base.len == 0) {
>> +               dev_dbg(&dev->dev, "Disabling VPD access.");
>> +               dev->vpd = NULL;
>> +               kfree(vpd);
>> +               return -ENXIO;
>> +       }
>>          return 0;
>>   }
>
> It looks like this still doesn't address the VPD_REF_F0 issue I
> mentioned earlier.  We don't need to compute the length for each
> function we only need to do it once.  I would recommend modifying
> things so that you set vpd->base.len to 0 if the VPD_REF_F0 flag is
> set.
>
But that would effectively inhibit access to the VPD on those 
devices, rendering the entire 'f0_ops' thingie quite pointless, right?

I think it's better to directly retrieve the VPD length from the 
base pci device, that would give us the correct length _and_ save 
duplicate calculations.

> Also I wouldn't delete the vpd configuration if the length is not
> correct as that will likely break several quirks that already exist
> that are setting the length.  Also there is no need to return an
> error, the fact is the part has VPD but we cannot determine the length
> as such the correct solution is to leave it at 0.  We can leave that
> for a quirk to sort out later if needed.  You could probably move the
> dev_dbg message to just before the return 0 in the pci_vpd_pci22_size
> call and drop the entire if statement in the init function.
>
Okay.
Will be sending a new patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)