[PATCH 0/5] PCI/VPD: Further improvements

[PATCH 0/5] PCI/VPD: Further improvements

[Heiner Kallweit]

Series with further improvements to PCI VPD handling.

Heiner Kallweit (5):
  PCI/VPD: Refactor pci_vpd_size
  PCI: Clean up VPD constants and functions in pci.h
  PCI/VPD: Remove old_size argument from pci_vpd_size
  PCI/VPD: Make pci_vpd_wait uninterruptible
  PCI/VPD: Remove pci_vpd member flag

 drivers/pci/vpd.c   | 106 ++++++++++++++++----------------------------
 include/linux/pci.h |  43 ------------------
 2 files changed, 37 insertions(+), 112 deletions(-)

-- 
2.31.1

[PATCH 2/5] PCI: Clean up VPD constants and functions in pci.h

[Heiner Kallweit]

Move some constants that are used by vpd.c only from include/linux/pci.h
to drivers/pci/vpd.c. In addition remove some unused VPD inline functions.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c   |  7 +++++++
 include/linux/pci.h | 43 -------------------------------------------
 2 files changed, 7 insertions(+), 43 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index ecdce170f..ff537371c 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -13,6 +13,13 @@
 
 /* VPD access through PCI 2.2+ VPD capability */
 
+/* Small Resource Data Type */
+#define PCI_VPD_SRDT_TAG_SIZE	1
+#define PCI_VPD_SRDT_END	(0x0f << 3) /* end tag */
+
+/* Large Resource Data Type */
+#define PCI_VPD_LRDT_TIN_MASK	0x7f
+
 struct pci_vpd_ops {
 	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c20211e59..c21558821 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2240,17 +2240,7 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask);
 #define PCI_VPD_LRDT_RO_DATA		PCI_VPD_LRDT_ID(PCI_VPD_LTIN_RO_DATA)
 #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		0x0f	/* 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
 
 #define PCI_VPD_INFO_FLD_HDR_SIZE	3
 
@@ -2271,39 +2261,6 @@ static inline u16 pci_vpd_lrdt_size(const u8 *lrdt)
 	return (u16)lrdt[1] + ((u16)lrdt[2] << 8);
 }
 
-/**
- * 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
- * @srdt: Pointer to the beginning of the Small Resource Data Type tag
- *
- * Returns the extracted Small Resource Data Type length.
- */
-static inline u8 pci_vpd_srdt_size(const u8 *srdt)
-{
-	return (*srdt) & PCI_VPD_SRDT_LEN_MASK;
-}
-
-/**
- * pci_vpd_srdt_tag - Extracts the Small Resource Data Type Tag Item
- * @srdt: 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
  * @info_field: Pointer to the beginning of an information field header
-- 
2.31.1

[PATCH 3/5] PCI/VPD: Remove old_size argument from pci_vpd_size

[Heiner Kallweit]

vpd->len is initialized to PCI_VPD_MAX_SIZE, and if a quirk is used to
set a specific VPD size, then pci_vpd_set_size() sets vpd->valid,
resulting in pci_vpd_size() not being called. Therefore we can remove
the old_size argument. Note that we don't have to check
off < PCI_VPD_MAX_SIZE because that's implicitly done by pci_read_vpd().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index ff537371c..e73a3a55f 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -75,14 +75,13 @@ EXPORT_SYMBOL(pci_write_vpd);
 /**
  * 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_size(struct pci_dev *dev, size_t old_size)
+static size_t pci_vpd_size(struct pci_dev *dev)
 {
 	size_t off = 0;
 	u8 header[3];	/* 1 byte tag, 2 bytes length */
 
-	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
+	while (pci_read_vpd(dev, off, 1, header) == 1) {
 		if (!header[0] && !off) {
 			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
 			return 0;
@@ -164,7 +163,7 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 
 	if (!vpd->valid) {
 		vpd->valid = 1;
-		vpd->len = pci_vpd_size(dev, vpd->len);
+		vpd->len = pci_vpd_size(dev);
 	}
 
 	if (vpd->len == 0)
@@ -231,7 +230,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 
 	if (!vpd->valid) {
 		vpd->valid = 1;
-		vpd->len = pci_vpd_size(dev, vpd->len);
+		vpd->len = pci_vpd_size(dev);
 	}
 
 	if (vpd->len == 0)
@@ -455,6 +454,7 @@ static void quirk_blacklist_vpd(struct pci_dev *dev)
 {
 	if (dev->vpd) {
 		dev->vpd->len = 0;
+		dev->vpd->valid = 1;
 		pci_warn(dev, FW_BUG "disabling VPD access (can't determine size of non-standard VPD format)\n");
 	}
 }
-- 
2.31.1

[PATCH 4/5] PCI/VPD: Make pci_vpd_wait uninterruptible

[Heiner Kallweit]

Reading/writing 4 bytes should be fast enough even on a slow bus,
therefore pci_vpd_wait() doesn't have to be interruptible.
Making it uninterruptible allows to simplify the code.
In addition make VPD writes uninterruptible in general.
It's about vital data, and allowing writes to be interruptible may
leave the VPD in an inconsistent state.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index e73a3a55f..76b20a13a 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -31,7 +31,6 @@ struct pci_vpd {
 	unsigned int	len;
 	u16		flag;
 	u8		cap;
-	unsigned int	busy:1;
 	unsigned int	valid:1;
 };
 
@@ -121,22 +120,14 @@ static int pci_vpd_wait(struct pci_dev *dev)
 	u16 status;
 	int ret;
 
-	if (!vpd->busy)
-		return 0;
-
 	do {
 		ret = pci_user_read_config_word(dev, vpd->cap + PCI_VPD_ADDR,
 						&status);
 		if (ret < 0)
 			return ret;
 
-		if ((status & PCI_VPD_ADDR_F) == vpd->flag) {
-			vpd->busy = 0;
+		if ((status & PCI_VPD_ADDR_F) == vpd->flag)
 			return 0;
-		}
-
-		if (fatal_signal_pending(current))
-			return -EINTR;
 
 		if (time_after(jiffies, timeout))
 			break;
@@ -154,7 +145,7 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 			    void *arg)
 {
 	struct pci_vpd *vpd = dev->vpd;
-	int ret;
+	int ret = 0;
 	loff_t end = pos + count;
 	u8 *buf = arg;
 
@@ -180,19 +171,19 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
 
-	ret = pci_vpd_wait(dev);
-	if (ret < 0)
-		goto out;
-
 	while (pos < end) {
 		u32 val;
 		unsigned int i, skip;
 
+		if (fatal_signal_pending(current)) {
+			ret = -EINTR;
+			break;
+		}
+
 		ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR,
 						 pos & ~3);
 		if (ret < 0)
 			break;
-		vpd->busy = 1;
 		vpd->flag = PCI_VPD_ADDR_F;
 		ret = pci_vpd_wait(dev);
 		if (ret < 0)
@@ -212,7 +203,7 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 			val >>= 8;
 		}
 	}
-out:
+
 	mutex_unlock(&vpd->lock);
 	return ret ? ret : count;
 }
@@ -242,10 +233,6 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 	if (mutex_lock_killable(&vpd->lock))
 		return -EINTR;
 
-	ret = pci_vpd_wait(dev);
-	if (ret < 0)
-		goto out;
-
 	while (pos < end) {
 		u32 val;
 
@@ -262,7 +249,6 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 		if (ret < 0)
 			break;
 
-		vpd->busy = 1;
 		vpd->flag = 0;
 		ret = pci_vpd_wait(dev);
 		if (ret < 0)
@@ -270,7 +256,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 
 		pos += sizeof(u32);
 	}
-out:
+
 	mutex_unlock(&vpd->lock);
 	return ret ? ret : count;
 }
@@ -333,7 +319,6 @@ void pci_vpd_init(struct pci_dev *dev)
 		vpd->ops = &pci_vpd_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
-	vpd->busy = 0;
 	vpd->valid = 0;
 	dev->vpd = vpd;
 }
-- 
2.31.1

[PATCH 1/5] PCI/VPD: Refactor pci_vpd_size

[Heiner Kallweit]

The only Short Resource Data Type tag is the end tag. This allows to
remove the generic SRDT tag handling and the code be significantly
simplified.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 46 ++++++++++++----------------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 26bf7c877..ecdce170f 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -73,50 +73,28 @@ EXPORT_SYMBOL(pci_write_vpd);
 static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 {
 	size_t off = 0;
-	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
+	u8 header[3];	/* 1 byte tag, 2 bytes length */
 
 	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
-		unsigned char tag;
-
 		if (!header[0] && !off) {
 			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
 			return 0;
 		}
 
-		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) {
-					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
-						 tag, off + 1);
-					return 0;
-				}
-				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 (header[0] == PCI_VPD_SRDT_END)
+			return off + PCI_VPD_SRDT_TAG_SIZE;
 
-		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
-		    (tag != PCI_VPD_LTIN_RO_DATA) &&
-		    (tag != PCI_VPD_LTIN_RW_DATA)) {
-			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
-				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
-				 tag, off);
+		if (header[0] != PCI_VPD_LRDT_ID_STRING &&
+		    header[0] != PCI_VPD_LRDT_RO_DATA &&
+		    header[0] != PCI_VPD_LRDT_RW_DATA) {
+			pci_warn(dev, "invalid VPD tag %02x at offset %zu", header[0], off);
 			return 0;
 		}
+
+		if (pci_read_vpd(dev, off + 1, 2, header + 1) != 2)
+			return 0;
+
+		off += PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(header);
 	}
 	return 0;
 }
-- 
2.31.1

[PATCH 5/5] PCI/VPD: Remove pci_vpd member flag

[Heiner Kallweit]

Remove the flag member and simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 76b20a13a..df252f906 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -29,7 +29,6 @@ struct pci_vpd {
 	const struct pci_vpd_ops *ops;
 	struct mutex	lock;
 	unsigned int	len;
-	u16		flag;
 	u8		cap;
 	unsigned int	valid:1;
 };
@@ -109,10 +108,11 @@ static size_t pci_vpd_size(struct pci_dev *dev)
  * This code has to spin since there is no other notification from the PCI
  * hardware. Since the VPD is often implemented by serial attachment to an
  * EEPROM, it may take many milliseconds to complete.
+ * @set: if true wait for flag to be set, else wait for it to be cleared
  *
  * Returns 0 on success, negative values indicate error.
  */
-static int pci_vpd_wait(struct pci_dev *dev)
+static int pci_vpd_wait(struct pci_dev *dev, bool set)
 {
 	struct pci_vpd *vpd = dev->vpd;
 	unsigned long timeout = jiffies + msecs_to_jiffies(125);
@@ -126,7 +126,7 @@ static int pci_vpd_wait(struct pci_dev *dev)
 		if (ret < 0)
 			return ret;
 
-		if ((status & PCI_VPD_ADDR_F) == vpd->flag)
+		if (!!(status & PCI_VPD_ADDR_F) == set)
 			return 0;
 
 		if (time_after(jiffies, timeout))
@@ -184,8 +184,7 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 						 pos & ~3);
 		if (ret < 0)
 			break;
-		vpd->flag = PCI_VPD_ADDR_F;
-		ret = pci_vpd_wait(dev);
+		ret = pci_vpd_wait(dev, true);
 		if (ret < 0)
 			break;
 
@@ -249,8 +248,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 		if (ret < 0)
 			break;
 
-		vpd->flag = 0;
-		ret = pci_vpd_wait(dev);
+		ret = pci_vpd_wait(dev, false);
 		if (ret < 0)
 			break;
 
-- 
2.31.1

Re: [PATCH 0/5] PCI/VPD: Further improvements

[Heiner Kallweit]

On 13.05.2021 22:53, Heiner Kallweit wrote:
> Series with further improvements to PCI VPD handling.
> 
> Heiner Kallweit (5):
>   PCI/VPD: Refactor pci_vpd_size
>   PCI: Clean up VPD constants and functions in pci.h
>   PCI/VPD: Remove old_size argument from pci_vpd_size
>   PCI/VPD: Make pci_vpd_wait uninterruptible
>   PCI/VPD: Remove pci_vpd member flag
> 
>  drivers/pci/vpd.c   | 106 ++++++++++++++++----------------------------
>  include/linux/pci.h |  43 ------------------
>  2 files changed, 37 insertions(+), 112 deletions(-)
> 
This series is still sitting as new in patchwork.
Is it simply waiting for free review capacity or .. ?

Re: [PATCH 0/5] PCI/VPD: Further improvements

[Bjorn Helgaas]

On Tue, Jul 06, 2021 at 07:56:11AM +0200, Heiner Kallweit wrote:
> On 13.05.2021 22:53, Heiner Kallweit wrote:
> > Series with further improvements to PCI VPD handling.
> > 
> > Heiner Kallweit (5):
> >   PCI/VPD: Refactor pci_vpd_size
> >   PCI: Clean up VPD constants and functions in pci.h
> >   PCI/VPD: Remove old_size argument from pci_vpd_size
> >   PCI/VPD: Make pci_vpd_wait uninterruptible
> >   PCI/VPD: Remove pci_vpd member flag
> > 
> >  drivers/pci/vpd.c   | 106 ++++++++++++++++----------------------------
> >  include/linux/pci.h |  43 ------------------
> >  2 files changed, 37 insertions(+), 112 deletions(-)
> > 
> This series is still sitting as new in patchwork.
> Is it simply waiting for free review capacity or .. ?

Yes, sorry, Heiner, I'm hopelessly behind.  Trying to finish up the
v5.14 pull request today, and then next week I'll start on the v5.15
material.

If this series applies cleanly to v5.14-rc1, you don't need to do
anything.  If it needs any rebasing for v5.14-rc1, it would save me
some time if you could post an update.

Bjorn

Re: [PATCH 1/5] PCI/VPD: Refactor pci_vpd_size

[Bjorn Helgaas]

[+cc Hannes]

On Thu, May 13, 2021 at 10:58:40PM +0200, Heiner Kallweit wrote:
> The only Short Resource Data Type tag is the end tag. This allows to
> remove the generic SRDT tag handling and the code be significantly
> simplified.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/vpd.c | 46 ++++++++++++----------------------------------
>  1 file changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 26bf7c877..ecdce170f 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -73,50 +73,28 @@ EXPORT_SYMBOL(pci_write_vpd);
>  static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>  {
>  	size_t off = 0;
> -	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
> +	u8 header[3];	/* 1 byte tag, 2 bytes length */
>  
>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
> -		unsigned char tag;
> -
>  		if (!header[0] && !off) {
>  			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
>  			return 0;
>  		}
>  
> -		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) {
> -					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
> -						 tag, off + 1);
> -					return 0;
> -				}
> -				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 (header[0] == PCI_VPD_SRDT_END)
> +			return off + PCI_VPD_SRDT_TAG_SIZE;

This makes the code beautiful.  But I think pci_vpd_size() is too
picky even now, and this patch makes it more so.

I don't know why pci_vpd_size() currently checks the tags for
ID_STRING, RO_DATA, and RW_DATA.  That seems too aggressive to me.

I think these data items originally came from PNP ISA, and it defines
several other tags.  Of course, that wasn't for PCI devices, but a
Google search for '"invalid" "vpd tag" "at offset"' does find several
cases where VPD contains things that look like PNP ISA data items.

I think we should compute the VPD size by iterating through it looking
only at the type (small or large) and the data item length until we
find the End Tag.

This code originally came from 104daa71b396 ("PCI: Determine actual
VPD size on first access"), so I added Hannes in case there was some
reason we do the extra validation.

> -		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
> -		    (tag != PCI_VPD_LTIN_RO_DATA) &&
> -		    (tag != PCI_VPD_LTIN_RW_DATA)) {
> -			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
> -				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
> -				 tag, off);
> +		if (header[0] != PCI_VPD_LRDT_ID_STRING &&
> +		    header[0] != PCI_VPD_LRDT_RO_DATA &&
> +		    header[0] != PCI_VPD_LRDT_RW_DATA) {
> +			pci_warn(dev, "invalid VPD tag %02x at offset %zu", header[0], off);
>  			return 0;
>  		}
> +
> +		if (pci_read_vpd(dev, off + 1, 2, header + 1) != 2)
> +			return 0;
> +
> +		off += PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(header);
>  	}
>  	return 0;
>  }
> -- 
> 2.31.1
> 
> 

Re: [PATCH 1/5] PCI/VPD: Refactor pci_vpd_size

[Heiner Kallweit]

On 13.07.2021 22:25, Bjorn Helgaas wrote:
> [+cc Hannes]
> 
> On Thu, May 13, 2021 at 10:58:40PM +0200, Heiner Kallweit wrote:
>> The only Short Resource Data Type tag is the end tag. This allows to
>> remove the generic SRDT tag handling and the code be significantly
>> simplified.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/pci/vpd.c | 46 ++++++++++++----------------------------------
>>  1 file changed, 12 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
>> index 26bf7c877..ecdce170f 100644
>> --- a/drivers/pci/vpd.c
>> +++ b/drivers/pci/vpd.c
>> @@ -73,50 +73,28 @@ EXPORT_SYMBOL(pci_write_vpd);
>>  static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>>  {
>>  	size_t off = 0;
>> -	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
>> +	u8 header[3];	/* 1 byte tag, 2 bytes length */
>>  
>>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
>> -		unsigned char tag;
>> -
>>  		if (!header[0] && !off) {
>>  			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
>>  			return 0;
>>  		}
>>  
>> -		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) {
>> -					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
>> -						 tag, off + 1);
>> -					return 0;
>> -				}
>> -				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 (header[0] == PCI_VPD_SRDT_END)
>> +			return off + PCI_VPD_SRDT_TAG_SIZE;
> 
> This makes the code beautiful.  But I think pci_vpd_size() is too
> picky even now, and this patch makes it more so.
> 
> I don't know why pci_vpd_size() currently checks the tags for
> ID_STRING, RO_DATA, and RW_DATA.  That seems too aggressive to me.
> 

It checks for these tags (+ the end tag) because it's the only ones
defined for VPD by the PCI spec.

> I think these data items originally came from PNP ISA, and it defines
> several other tags.  Of course, that wasn't for PCI devices, but a
> Google search for '"invalid" "vpd tag" "at offset"' does find several
> cases where VPD contains things that look like PNP ISA data items.
> 

Right, the tag format is based on PNP ISA. But PCI spec explicitly
lists the supported tags.
I tried to do the same search and found:
- "invalid short vpd tag 00" and "invalid large tag 7f"
   Both are symptom of a missing optional VPD EEPROM.
- "ixgbe 0000:0b:00.0: invalid short VPD tag 06 at offset 4" and a
  similar message for igb
  I didn't see any response explaining what causes this issue.
  My personal guess: Some OEM provided invalid VPD EEPROM content.
  Offset 4 is the first character of the ID string. The message
  indicates that the ID tag declares an empty ID. That would be weird.

> I think we should compute the VPD size by iterating through it looking
> only at the type (small or large) and the data item length until we
> find the End Tag.
> 

Still I didn't see any example of a rejected valid VPD image.
Not checking for supported tags increases he risk that we interpret
a random byte as tag and read beyond the VPD end, what is known to
cause a freeze on some devices.

> This code originally came from 104daa71b396 ("PCI: Determine actual
> VPD size on first access"), so I added Hannes in case there was some
> reason we do the extra validation.
> 
>> -		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
>> -		    (tag != PCI_VPD_LTIN_RO_DATA) &&
>> -		    (tag != PCI_VPD_LTIN_RW_DATA)) {
>> -			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
>> -				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
>> -				 tag, off);
>> +		if (header[0] != PCI_VPD_LRDT_ID_STRING &&
>> +		    header[0] != PCI_VPD_LRDT_RO_DATA &&
>> +		    header[0] != PCI_VPD_LRDT_RW_DATA) {
>> +			pci_warn(dev, "invalid VPD tag %02x at offset %zu", header[0], off);
>>  			return 0;
>>  		}
>> +
>> +		if (pci_read_vpd(dev, off + 1, 2, header + 1) != 2)
>> +			return 0;
>> +
>> +		off += PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(header);
>>  	}
>>  	return 0;
>>  }
>> -- 
>> 2.31.1
>>
>>

Re: [PATCH 1/5] PCI/VPD: Refactor pci_vpd_size

[Bjorn Helgaas]

On Tue, Jul 13, 2021 at 11:13:31PM +0200, Heiner Kallweit wrote:
> On 13.07.2021 22:25, Bjorn Helgaas wrote:
> > [+cc Hannes]
> > 
> > On Thu, May 13, 2021 at 10:58:40PM +0200, Heiner Kallweit wrote:
> >> The only Short Resource Data Type tag is the end tag. This allows to
> >> remove the generic SRDT tag handling and the code be significantly
> >> simplified.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/pci/vpd.c | 46 ++++++++++++----------------------------------
> >>  1 file changed, 12 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> >> index 26bf7c877..ecdce170f 100644
> >> --- a/drivers/pci/vpd.c
> >> +++ b/drivers/pci/vpd.c
> >> @@ -73,50 +73,28 @@ EXPORT_SYMBOL(pci_write_vpd);
> >>  static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >>  {
> >>  	size_t off = 0;
> >> -	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
> >> +	u8 header[3];	/* 1 byte tag, 2 bytes length */
> >>  
> >>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
> >> -		unsigned char tag;
> >> -
> >>  		if (!header[0] && !off) {
> >>  			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
> >>  			return 0;
> >>  		}
> >>  
> >> -		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) {
> >> -					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
> >> -						 tag, off + 1);
> >> -					return 0;
> >> -				}
> >> -				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 (header[0] == PCI_VPD_SRDT_END)
> >> +			return off + PCI_VPD_SRDT_TAG_SIZE;
> > 
> > This makes the code beautiful.  But I think pci_vpd_size() is too
> > picky even now, and this patch makes it more so.
> > 
> > I don't know why pci_vpd_size() currently checks the tags for
> > ID_STRING, RO_DATA, and RW_DATA.  That seems too aggressive to me.
> 
> It checks for these tags (+ the end tag) because it's the only ones
> defined for VPD by the PCI spec.
> 
> > I think these data items originally came from PNP ISA, and it defines
> > several other tags.  Of course, that wasn't for PCI devices, but a
> > Google search for '"invalid" "vpd tag" "at offset"' does find several
> > cases where VPD contains things that look like PNP ISA data items.
> 
> Right, the tag format is based on PNP ISA. But PCI spec explicitly
> lists the supported tags.

Yes.  I guess my point is that I think we should make the minimum
assumptions required by the spec, and we shouldn't make things fail
because of minor spec violations that don't get in the way of what
we're trying to accomplish.  Sort of the "be liberal in what you
accept" idea.

In this case, we're trying to determine the VPD size.  We don't need
to know what data types are in the VPD; we only need to know the size
of each data item, which is determined by the small/large bit and the
data item length.  The "name" (ID_STRING, RO_DATA, RW_DATA) isn't
involved at all except that we need to recognize the END tag.

The value of rejecting unrecognized data types is a bit fuzzy.  Yes,
it may keep us from reading beyond the VPD end.  But it's no
guarantee; we can still read beyond the end if the VPD is
ill-structured (missing END tag, too-large item length, etc).

The kernel itself doesn't depend on any data from VPD, so I don't
think it should be in the business of validating that VPD only uses
the approved data item types.

> I tried to do the same search and found:
> - "invalid short vpd tag 00" and "invalid large tag 7f"
>    Both are symptom of a missing optional VPD EEPROM.

If we read a tag that is inconsistent, we *have* to stop because we
can no longer compute the size.  Neither of the trivial cases of a
missing VPD EEPROM looking like all zeroes or all 0xff can be
interpreted as valid tags, so it's easy to discard those cases.

Incidentally, I think we may be able to improve our diagnostic
messages a bit.  The "Invalid VPD tag 00, assume missing optional VPD
EPROM" message is good, but the 0xff case isn't quite as clear.

> - "ixgbe 0000:0b:00.0: invalid short VPD tag 06 at offset 4" and a
>   similar message for igb
>   I didn't see any response explaining what causes this issue.
>   My personal guess: Some OEM provided invalid VPD EEPROM content.
>   Offset 4 is the first character of the ID string. The message
>   indicates that the ID tag declares an empty ID. That would be weird.
> 
> > I think we should compute the VPD size by iterating through it looking
> > only at the type (small or large) and the data item length until we
> > find the End Tag.
> 
> Still I didn't see any example of a rejected valid VPD image.
> Not checking for supported tags increases the risk that we interpret
> a random byte as tag and read beyond the VPD end, what is known to
> cause a freeze on some devices.
> 
> > This code originally came from 104daa71b396 ("PCI: Determine actual
> > VPD size on first access"), so I added Hannes in case there was some
> > reason we do the extra validation.
> > 
> >> -		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
> >> -		    (tag != PCI_VPD_LTIN_RO_DATA) &&
> >> -		    (tag != PCI_VPD_LTIN_RW_DATA)) {
> >> -			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
> >> -				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
> >> -				 tag, off);
> >> +		if (header[0] != PCI_VPD_LRDT_ID_STRING &&
> >> +		    header[0] != PCI_VPD_LRDT_RO_DATA &&
> >> +		    header[0] != PCI_VPD_LRDT_RW_DATA) {
> >> +			pci_warn(dev, "invalid VPD tag %02x at offset %zu", header[0], off);
> >>  			return 0;
> >>  		}
> >> +
> >> +		if (pci_read_vpd(dev, off + 1, 2, header + 1) != 2)
> >> +			return 0;
> >> +
> >> +		off += PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(header);
> >>  	}
> >>  	return 0;
> >>  }
> >> -- 
> >> 2.31.1
> >>
> >>
> 

Re: [PATCH 2/5] PCI: Clean up VPD constants and functions in pci.h

[Bjorn Helgaas]

On Thu, May 13, 2021 at 10:55:26PM +0200, Heiner Kallweit wrote:
> Move some constants that are used by vpd.c only from include/linux/pci.h
> to drivers/pci/vpd.c. In addition remove some unused VPD inline functions.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/vpd.c   |  7 +++++++
>  include/linux/pci.h | 43 -------------------------------------------
>  2 files changed, 7 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index ecdce170f..ff537371c 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -13,6 +13,13 @@
>  
>  /* VPD access through PCI 2.2+ VPD capability */
>  
> +/* Small Resource Data Type */
> +#define PCI_VPD_SRDT_TAG_SIZE	1
> +#define PCI_VPD_SRDT_END	(0x0f << 3) /* end tag */
> +
> +/* Large Resource Data Type */
> +#define PCI_VPD_LRDT_TIN_MASK	0x7f
> +
>  struct pci_vpd_ops {
>  	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
>  	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e59..c21558821 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2240,17 +2240,7 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask);
>  #define PCI_VPD_LRDT_RO_DATA		PCI_VPD_LRDT_ID(PCI_VPD_LTIN_RO_DATA)
>  #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		0x0f	/* 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
>  
>  #define PCI_VPD_INFO_FLD_HDR_SIZE	3
>  
> @@ -2271,39 +2261,6 @@ static inline u16 pci_vpd_lrdt_size(const u8 *lrdt)
>  	return (u16)lrdt[1] + ((u16)lrdt[2] << 8);
>  }
>  
> -/**
> - * 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
> - * @srdt: Pointer to the beginning of the Small Resource Data Type tag
> - *
> - * Returns the extracted Small Resource Data Type length.
> - */
> -static inline u8 pci_vpd_srdt_size(const u8 *srdt)
> -{
> -	return (*srdt) & PCI_VPD_SRDT_LEN_MASK;
> -}
> -
> -/**
> - * pci_vpd_srdt_tag - Extracts the Small Resource Data Type Tag Item
> - * @srdt: 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;
> -}

I think the last uses of these were removed by the previous patch.
Can you remove these definitions in the same patch that removed the
last use?  Then we don't have the unused things dangling between
patches.

>  /**
>   * pci_vpd_info_field_size - Extracts the information field length
>   * @info_field: Pointer to the beginning of an information field header
> -- 
> 2.31.1
> 
> 

Re: [PATCH 1/5] PCI/VPD: Refactor pci_vpd_size

[Heiner Kallweit]

On 14.07.2021 17:50, Bjorn Helgaas wrote:
> On Tue, Jul 13, 2021 at 11:13:31PM +0200, Heiner Kallweit wrote:
>> On 13.07.2021 22:25, Bjorn Helgaas wrote:
>>> [+cc Hannes]
>>>
>>> On Thu, May 13, 2021 at 10:58:40PM +0200, Heiner Kallweit wrote:
>>>> The only Short Resource Data Type tag is the end tag. This allows to
>>>> remove the generic SRDT tag handling and the code be significantly
>>>> simplified.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/pci/vpd.c | 46 ++++++++++++----------------------------------
>>>>  1 file changed, 12 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
>>>> index 26bf7c877..ecdce170f 100644
>>>> --- a/drivers/pci/vpd.c
>>>> +++ b/drivers/pci/vpd.c
>>>> @@ -73,50 +73,28 @@ EXPORT_SYMBOL(pci_write_vpd);
>>>>  static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>>>>  {
>>>>  	size_t off = 0;
>>>> -	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
>>>> +	u8 header[3];	/* 1 byte tag, 2 bytes length */
>>>>  
>>>>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
>>>> -		unsigned char tag;
>>>> -
>>>>  		if (!header[0] && !off) {
>>>>  			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
>>>>  			return 0;
>>>>  		}
>>>>  
>>>> -		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) {
>>>> -					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
>>>> -						 tag, off + 1);
>>>> -					return 0;
>>>> -				}
>>>> -				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 (header[0] == PCI_VPD_SRDT_END)
>>>> +			return off + PCI_VPD_SRDT_TAG_SIZE;
>>>
>>> This makes the code beautiful.  But I think pci_vpd_size() is too
>>> picky even now, and this patch makes it more so.
>>>
>>> I don't know why pci_vpd_size() currently checks the tags for
>>> ID_STRING, RO_DATA, and RW_DATA.  That seems too aggressive to me.
>>
>> It checks for these tags (+ the end tag) because it's the only ones
>> defined for VPD by the PCI spec.
>>
>>> I think these data items originally came from PNP ISA, and it defines
>>> several other tags.  Of course, that wasn't for PCI devices, but a
>>> Google search for '"invalid" "vpd tag" "at offset"' does find several
>>> cases where VPD contains things that look like PNP ISA data items.
>>
>> Right, the tag format is based on PNP ISA. But PCI spec explicitly
>> lists the supported tags.
> 
> Yes.  I guess my point is that I think we should make the minimum
> assumptions required by the spec, and we shouldn't make things fail
> because of minor spec violations that don't get in the way of what
> we're trying to accomplish.  Sort of the "be liberal in what you
> accept" idea.
> 
> In this case, we're trying to determine the VPD size.  We don't need
> to know what data types are in the VPD; we only need to know the size
> of each data item, which is determined by the small/large bit and the
> data item length.  The "name" (ID_STRING, RO_DATA, RW_DATA) isn't
> involved at all except that we need to recognize the END tag.
> 
> The value of rejecting unrecognized data types is a bit fuzzy.  Yes,
> it may keep us from reading beyond the VPD end.  But it's no
> guarantee; we can still read beyond the end if the VPD is
> ill-structured (missing END tag, too-large item length, etc).
> 
> The kernel itself doesn't depend on any data from VPD, so I don't
> think it should be in the business of validating that VPD only uses
> the approved data item types.
> 
>> I tried to do the same search and found:
>> - "invalid short vpd tag 00" and "invalid large tag 7f"
>>    Both are symptom of a missing optional VPD EEPROM.
> 
> If we read a tag that is inconsistent, we *have* to stop because we
> can no longer compute the size.  Neither of the trivial cases of a
> missing VPD EEPROM looking like all zeroes or all 0xff can be
> interpreted as valid tags, so it's easy to discard those cases.
> 
> Incidentally, I think we may be able to improve our diagnostic
> messages a bit.  The "Invalid VPD tag 00, assume missing optional VPD
> EPROM" message is good, but the 0xff case isn't quite as clear.
> 
Right, we should handle 0xff together with 0x00. I'll submit a patch
for it.

>> - "ixgbe 0000:0b:00.0: invalid short VPD tag 06 at offset 4" and a
>>   similar message for igb
>>   I didn't see any response explaining what causes this issue.
>>   My personal guess: Some OEM provided invalid VPD EEPROM content.
>>   Offset 4 is the first character of the ID string. The message
>>   indicates that the ID tag declares an empty ID. That would be weird.
>>
>>> I think we should compute the VPD size by iterating through it looking
>>> only at the type (small or large) and the data item length until we
>>> find the End Tag.
>>
>> Still I didn't see any example of a rejected valid VPD image.
>> Not checking for supported tags increases the risk that we interpret
>> a random byte as tag and read beyond the VPD end, what is known to
>> cause a freeze on some devices.
>>
>>> This code originally came from 104daa71b396 ("PCI: Determine actual
>>> VPD size on first access"), so I added Hannes in case there was some
>>> reason we do the extra validation.
>>>
>>>> -		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
>>>> -		    (tag != PCI_VPD_LTIN_RO_DATA) &&
>>>> -		    (tag != PCI_VPD_LTIN_RW_DATA)) {
>>>> -			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
>>>> -				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
>>>> -				 tag, off);
>>>> +		if (header[0] != PCI_VPD_LRDT_ID_STRING &&
>>>> +		    header[0] != PCI_VPD_LRDT_RO_DATA &&
>>>> +		    header[0] != PCI_VPD_LRDT_RW_DATA) {
>>>> +			pci_warn(dev, "invalid VPD tag %02x at offset %zu", header[0], off);
>>>>  			return 0;
>>>>  		}
>>>> +
>>>> +		if (pci_read_vpd(dev, off + 1, 2, header + 1) != 2)
>>>> +			return 0;
>>>> +
>>>> +		off += PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(header);
>>>>  	}
>>>>  	return 0;
>>>>  }
>>>> -- 
>>>> 2.31.1
>>>>
>>>>
>>

[PATCH 0/5] PCI/VPD: pci_vpd_size() cleanups

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is a follow-up to the conversation here:
https://lore.kernel.org/r/1cdda5f1-e1ea-af9f-cfbe-952b7d37e246@gmail.com
about how and whether we should validate the contents of VPD.

My proposal is to basically do minimal validation since the kernel really
doesn't care about the actual *contents* of VPD and mainly provides an
access mechanism (although there a few drivers that get part numbers and
maybe even clocking information from VPD).

I welcome any feedback!

Bjorn Helgaas (5):
  PCI/VPD: Correct diagnostic for VPD read failure
  PCI/VPD: Check Resource tags against those valid for type
  PCI/VPD: Consolidate missing EEPROM checks
  PCI/VPD: Don't check Large Resource types for validity
  PCI/VPD: Allow access to valid parts of VPD if some is invalid

 drivers/pci/vpd.c | 63 +++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH 1/5] PCI/VPD: Correct diagnostic for VPD read failure

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously, when a VPD read failed, we warned about an "invalid large
VPD tag".  Warn about the VPD read failure instead.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/vpd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 26bf7c877de5..7bfb8fc4251b 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -92,8 +92,8 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 			    (tag == PCI_VPD_LTIN_RW_DATA)) {
 				if (pci_read_vpd(dev, off+1, 2,
 						 &header[1]) != 2) {
-					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
-						 tag, off + 1);
+					pci_warn(dev, "failed VPD read at offset %zu",
+						 off + 1);
 					return 0;
 				}
 				off += PCI_VPD_LRDT_TAG_SIZE +
-- 
2.25.1

[PATCH 2/5] PCI/VPD: Check Resource tags against those valid for type

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously, we checked for PCI_VPD_STIN_END, PCI_VPD_LTIN_ID_STRING, etc.,
outside the Large and Small Resource cases, so we checked Large Resource
tags against a Small Resource name and vice versa.

Move these tests into the Large and Small Resource cases, so we only check
PCI_VPD_STIN_END for Small Resources and PCI_VPD_LTIN_* for Large
Resources.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/vpd.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 7bfb8fc4251b..9b54dd95e42c 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -98,24 +98,18 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 				}
 				off += PCI_VPD_LRDT_TAG_SIZE +
 					pci_vpd_lrdt_size(header);
+			} else {
+				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
+					 tag, off);
+				return 0;
 			}
 		} 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)) {
-			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
-				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
-				 tag, off);
-			return 0;
+			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
+				return off;
 		}
 	}
 	return 0;
-- 
2.25.1

[PATCH 3/5] PCI/VPD: Consolidate missing EEPROM checks

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

A missing VPD EEPROM typically reads as either all 0xff or all zeroes.
Both cases lead to invalid VPD resource items.  A 0xff tag would be a Large
Resource with length 0xffff (65535).  That's invalid because VPD can only
be 32768 bytes, limited by the size of the address register in the VPD
Capability.

A VPD that reads as all zeroes is also invalid because a 0x00 tag is a
Small Resource with length 0, which would result in an item of length 1.
This isn't explicitly illegal in PCIe r5.0, sec 6.28, but the format is
derived from PNP ISA, which *does* say "a small resource data type may be
2-8 bytes in size" (Plug and Play ISA v1.0a, sec 6.2.2.

Check for these invalid tags and return VPD size of zero if we find them.
If they occur at the beginning of VPD, assume it's the result of a missing
EEPROM.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/vpd.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 9b54dd95e42c..9c2744d79b53 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -77,11 +77,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 
 	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
 		unsigned char tag;
-
-		if (!header[0] && !off) {
-			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
-			return 0;
-		}
+		size_t size;
 
 		if (header[0] & PCI_VPD_LRDT) {
 			/* Large Resource Data Type Tag */
@@ -96,8 +92,16 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 						 off + 1);
 					return 0;
 				}
-				off += PCI_VPD_LRDT_TAG_SIZE +
-					pci_vpd_lrdt_size(header);
+				size = pci_vpd_lrdt_size(header);
+
+				/*
+				 * Missing EEPROM may read as 0xff.
+				 * Length of 0xffff (65535) cannot be valid
+				 * because VPD can't be that large.
+				 */
+				if (size > PCI_VPD_MAX_SIZE)
+					goto error;
+				off += PCI_VPD_LRDT_TAG_SIZE + size;
 			} else {
 				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
 					 tag, off);
@@ -105,14 +109,28 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 			}
 		} else {
 			/* Short Resource Data Type Tag */
-			off += PCI_VPD_SRDT_TAG_SIZE +
-				pci_vpd_srdt_size(header);
 			tag = pci_vpd_srdt_tag(header);
+			size = pci_vpd_srdt_size(header);
+
+			/*
+			 * Missing EEPROM may read as 0x00.  A small item
+			 * must be at least 2 bytes.
+			 */
+			if (size == 0)
+				goto error;
+
+			off += PCI_VPD_SRDT_TAG_SIZE + size;
 			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
 				return off;
 		}
 	}
 	return 0;
+
+error:
+	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
+		 header[0], off, off == 0 ?
+		 "; assume missing optional EEPROM" : "");
+	return 0;
 }
 
 /*
-- 
2.25.1

[PATCH 4/5] PCI/VPD: Don't check Large Resource types for validity

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

VPD consists of a series of Small and Large Resources.  Computing the size
of VPD requires only the length of each, which is specified in the generic
tag of each resource.  We only expect to see ID_STRING, RO_DATA, and
RW_DATA in VPD, but it's not a problem if it contains other resource types.

Drop the validity checking of Large Resource items.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/vpd.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 9c2744d79b53..d7a4a9f05bd6 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -82,31 +82,22 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 		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) {
-					pci_warn(dev, "failed VPD read at offset %zu",
-						 off + 1);
-					return 0;
-				}
-				size = pci_vpd_lrdt_size(header);
-
-				/*
-				 * Missing EEPROM may read as 0xff.
-				 * Length of 0xffff (65535) cannot be valid
-				 * because VPD can't be that large.
-				 */
-				if (size > PCI_VPD_MAX_SIZE)
-					goto error;
-				off += PCI_VPD_LRDT_TAG_SIZE + size;
-			} else {
-				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
-					 tag, off);
+			if (pci_read_vpd(dev, off + 1, 2, &header[1]) != 2) {
+				pci_warn(dev, "failed VPD read at offset %zu",
+					 off + 1);
 				return 0;
 			}
+			size = pci_vpd_lrdt_size(header);
+
+			/*
+			 * Missing EEPROM may read as 0xff.  Length of
+			 * 0xffff (65535) cannot be valid because VPD can't
+			 * be that large.
+			 */
+			if (size > PCI_VPD_MAX_SIZE)
+				goto error;
+
+			off += PCI_VPD_LRDT_TAG_SIZE + size;
 		} else {
 			/* Short Resource Data Type Tag */
 			tag = pci_vpd_srdt_tag(header);
-- 
2.25.1

[PATCH 5/5] PCI/VPD: Allow access to valid parts of VPD if some is invalid

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously, if we found any error in the VPD, we returned size 0, which
prevents access to all of VPD.  But there may be valid resources in VPD
before the error, and there's no reason to prevent access to those.

"off" covers only VPD resources known to have valid header tags.  In case
of error, return "off" (which may be zero if we haven't found any valid
header tags at all).

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/vpd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index d7a4a9f05bd6..92acbbcc8059 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -85,7 +85,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 			if (pci_read_vpd(dev, off + 1, 2, &header[1]) != 2) {
 				pci_warn(dev, "failed VPD read at offset %zu",
 					 off + 1);
-				return 0;
+				return off;
 			}
 			size = pci_vpd_lrdt_size(header);
 
@@ -115,13 +115,13 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 				return off;
 		}
 	}
-	return 0;
+	return off;
 
 error:
 	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
 		 header[0], off, off == 0 ?
 		 "; assume missing optional EEPROM" : "");
-	return 0;
+	return off;
 }
 
 /*
-- 
2.25.1

Re: [PATCH 1/5] PCI/VPD: Correct diagnostic for VPD read failure

[Heiner Kallweit]

On 15.07.2021 23:59, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Previously, when a VPD read failed, we warned about an "invalid large
> VPD tag".  Warn about the VPD read failure instead.
> 
LGTM. Seems no VPD has been broken enough yet to trigger this warning
(at that place), so there's not much need to argue.

> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/vpd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 26bf7c877de5..7bfb8fc4251b 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -92,8 +92,8 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>  			    (tag == PCI_VPD_LTIN_RW_DATA)) {
>  				if (pci_read_vpd(dev, off+1, 2,
>  						 &header[1]) != 2) {
> -					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
> -						 tag, off + 1);
> +					pci_warn(dev, "failed VPD read at offset %zu",
> +						 off + 1);
>  					return 0;
>  				}
>  				off += PCI_VPD_LRDT_TAG_SIZE +
> 

Re: [PATCH 3/5] PCI/VPD: Consolidate missing EEPROM checks

[Heiner Kallweit]

On 15.07.2021 23:59, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> A missing VPD EEPROM typically reads as either all 0xff or all zeroes.
> Both cases lead to invalid VPD resource items.  A 0xff tag would be a Large
> Resource with length 0xffff (65535).  That's invalid because VPD can only
> be 32768 bytes, limited by the size of the address register in the VPD
> Capability.
> 
> A VPD that reads as all zeroes is also invalid because a 0x00 tag is a
> Small Resource with length 0, which would result in an item of length 1.
> This isn't explicitly illegal in PCIe r5.0, sec 6.28, but the format is
> derived from PNP ISA, which *does* say "a small resource data type may be
> 2-8 bytes in size" (Plug and Play ISA v1.0a, sec 6.2.2.
> 
> Check for these invalid tags and return VPD size of zero if we find them.
> If they occur at the beginning of VPD, assume it's the result of a missing
> EEPROM.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/vpd.c | 36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 9b54dd95e42c..9c2744d79b53 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -77,11 +77,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>  
>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
>  		unsigned char tag;
> -
> -		if (!header[0] && !off) {
> -			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
> -			return 0;
> -		}
> +		size_t size;
>  
>  		if (header[0] & PCI_VPD_LRDT) {
>  			/* Large Resource Data Type Tag */
> @@ -96,8 +92,16 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>  						 off + 1);
>  					return 0;
>  				}
> -				off += PCI_VPD_LRDT_TAG_SIZE +
> -					pci_vpd_lrdt_size(header);
> +				size = pci_vpd_lrdt_size(header);
> +
> +				/*
> +				 * Missing EEPROM may read as 0xff.
> +				 * Length of 0xffff (65535) cannot be valid
> +				 * because VPD can't be that large.
> +				 */

I'm not fully convinced. Instead of checking for a "no VPD EPROM" read (00/ff)
directly, we now do it indirectly based on the internal tag structure.
We have pci_vpd_lrdt_size() et al to encapsulate the internal structure.
IMO the code is harder to understand now.

> +				if (size > PCI_VPD_MAX_SIZE)
> +					goto error;
> +				off += PCI_VPD_LRDT_TAG_SIZE + size;
>  			} else {
>  				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
>  					 tag, off);
> @@ -105,14 +109,28 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>  			}
>  		} else {
>  			/* Short Resource Data Type Tag */
> -			off += PCI_VPD_SRDT_TAG_SIZE +
> -				pci_vpd_srdt_size(header);
>  			tag = pci_vpd_srdt_tag(header);
> +			size = pci_vpd_srdt_size(header);
> +
> +			/*
> +			 * Missing EEPROM may read as 0x00.  A small item
> +			 * must be at least 2 bytes.
> +			 */
> +			if (size == 0)
> +				goto error;
> +
> +			off += PCI_VPD_SRDT_TAG_SIZE + size;
>  			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
>  				return off;
>  		}
>  	}
>  	return 0;
> +
> +error:
> +	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
> +		 header[0], off, off == 0 ?
> +		 "; assume missing optional EEPROM" : "");
> +	return 0;
>  }
>  
>  /*
> 

Re: [PATCH 1/5] PCI/VPD: Correct diagnostic for VPD read failure

[Hannes Reinecke]

On 7/15/21 11:59 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Previously, when a VPD read failed, we warned about an "invalid large
> VPD tag".  Warn about the VPD read failure instead.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/vpd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 26bf7c877de5..7bfb8fc4251b 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -92,8 +92,8 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>   			    (tag == PCI_VPD_LTIN_RW_DATA)) {
>   				if (pci_read_vpd(dev, off+1, 2,
>   						 &header[1]) != 2) {
> -					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
> -						 tag, off + 1);
> +					pci_warn(dev, "failed VPD read at offset %zu",
> +						 off + 1);
>   					return 0;
>   				}
>   				off += PCI_VPD_LRDT_TAG_SIZE +
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/5] PCI/VPD: Check Resource tags against those valid for type

[Hannes Reinecke]

On 7/15/21 11:59 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Previously, we checked for PCI_VPD_STIN_END, PCI_VPD_LTIN_ID_STRING, etc.,
> outside the Large and Small Resource cases, so we checked Large Resource
> tags against a Small Resource name and vice versa.
> 
> Move these tests into the Large and Small Resource cases, so we only check
> PCI_VPD_STIN_END for Small Resources and PCI_VPD_LTIN_* for Large
> Resources.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/vpd.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 3/5] PCI/VPD: Consolidate missing EEPROM checks

[Hannes Reinecke]

On 7/15/21 11:59 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> A missing VPD EEPROM typically reads as either all 0xff or all zeroes.
> Both cases lead to invalid VPD resource items.  A 0xff tag would be a Large
> Resource with length 0xffff (65535).  That's invalid because VPD can only
> be 32768 bytes, limited by the size of the address register in the VPD
> Capability.
> 
> A VPD that reads as all zeroes is also invalid because a 0x00 tag is a
> Small Resource with length 0, which would result in an item of length 1.
> This isn't explicitly illegal in PCIe r5.0, sec 6.28, but the format is
> derived from PNP ISA, which *does* say "a small resource data type may be
> 2-8 bytes in size" (Plug and Play ISA v1.0a, sec 6.2.2.
> 
> Check for these invalid tags and return VPD size of zero if we find them.
> If they occur at the beginning of VPD, assume it's the result of a missing
> EEPROM.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/vpd.c | 36 +++++++++++++++++++++++++++---------
>   1 file changed, 27 insertions(+), 9 deletions(-)
> 
Very good idea.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 4/5] PCI/VPD: Don't check Large Resource types for validity

[Hannes Reinecke]

On 7/15/21 11:59 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> VPD consists of a series of Small and Large Resources.  Computing the size
> of VPD requires only the length of each, which is specified in the generic
> tag of each resource.  We only expect to see ID_STRING, RO_DATA, and
> RW_DATA in VPD, but it's not a problem if it contains other resource types.
> 
> Drop the validity checking of Large Resource items.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/vpd.c | 37 ++++++++++++++-----------------------
>   1 file changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 9c2744d79b53..d7a4a9f05bd6 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -82,31 +82,22 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>   		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) {
> -					pci_warn(dev, "failed VPD read at offset %zu",
> -						 off + 1);
> -					return 0;
> -				}
> -				size = pci_vpd_lrdt_size(header);
> -
> -				/*
> -				 * Missing EEPROM may read as 0xff.
> -				 * Length of 0xffff (65535) cannot be valid
> -				 * because VPD can't be that large.
> -				 */
> -				if (size > PCI_VPD_MAX_SIZE)
> -					goto error;
> -				off += PCI_VPD_LRDT_TAG_SIZE + size;
> -			} else {
> -				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
> -					 tag, off);
> +			if (pci_read_vpd(dev, off + 1, 2, &header[1]) != 2) {
> +				pci_warn(dev, "failed VPD read at offset %zu",
> +					 off + 1);
>   				return 0;
>   			}
> +			size = pci_vpd_lrdt_size(header);
> +
> +			/*
> +			 * Missing EEPROM may read as 0xff.  Length of
> +			 * 0xffff (65535) cannot be valid because VPD can't
> +			 * be that large.
> +			 */
> +			if (size > PCI_VPD_MAX_SIZE)
> +				goto error;
> +
> +			off += PCI_VPD_LRDT_TAG_SIZE + size;
>   		} else {
>   			/* Short Resource Data Type Tag */
>   			tag = pci_vpd_srdt_tag(header);
> 
I'm not entirely happy with this; we really have to rely on well-formed 
VPD tags for the protocol to work correctly, and that's why I took the 
cautious approach. But with the check for missing EEPROM I hope we've 
covered the most common causes for invalid tags. Let's see how it goes.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 5/5] PCI/VPD: Allow access to valid parts of VPD if some is invalid

[Hannes Reinecke]

On 7/15/21 11:59 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Previously, if we found any error in the VPD, we returned size 0, which
> prevents access to all of VPD.  But there may be valid resources in VPD
> before the error, and there's no reason to prevent access to those.
> 
> "off" covers only VPD resources known to have valid header tags.  In case
> of error, return "off" (which may be zero if we haven't found any valid
> header tags at all).
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/vpd.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
D'oh. Of course.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 3/5] PCI/VPD: Consolidate missing EEPROM checks

[Bjorn Helgaas]

On Fri, Jul 16, 2021 at 12:16:55AM +0200, Heiner Kallweit wrote:
> On 15.07.2021 23:59, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > A missing VPD EEPROM typically reads as either all 0xff or all zeroes.
> > Both cases lead to invalid VPD resource items.  A 0xff tag would be a Large
> > Resource with length 0xffff (65535).  That's invalid because VPD can only
> > be 32768 bytes, limited by the size of the address register in the VPD
> > Capability.
> > 
> > A VPD that reads as all zeroes is also invalid because a 0x00 tag is a
> > Small Resource with length 0, which would result in an item of length 1.
> > This isn't explicitly illegal in PCIe r5.0, sec 6.28, but the format is
> > derived from PNP ISA, which *does* say "a small resource data type may be
> > 2-8 bytes in size" (Plug and Play ISA v1.0a, sec 6.2.2.
> > 
> > Check for these invalid tags and return VPD size of zero if we find them.
> > If they occur at the beginning of VPD, assume it's the result of a missing
> > EEPROM.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/vpd.c | 36 +++++++++++++++++++++++++++---------
> >  1 file changed, 27 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > index 9b54dd95e42c..9c2744d79b53 100644
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -77,11 +77,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >  
> >  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
> >  		unsigned char tag;
> > -
> > -		if (!header[0] && !off) {
> > -			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
> > -			return 0;
> > -		}
> > +		size_t size;
> >  
> >  		if (header[0] & PCI_VPD_LRDT) {
> >  			/* Large Resource Data Type Tag */
> > @@ -96,8 +92,16 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >  						 off + 1);
> >  					return 0;
> >  				}
> > -				off += PCI_VPD_LRDT_TAG_SIZE +
> > -					pci_vpd_lrdt_size(header);
> > +				size = pci_vpd_lrdt_size(header);
> > +
> > +				/*
> > +				 * Missing EEPROM may read as 0xff.
> > +				 * Length of 0xffff (65535) cannot be valid
> > +				 * because VPD can't be that large.
> > +				 */
> 
> I'm not fully convinced. Instead of checking for a "no VPD EPROM" read (00/ff)
> directly, we now do it indirectly based on the internal tag structure.
> We have pci_vpd_lrdt_size() et al to encapsulate the internal structure.
> IMO the code is harder to understand now.

I don't quite follow.  Previously we checked for 0x00 data
("if (!header[0] && !off)"), but we didn't check directly for 0xff.

If we read 0xff, we took the PCI_VPD_LRDT case, but it wouldn't match
ID_STRING, RO_DATA, or RW_DATA, so we'd fall out and check again
against ID_STRING, RO_DATA, and RW_DATA, and take the "invalid
%s VPD tag" error path because it doesn't match any.

This results in failure for any large resource except ID_STRING,
RO_DATA, and RW_DATA, regardless of the size.

My proposed code catches a different set of invalid things.
"size > PCI_VPD_MAX_SIZE" will catch any large resource headers with
length 0x8001 through 0xffff.

Possibly it should actually check for "off + size > PCI_VPD_MAX_SIZE"
so e.g., it would catch a 0x20 byte resource starting at 0x7ff0.

> > +				if (size > PCI_VPD_MAX_SIZE)
> > +					goto error;
> > +				off += PCI_VPD_LRDT_TAG_SIZE + size;
> >  			} else {
> >  				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
> >  					 tag, off);
> > @@ -105,14 +109,28 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >  			}
> >  		} else {
> >  			/* Short Resource Data Type Tag */
> > -			off += PCI_VPD_SRDT_TAG_SIZE +
> > -				pci_vpd_srdt_size(header);
> >  			tag = pci_vpd_srdt_tag(header);
> > +			size = pci_vpd_srdt_size(header);
> > +
> > +			/*
> > +			 * Missing EEPROM may read as 0x00.  A small item
> > +			 * must be at least 2 bytes.
> > +			 */
> > +			if (size == 0)
> > +				goto error;
> > +
> > +			off += PCI_VPD_SRDT_TAG_SIZE + size;
> >  			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
> >  				return off;
> >  		}
> >  	}
> >  	return 0;
> > +
> > +error:
> > +	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
> > +		 header[0], off, off == 0 ?
> > +		 "; assume missing optional EEPROM" : "");
> > +	return 0;
> >  }
> >  
> >  /*
> > 
> 

Re: [PATCH 4/5] PCI/VPD: Don't check Large Resource types for validity

[Bjorn Helgaas]

On Fri, Jul 16, 2021 at 08:03:45AM +0200, Hannes Reinecke wrote:
> On 7/15/21 11:59 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > VPD consists of a series of Small and Large Resources.  Computing the size
> > of VPD requires only the length of each, which is specified in the generic
> > tag of each resource.  We only expect to see ID_STRING, RO_DATA, and
> > RW_DATA in VPD, but it's not a problem if it contains other resource types.
> > 
> > Drop the validity checking of Large Resource items.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >   drivers/pci/vpd.c | 37 ++++++++++++++-----------------------
> >   1 file changed, 14 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > index 9c2744d79b53..d7a4a9f05bd6 100644
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -82,31 +82,22 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >   		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) {
> > -					pci_warn(dev, "failed VPD read at offset %zu",
> > -						 off + 1);
> > -					return 0;
> > -				}
> > -				size = pci_vpd_lrdt_size(header);
> > -
> > -				/*
> > -				 * Missing EEPROM may read as 0xff.
> > -				 * Length of 0xffff (65535) cannot be valid
> > -				 * because VPD can't be that large.
> > -				 */
> > -				if (size > PCI_VPD_MAX_SIZE)
> > -					goto error;
> > -				off += PCI_VPD_LRDT_TAG_SIZE + size;
> > -			} else {
> > -				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
> > -					 tag, off);
> > +			if (pci_read_vpd(dev, off + 1, 2, &header[1]) != 2) {
> > +				pci_warn(dev, "failed VPD read at offset %zu",
> > +					 off + 1);
> >   				return 0;
> >   			}
> > +			size = pci_vpd_lrdt_size(header);
> > +
> > +			/*
> > +			 * Missing EEPROM may read as 0xff.  Length of
> > +			 * 0xffff (65535) cannot be valid because VPD can't
> > +			 * be that large.
> > +			 */
> > +			if (size > PCI_VPD_MAX_SIZE)
> > +				goto error;
> > +
> > +			off += PCI_VPD_LRDT_TAG_SIZE + size;
> >   		} else {
> >   			/* Short Resource Data Type Tag */
> >   			tag = pci_vpd_srdt_tag(header);
> > 
> I'm not entirely happy with this; we really have to rely on well-formed VPD
> tags for the protocol to work correctly, and that's why I took the cautious
> approach. But with the check for missing EEPROM I hope we've covered the
> most common causes for invalid tags. Let's see how it goes.

True.  The tags need to be well-formed in the sense of having
intelligible lengths so we can identify the beginning and end of each
resource.  But we don't actually depend on the resource name
(ID_STRING, etc) or the content.

> Reviewed-by: Hannes Reinecke <hare@suse.de>
> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke                Kernel Storage Architect
> hare@suse.de                              +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 3/5] PCI/VPD: Consolidate missing EEPROM checks

[Heiner Kallweit]

On 29.07.2021 01:42, Bjorn Helgaas wrote:
> On Fri, Jul 16, 2021 at 12:16:55AM +0200, Heiner Kallweit wrote:
>> On 15.07.2021 23:59, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> A missing VPD EEPROM typically reads as either all 0xff or all zeroes.
>>> Both cases lead to invalid VPD resource items.  A 0xff tag would be a Large
>>> Resource with length 0xffff (65535).  That's invalid because VPD can only
>>> be 32768 bytes, limited by the size of the address register in the VPD
>>> Capability.
>>>
>>> A VPD that reads as all zeroes is also invalid because a 0x00 tag is a
>>> Small Resource with length 0, which would result in an item of length 1.
>>> This isn't explicitly illegal in PCIe r5.0, sec 6.28, but the format is
>>> derived from PNP ISA, which *does* say "a small resource data type may be
>>> 2-8 bytes in size" (Plug and Play ISA v1.0a, sec 6.2.2.
>>>
>>> Check for these invalid tags and return VPD size of zero if we find them.
>>> If they occur at the beginning of VPD, assume it's the result of a missing
>>> EEPROM.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>  drivers/pci/vpd.c | 36 +++++++++++++++++++++++++++---------
>>>  1 file changed, 27 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
>>> index 9b54dd95e42c..9c2744d79b53 100644
>>> --- a/drivers/pci/vpd.c
>>> +++ b/drivers/pci/vpd.c
>>> @@ -77,11 +77,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>>>  
>>>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
>>>  		unsigned char tag;
>>> -
>>> -		if (!header[0] && !off) {
>>> -			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
>>> -			return 0;
>>> -		}
>>> +		size_t size;
>>>  
>>>  		if (header[0] & PCI_VPD_LRDT) {
>>>  			/* Large Resource Data Type Tag */
>>> @@ -96,8 +92,16 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>>>  						 off + 1);
>>>  					return 0;
>>>  				}
>>> -				off += PCI_VPD_LRDT_TAG_SIZE +
>>> -					pci_vpd_lrdt_size(header);
>>> +				size = pci_vpd_lrdt_size(header);
>>> +
>>> +				/*
>>> +				 * Missing EEPROM may read as 0xff.
>>> +				 * Length of 0xffff (65535) cannot be valid
>>> +				 * because VPD can't be that large.
>>> +				 */
>>
>> I'm not fully convinced. Instead of checking for a "no VPD EPROM" read (00/ff)
>> directly, we now do it indirectly based on the internal tag structure.
>> We have pci_vpd_lrdt_size() et al to encapsulate the internal structure.
>> IMO the code is harder to understand now.
> 
> I don't quite follow.  Previously we checked for 0x00 data
> ("if (!header[0] && !off)"), but we didn't check directly for 0xff.
> 
> If we read 0xff, we took the PCI_VPD_LRDT case, but it wouldn't match
> ID_STRING, RO_DATA, or RW_DATA, so we'd fall out and check again
> against ID_STRING, RO_DATA, and RW_DATA, and take the "invalid
> %s VPD tag" error path because it doesn't match any.
> 
> This results in failure for any large resource except ID_STRING,
> RO_DATA, and RW_DATA, regardless of the size.
> 
> My proposed code catches a different set of invalid things.
> "size > PCI_VPD_MAX_SIZE" will catch any large resource headers with
> length 0x8001 through 0xffff.
> 
> Possibly it should actually check for "off + size > PCI_VPD_MAX_SIZE"
> so e.g., it would catch a 0x20 byte resource starting at 0x7ff0.
> 

For handling the 0xff case w/o additional overhead the following is pending:
https://patchwork.kernel.org/project/linux-pci/patch/8de8c906-9284-93b9-bb44-4ffdc3470740@gmail.com/

As far as I can see the VPD structure hasn't changed since it was
introduced in PCI 2.2. This was how many years ago?
Instead of adding code at least my personal objective would be
to make support for such legacy features as simple and maintainable
as possible.

>>> +				if (size > PCI_VPD_MAX_SIZE)
>>> +					goto error;
>>> +				off += PCI_VPD_LRDT_TAG_SIZE + size;
>>>  			} else {
>>>  				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
>>>  					 tag, off);
>>> @@ -105,14 +109,28 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>>>  			}
>>>  		} else {
>>>  			/* Short Resource Data Type Tag */
>>> -			off += PCI_VPD_SRDT_TAG_SIZE +
>>> -				pci_vpd_srdt_size(header);
>>>  			tag = pci_vpd_srdt_tag(header);
>>> +			size = pci_vpd_srdt_size(header);
>>> +
>>> +			/*
>>> +			 * Missing EEPROM may read as 0x00.  A small item
>>> +			 * must be at least 2 bytes.
>>> +			 */
>>> +			if (size == 0)
>>> +				goto error;
>>> +
>>> +			off += PCI_VPD_SRDT_TAG_SIZE + size;
>>>  			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
>>>  				return off;
>>>  		}
>>>  	}
>>>  	return 0;
>>> +
>>> +error:
>>> +	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
>>> +		 header[0], off, off == 0 ?
>>> +		 "; assume missing optional EEPROM" : "");
>>> +	return 0;
>>>  }
>>>  
>>>  /*
>>>
>>

Re: [PATCH 3/5] PCI/VPD: Consolidate missing EEPROM checks

[Bjorn Helgaas]

On Thu, Jul 29, 2021 at 08:10:25AM +0200, Heiner Kallweit wrote:
> On 29.07.2021 01:42, Bjorn Helgaas wrote:
> > On Fri, Jul 16, 2021 at 12:16:55AM +0200, Heiner Kallweit wrote:
> >> On 15.07.2021 23:59, Bjorn Helgaas wrote:
> >>> From: Bjorn Helgaas <bhelgaas@google.com>
> >>>
> >>> A missing VPD EEPROM typically reads as either all 0xff or all zeroes.
> >>> Both cases lead to invalid VPD resource items.  A 0xff tag would be a Large
> >>> Resource with length 0xffff (65535).  That's invalid because VPD can only
> >>> be 32768 bytes, limited by the size of the address register in the VPD
> >>> Capability.
> >>>
> >>> A VPD that reads as all zeroes is also invalid because a 0x00 tag is a
> >>> Small Resource with length 0, which would result in an item of length 1.
> >>> This isn't explicitly illegal in PCIe r5.0, sec 6.28, but the format is
> >>> derived from PNP ISA, which *does* say "a small resource data type may be
> >>> 2-8 bytes in size" (Plug and Play ISA v1.0a, sec 6.2.2.
> >>>
> >>> Check for these invalid tags and return VPD size of zero if we find them.
> >>> If they occur at the beginning of VPD, assume it's the result of a missing
> >>> EEPROM.
> >>>
> >>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >>> ---
> >>>  drivers/pci/vpd.c | 36 +++++++++++++++++++++++++++---------
> >>>  1 file changed, 27 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> >>> index 9b54dd95e42c..9c2744d79b53 100644
> >>> --- a/drivers/pci/vpd.c
> >>> +++ b/drivers/pci/vpd.c
> >>> @@ -77,11 +77,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >>>  
> >>>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
> >>>  		unsigned char tag;
> >>> -
> >>> -		if (!header[0] && !off) {
> >>> -			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
> >>> -			return 0;
> >>> -		}
> >>> +		size_t size;
> >>>  
> >>>  		if (header[0] & PCI_VPD_LRDT) {
> >>>  			/* Large Resource Data Type Tag */
> >>> @@ -96,8 +92,16 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >>>  						 off + 1);
> >>>  					return 0;
> >>>  				}
> >>> -				off += PCI_VPD_LRDT_TAG_SIZE +
> >>> -					pci_vpd_lrdt_size(header);
> >>> +				size = pci_vpd_lrdt_size(header);
> >>> +
> >>> +				/*
> >>> +				 * Missing EEPROM may read as 0xff.
> >>> +				 * Length of 0xffff (65535) cannot be valid
> >>> +				 * because VPD can't be that large.
> >>> +				 */
> >>
> >> I'm not fully convinced. Instead of checking for a "no VPD EPROM" read (00/ff)
> >> directly, we now do it indirectly based on the internal tag structure.
> >> We have pci_vpd_lrdt_size() et al to encapsulate the internal structure.
> >> IMO the code is harder to understand now.
> > 
> > I don't quite follow.  Previously we checked for 0x00 data
> > ("if (!header[0] && !off)"), but we didn't check directly for 0xff.
> > 
> > If we read 0xff, we took the PCI_VPD_LRDT case, but it wouldn't match
> > ID_STRING, RO_DATA, or RW_DATA, so we'd fall out and check again
> > against ID_STRING, RO_DATA, and RW_DATA, and take the "invalid
> > %s VPD tag" error path because it doesn't match any.
> > 
> > This results in failure for any large resource except ID_STRING,
> > RO_DATA, and RW_DATA, regardless of the size.
> > 
> > My proposed code catches a different set of invalid things.
> > "size > PCI_VPD_MAX_SIZE" will catch any large resource headers with
> > length 0x8001 through 0xffff.
> > 
> > Possibly it should actually check for "off + size > PCI_VPD_MAX_SIZE"
> > so e.g., it would catch a 0x20 byte resource starting at 0x7ff0.
> > 
> 
> For handling the 0xff case w/o additional overhead the following is pending:
> https://patchwork.kernel.org/project/linux-pci/patch/8de8c906-9284-93b9-bb44-4ffdc3470740@gmail.com/

Makes sense, I'll add that in the middle and post a v2 so you can see
what you think.

> As far as I can see the VPD structure hasn't changed since it was
> introduced in PCI 2.2. This was how many years ago?
> Instead of adding code at least my personal objective would be
> to make support for such legacy features as simple and maintainable
> as possible.
> 
> >>> +				if (size > PCI_VPD_MAX_SIZE)
> >>> +					goto error;
> >>> +				off += PCI_VPD_LRDT_TAG_SIZE + size;
> >>>  			} else {
> >>>  				pci_warn(dev, "invalid large VPD tag %02x at offset %zu",
> >>>  					 tag, off);
> >>> @@ -105,14 +109,28 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >>>  			}
> >>>  		} else {
> >>>  			/* Short Resource Data Type Tag */
> >>> -			off += PCI_VPD_SRDT_TAG_SIZE +
> >>> -				pci_vpd_srdt_size(header);
> >>>  			tag = pci_vpd_srdt_tag(header);
> >>> +			size = pci_vpd_srdt_size(header);
> >>> +
> >>> +			/*
> >>> +			 * Missing EEPROM may read as 0x00.  A small item
> >>> +			 * must be at least 2 bytes.
> >>> +			 */
> >>> +			if (size == 0)
> >>> +				goto error;
> >>> +
> >>> +			off += PCI_VPD_SRDT_TAG_SIZE + size;
> >>>  			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
> >>>  				return off;
> >>>  		}
> >>>  	}
> >>>  	return 0;
> >>> +
> >>> +error:
> >>> +	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
> >>> +		 header[0], off, off == 0 ?
> >>> +		 "; assume missing optional EEPROM" : "");
> >>> +	return 0;
> >>>  }
> >>>  
> >>>  /*
> >>>
> >>
> 

Re: [PATCH 0/5] PCI/VPD: Further improvements

[Bjorn Helgaas]

On Thu, May 13, 2021 at 10:53:44PM +0200, Heiner Kallweit wrote:
> Series with further improvements to PCI VPD handling.
> 
> Heiner Kallweit (5):
>   PCI/VPD: Refactor pci_vpd_size
>   PCI: Clean up VPD constants and functions in pci.h
>   PCI/VPD: Remove old_size argument from pci_vpd_size
>   PCI/VPD: Make pci_vpd_wait uninterruptible
>   PCI/VPD: Remove pci_vpd member flag
> 
>  drivers/pci/vpd.c   | 106 ++++++++++++++++----------------------------
>  include/linux/pci.h |  43 ------------------
>  2 files changed, 37 insertions(+), 112 deletions(-)

Applied to pci/vpd for v5.15, thanks!

Re: [PATCH 0/5] PCI/VPD: Further improvements

[Heiner Kallweit]

On 03.08.2021 00:32, Bjorn Helgaas wrote:
> On Thu, May 13, 2021 at 10:53:44PM +0200, Heiner Kallweit wrote:
>> Series with further improvements to PCI VPD handling.
>>
>> Heiner Kallweit (5):
>>   PCI/VPD: Refactor pci_vpd_size
>>   PCI: Clean up VPD constants and functions in pci.h
>>   PCI/VPD: Remove old_size argument from pci_vpd_size
>>   PCI/VPD: Make pci_vpd_wait uninterruptible
>>   PCI/VPD: Remove pci_vpd member flag
>>
>>  drivers/pci/vpd.c   | 106 ++++++++++++++++----------------------------
>>  include/linux/pci.h |  43 ------------------
>>  2 files changed, 37 insertions(+), 112 deletions(-)
> 
> Applied to pci/vpd for v5.15, thanks!
> 
pci/vpd hasn't been merged yet into next, is this intentional?

Re: [PATCH 0/5] PCI/VPD: Further improvements

[Bjorn Helgaas]

On Thu, Aug 05, 2021 at 09:10:51PM +0200, Heiner Kallweit wrote:
> On 03.08.2021 00:32, Bjorn Helgaas wrote:
> > On Thu, May 13, 2021 at 10:53:44PM +0200, Heiner Kallweit wrote:
> >> Series with further improvements to PCI VPD handling.
> >>
> >> Heiner Kallweit (5):
> >>   PCI/VPD: Refactor pci_vpd_size
> >>   PCI: Clean up VPD constants and functions in pci.h
> >>   PCI/VPD: Remove old_size argument from pci_vpd_size
> >>   PCI/VPD: Make pci_vpd_wait uninterruptible
> >>   PCI/VPD: Remove pci_vpd member flag
> >>
> >>  drivers/pci/vpd.c   | 106 ++++++++++++++++----------------------------
> >>  include/linux/pci.h |  43 ------------------
> >>  2 files changed, 37 insertions(+), 112 deletions(-)
> > 
> > Applied to pci/vpd for v5.15, thanks!
> > 
> pci/vpd hasn't been merged yet into next, is this intentional?

I let the 0-day bot build things before merging into next.  But thanks
for the reminder, there are a couple branches I should merge.

Bjorn