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

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

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The basic idea is to validate VPD resource *size* without validating the
actual content, since the kernel really doesn't care about the content.

Thanks very much for the feedback on v1, and I'd be glad for any additional
feedback.

Follow-up to:
https://lore.kernel.org/r/20210715215959.2014576-1-helgaas@kernel.org

Changes since v1:
  - Incorporate Heiner's patch to reject VPD if first byte is 0x00 or 0xff
    (https://lore.kernel.org/r/8de8c906-9284-93b9-bb44-4ffdc3470740@gmail.com/)
  - Update size checks to reject resources that would extend past the
    maximum VPD size

Bjorn Helgaas (5):
  PCI/VPD: Correct diagnostic for VPD read failure
  PCI/VPD: Check Resource Item Names against those valid for type
  PCI/VPD: Reject resource tags with invalid size
  PCI/VPD: Don't check Large Resource Item Names for validity
  PCI/VPD: Allow access to valid parts of VPD if some is invalid

Heiner Kallweit (1):
  PCI/VPD: Treat initial 0xff as missing EEPROM

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

-- 
2.25.1

[PATCH v2 1/6] 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 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 v2 2/6] PCI/VPD: Check Resource Item Names 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
Item Names 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 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 v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM

[Bjorn Helgaas]

From: Heiner Kallweit <hkallweit1@gmail.com>

Previously we assumed that the first tag being 0x00 meant an EEPROM was
missing.  The first tag being 0xff means the same thing; check for that
also.

[bhelgaas: rework error mesage]
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/vpd.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 9b54dd95e42c..66703de2cf2b 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -78,10 +78,8 @@ 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;
-		}
+		if (off == 0 && (header[0] == 0x00 || header[0] == 0xff))
+			goto error;
 
 		if (header[0] & PCI_VPD_LRDT) {
 			/* Large Resource Data Type Tag */
@@ -113,6 +111,12 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
 		}
 	}
 	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 v2 4/6] PCI/VPD: Reject resource tags with invalid size

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

VPD is limited in size by the 15-bit VPD Address field in the VPD
Capability.  Each resource tag includes a length that determines the
overall size of the resource.  Reject any resources that would extend past
the maximum VPD size.

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

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 66703de2cf2b..e52382050e3e 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -77,6 +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;
+		size_t size;
 
 		if (off == 0 && (header[0] == 0x00 || header[0] == 0xff))
 			goto error;
@@ -94,8 +95,11 @@ 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);
+				if (off + 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);
@@ -103,9 +107,12 @@ 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);
+			if (size == 0 || off + size > PCI_VPD_MAX_SIZE)
+				goto error;
+
+			off += PCI_VPD_SRDT_TAG_SIZE + size;
 			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
 				return off;
 		}
-- 
2.25.1

[PATCH v2 5/6] PCI/VPD: Don't check Large Resource Item Names 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
because all we care about is the size.

Drop the validity checking of Large Resource items.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/pci/vpd.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index e52382050e3e..6fa09e969d6e 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -85,26 +85,11 @@ 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);
-				if (off + size > PCI_VPD_MAX_SIZE)
-					goto error;
+			size = pci_vpd_lrdt_size(header);
+			if (off + 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);
-				return 0;
-			}
+			off += PCI_VPD_LRDT_TAG_SIZE + size;
 		} else {
 			/* Short Resource Data Type Tag */
 			tag = pci_vpd_srdt_tag(header);
-- 
2.25.1

[PATCH v2 6/6] 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/pci/vpd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 6fa09e969d6e..f74d2f5194e4 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -85,6 +85,11 @@ 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);
+			if (pci_read_vpd(dev, off + 1, 2, &header[1]) != 2) {
+				pci_warn(dev, "failed VPD read at offset %zu",
+					 off + 1);
+				return off;
+			}
 			size = pci_vpd_lrdt_size(header);
 			if (off + size > PCI_VPD_MAX_SIZE)
 				goto error;
@@ -102,13 +107,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 v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM

[Hannes Reinecke]

On 7/29/21 8:42 PM, Bjorn Helgaas wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Previously we assumed that the first tag being 0x00 meant an EEPROM was
> missing.  The first tag being 0xff means the same thing; check for that
> also.
> 
> [bhelgaas: rework error mesage]
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/vpd.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 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 v2 4/6] PCI/VPD: Reject resource tags with invalid size

[Hannes Reinecke]

On 7/29/21 8:42 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> VPD is limited in size by the 15-bit VPD Address field in the VPD
> Capability.  Each resource tag includes a length that determines the
> overall size of the resource.  Reject any resources that would extend past
> the maximum VPD size.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/vpd.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 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 v2 0/6] PCI/VPD: pci_vpd_size() cleanups

[Bjorn Helgaas]

On Thu, Jul 29, 2021 at 01:42:28PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The basic idea is to validate VPD resource *size* without validating the
> actual content, since the kernel really doesn't care about the content.
> 
> Thanks very much for the feedback on v1, and I'd be glad for any additional
> feedback.
> 
> Follow-up to:
> https://lore.kernel.org/r/20210715215959.2014576-1-helgaas@kernel.org
> 
> Changes since v1:
>   - Incorporate Heiner's patch to reject VPD if first byte is 0x00 or 0xff
>     (https://lore.kernel.org/r/8de8c906-9284-93b9-bb44-4ffdc3470740@gmail.com/)
>   - Update size checks to reject resources that would extend past the
>     maximum VPD size
> 
> Bjorn Helgaas (5):
>   PCI/VPD: Correct diagnostic for VPD read failure
>   PCI/VPD: Check Resource Item Names against those valid for type
>   PCI/VPD: Reject resource tags with invalid size
>   PCI/VPD: Don't check Large Resource Item Names for validity
>   PCI/VPD: Allow access to valid parts of VPD if some is invalid
> 
> Heiner Kallweit (1):
>   PCI/VPD: Treat initial 0xff as missing EEPROM
> 
>  drivers/pci/vpd.c | 55 +++++++++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 30 deletions(-)

I applied this to pci/vpd for v5.15.

Thanks for the feedback so far, and I'll still be happy to receive
more.

Re: [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size

[Qian Cai]

On 7/29/2021 2:42 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> VPD is limited in size by the 15-bit VPD Address field in the VPD
> Capability.  Each resource tag includes a length that determines the
> overall size of the resource.  Reject any resources that would extend past
> the maximum VPD size.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

+ mlx5_core maintainers

Hi there, running the latest linux-next with this commit, our system started to
get noisy. Could those indicate some device firmware bugs?

[  164.937191] mlx5_core 0000:01:00.0: invalid VPD tag 0x78 at offset 113
[  165.933527] mlx5_core 0000:01:00.1: invalid VPD tag 0x78 at offset 113

0000:01:00.0 Ethernet controller: Mellanox Technologies MT27710 Family [ConnectX-4 Lx]
	Subsystem: Mellanox Technologies MCX4421A-ACQN ConnectX-4 Lx EN OCP,2x25G
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 318
	Region 0: Memory at 10100000000 (64-bit, prefetchable) [size=32M]
	Expansion ROM at 10030000000 [disabled] [size=1M]
	Capabilities: [60] Express (v2) Endpoint, MSI 00
		DevCap:	MaxPayload 512 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
			ExtTag+ AttnBtn- AttnInd- PwrInd- RBE+ FLReset+ SlotPowerLimit 75.000W
		DevCtl:	CorrErr- NonFatalErr- FatalErr- UnsupReq-
			RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop+ FLReset-
			MaxPayload 512 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr- TransPend-
		LnkCap:	Port #0, Speed 8GT/s, Width x8, ASPM L1, Exit Latency L1 <4us
			ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp+
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 8GT/s (ok), Width x8 (ok)
			TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
		DevCap2: Completion Timeout: Range ABC, TimeoutDis+, NROPrPrP-, LTR-
			 10BitTagComp-, 10BitTagReq-, OBFF Not Supported, ExtFmt-, EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS-, TPHComp-, ExtTPHComp-
			 AtomicOpsCap: 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 260ms to 900ms, TimeoutDis-, LTR-, OBFF Disabled
			 AtomicOpsCtl: ReqEn-
		LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete+, EqualizationPhase1+
			 EqualizationPhase2+, EqualizationPhase3+, LinkEqualizationRequest-
	Capabilities: [48] Vital Product Data
		Product Name: CX4421A- ConnectX-4 LX SFP28
		Read-only fields:
			[PN] Part number: MCX4421A-ACQN        
			[EC] Engineering changes: AE
			[SN] Serial number: MT2042X16761            
			[V0] Vendor specific: PCIeGen3 x8     
			[RV] Reserved: checksum good, 0 byte(s) reserved
		No end tag found
	Capabilities: [9c] MSI-X: Enable+ Count=64 Masked-
		Vector table: BAR=0 offset=00002000
		PBA: BAR=0 offset=00003000
	Capabilities: [c0] Vendor Specific Information: Len=18 <?>
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
		Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [100 v1] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 04, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
	Capabilities: [150 v1] Alternative Routing-ID Interpretation (ARI)
		ARICap:	MFVC- ACS-, Next Function: 1
		ARICtl:	MFVC- ACS-, Function Group: 0
	Capabilities: [180 v1] Single Root I/O Virtualization (SR-IOV)
		IOVCap:	Migration-, Interrupt Message Number: 000
		IOVCtl:	Enable- Migration- Interrupt- MSE- ARIHierarchy+
		IOVSta:	Migration-
		Initial VFs: 8, Total VFs: 8, Number of VFs: 0, Function Dependency Link: 00
		VF offset: 2, stride: 1, Device ID: 1016
		Supported Page Size: 000007ff, System Page Size: 00000010
		Region 0: Memory at 0000010104000000 (64-bit, prefetchable)
		VF Migration: offset: 00000000, BIR: 0
	Capabilities: [1c0 v1] Secondary PCI Express
		LnkCtl3: LnkEquIntrruptEn-, PerformEqu-
		LaneErrStat: 0
	Capabilities: [230 v1] Access Control Services
		ACSCap:	SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd- EgressCtrl- DirectTrans-
		ACSCtl:	SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd- EgressCtrl- DirectTrans-
	Kernel driver in use: mlx5_core
	Kernel modules: mlx5_core


0000:01:00.1 Ethernet controller: Mellanox Technologies MT27710 Family [ConnectX-4 Lx]
	Subsystem: Mellanox Technologies MCX4421A-ACQN ConnectX-4 Lx EN OCP,2x25G
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin B routed to IRQ 392
	Region 0: Memory at 10102000000 (64-bit, prefetchable) [size=32M]
	Expansion ROM at 10030100000 [disabled] [size=1M]
	Capabilities: [60] Express (v2) Endpoint, MSI 00
		DevCap:	MaxPayload 512 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
			ExtTag+ AttnBtn- AttnInd- PwrInd- RBE+ FLReset+ SlotPowerLimit 75.000W
		DevCtl:	CorrErr- NonFatalErr- FatalErr- UnsupReq-
			RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop+ FLReset-
			MaxPayload 512 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr- TransPend-
		LnkCap:	Port #0, Speed 8GT/s, Width x8, ASPM L1, Exit Latency L1 <4us
			ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp+
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 8GT/s (ok), Width x8 (ok)
			TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
		DevCap2: Completion Timeout: Range ABC, TimeoutDis+, NROPrPrP-, LTR-
			 10BitTagComp-, 10BitTagReq-, OBFF Not Supported, ExtFmt-, EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS-, TPHComp-, ExtTPHComp-
			 AtomicOpsCap: 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 260ms to 900ms, TimeoutDis-, LTR-, OBFF Disabled
			 AtomicOpsCtl: ReqEn-
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1-
			 EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
	Capabilities: [48] Vital Product Data
		Product Name: CX4421A- ConnectX-4 LX SFP28
		Read-only fields:
			[PN] Part number: MCX4421A-ACQN        
			[EC] Engineering changes: AE
			[SN] Serial number: MT2042X16761            
			[V0] Vendor specific: PCIeGen3 x8     
			[RV] Reserved: checksum good, 0 byte(s) reserved
		No end tag found
	Capabilities: [9c] MSI-X: Enable+ Count=64 Masked-
		Vector table: BAR=0 offset=00002000
		PBA: BAR=0 offset=00003000
	Capabilities: [c0] Vendor Specific Information: Len=18 <?>
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
		Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [100 v1] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 04, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
	Capabilities: [150 v1] Alternative Routing-ID Interpretation (ARI)
		ARICap:	MFVC- ACS-, Next Function: 0
		ARICtl:	MFVC- ACS-, Function Group: 0
	Capabilities: [180 v1] Single Root I/O Virtualization (SR-IOV)
		IOVCap:	Migration-, Interrupt Message Number: 000
		IOVCtl:	Enable- Migration- Interrupt- MSE- ARIHierarchy-
		IOVSta:	Migration-
		Initial VFs: 8, Total VFs: 8, Number of VFs: 0, Function Dependency Link: 00
		VF offset: 9, stride: 1, Device ID: 1016
		Supported Page Size: 000007ff, System Page Size: 00000010
		Region 0: Memory at 0000010104800000 (64-bit, prefetchable)
		VF Migration: offset: 00000000, BIR: 0
	Capabilities: [230 v1] Access Control Services
		ACSCap:	SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd- EgressCtrl- DirectTrans-
		ACSCtl:	SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd- EgressCtrl- DirectTrans-
	Kernel driver in use: mlx5_core
	Kernel modules: mlx5_core


> ---
>  drivers/pci/vpd.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 66703de2cf2b..e52382050e3e 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -77,6 +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;
> +		size_t size;
>  
>  		if (off == 0 && (header[0] == 0x00 || header[0] == 0xff))
>  			goto error;
> @@ -94,8 +95,11 @@ 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);
> +				if (off + 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);
> @@ -103,9 +107,12 @@ 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);
> +			if (size == 0 || off + size > PCI_VPD_MAX_SIZE)
> +				goto error;
> +
> +			off += PCI_VPD_SRDT_TAG_SIZE + size;
>  			if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
>  				return off;
>  		}
> 

Re: [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size

[Bjorn Helgaas]

On Mon, Aug 09, 2021 at 02:15:20PM -0400, Qian Cai wrote:
> 
> 
> On 7/29/2021 2:42 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > VPD is limited in size by the 15-bit VPD Address field in the VPD
> > Capability.  Each resource tag includes a length that determines the
> > overall size of the resource.  Reject any resources that would extend past
> > the maximum VPD size.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> + mlx5_core maintainers
> 
> Hi there, running the latest linux-next with this commit, our system started to
> get noisy. Could those indicate some device firmware bugs?
> 
> [  164.937191] mlx5_core 0000:01:00.0: invalid VPD tag 0x78 at offset 113
> [  165.933527] mlx5_core 0000:01:00.1: invalid VPD tag 0x78 at offset 113

Thanks a lot for reporting this!

I guess VPD contains a tag of 0x78, which is a small resource item
with name 0xf (an End Tag) and size 0.  Per PNPISA, the End Tag should
have a length 1, with the single byte being a checksum of the resource
data.  But the PCI spec doesn't mention that checksum byte, so I think
we should accept the size 0 without any message.

I think the following patch on top of linux-next should do this:

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 5e2e638093f1..d7f705ba6664 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -69,12 +69,11 @@ EXPORT_SYMBOL(pci_write_vpd);
  */
 static size_t pci_vpd_size(struct pci_dev *dev)
 {
-	size_t off = 0;
-	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
+	size_t off = 0, size;
+	unsigned char tag, header[1+2];	/* 1 byte tag, 2 bytes length */
 
 	while (pci_read_vpd(dev, off, 1, header) == 1) {
-		unsigned char tag;
-		size_t size;
+		size = 0;
 
 		if (off == 0 && (header[0] == 0x00 || header[0] == 0xff))
 			goto error;
@@ -95,7 +94,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 			/* Short Resource Data Type Tag */
 			tag = pci_vpd_srdt_tag(header);
 			size = pci_vpd_srdt_size(header);
-			if (size == 0 || off + size > PCI_VPD_MAX_SIZE)
+			if (off + size > PCI_VPD_MAX_SIZE)
 				goto error;
 
 			off += PCI_VPD_SRDT_TAG_SIZE + size;
@@ -106,8 +105,8 @@ static size_t pci_vpd_size(struct pci_dev *dev)
 	return off;
 
 error:
-	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
-		 header[0], off, off == 0 ?
+	pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
+		 header[0], size, off, off == 0 ?
 		 "; assume missing optional EEPROM" : "");
 	return off;
 }

Re: [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size

[Heiner Kallweit]

On 09.08.2021 20:46, Bjorn Helgaas wrote:
> On Mon, Aug 09, 2021 at 02:15:20PM -0400, Qian Cai wrote:
>>
>>
>> On 7/29/2021 2:42 PM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> VPD is limited in size by the 15-bit VPD Address field in the VPD
>>> Capability.  Each resource tag includes a length that determines the
>>> overall size of the resource.  Reject any resources that would extend past
>>> the maximum VPD size.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> + mlx5_core maintainers
>>
>> Hi there, running the latest linux-next with this commit, our system started to
>> get noisy. Could those indicate some device firmware bugs?
>>
>> [  164.937191] mlx5_core 0000:01:00.0: invalid VPD tag 0x78 at offset 113
>> [  165.933527] mlx5_core 0000:01:00.1: invalid VPD tag 0x78 at offset 113
> 
> Thanks a lot for reporting this!
> 
> I guess VPD contains a tag of 0x78, which is a small resource item
> with name 0xf (an End Tag) and size 0.  Per PNPISA, the End Tag should
> have a length 1, with the single byte being a checksum of the resource
> data.  But the PCI spec doesn't mention that checksum byte, so I think
> we should accept the size 0 without any message.
> 

PCI 2.2 spec includes a VPD example in section I.3.2.
There the end tag is listed as 0x78.

> I think the following patch on top of linux-next should do this:
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 5e2e638093f1..d7f705ba6664 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -69,12 +69,11 @@ EXPORT_SYMBOL(pci_write_vpd);
>   */
>  static size_t pci_vpd_size(struct pci_dev *dev)
>  {
> -	size_t off = 0;
> -	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
> +	size_t off = 0, size;
> +	unsigned char tag, header[1+2];	/* 1 byte tag, 2 bytes length */
>  
>  	while (pci_read_vpd(dev, off, 1, header) == 1) {
> -		unsigned char tag;
> -		size_t size;
> +		size = 0;
>  
>  		if (off == 0 && (header[0] == 0x00 || header[0] == 0xff))
>  			goto error;
> @@ -95,7 +94,7 @@ static size_t pci_vpd_size(struct pci_dev *dev)
>  			/* Short Resource Data Type Tag */
>  			tag = pci_vpd_srdt_tag(header);
>  			size = pci_vpd_srdt_size(header);
> -			if (size == 0 || off + size > PCI_VPD_MAX_SIZE)
> +			if (off + size > PCI_VPD_MAX_SIZE)
>  				goto error;
>  
>  			off += PCI_VPD_SRDT_TAG_SIZE + size;
> @@ -106,8 +105,8 @@ static size_t pci_vpd_size(struct pci_dev *dev)
>  	return off;
>  
>  error:
> -	pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
> -		 header[0], off, off == 0 ?
> +	pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n",
> +		 header[0], size, off, off == 0 ?
>  		 "; assume missing optional EEPROM" : "");
>  	return off;
>  }
>