* [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups
@ 2021-07-29 18:42 Bjorn Helgaas
2021-07-29 18:42 ` [PATCH v2 1/6] PCI/VPD: Correct diagnostic for VPD read failure Bjorn Helgaas
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Bjorn Helgaas @ 2021-07-29 18:42 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/6] PCI/VPD: Correct diagnostic for VPD read failure 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas @ 2021-07-29 18:42 ` Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 2/6] PCI/VPD: Check Resource Item Names against those valid for type Bjorn Helgaas ` (5 subsequent siblings) 6 siblings, 0 replies; 13+ messages in thread From: Bjorn Helgaas @ 2021-07-29 18:42 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/6] PCI/VPD: Check Resource Item Names against those valid for type 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 1/6] PCI/VPD: Correct diagnostic for VPD read failure Bjorn Helgaas @ 2021-07-29 18:42 ` Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM Bjorn Helgaas ` (4 subsequent siblings) 6 siblings, 0 replies; 13+ messages in thread From: Bjorn Helgaas @ 2021-07-29 18:42 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 1/6] PCI/VPD: Correct diagnostic for VPD read failure Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 2/6] PCI/VPD: Check Resource Item Names against those valid for type Bjorn Helgaas @ 2021-07-29 18:42 ` Bjorn Helgaas 2021-07-30 6:04 ` Hannes Reinecke 2021-07-29 18:42 ` [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size Bjorn Helgaas ` (3 subsequent siblings) 6 siblings, 1 reply; 13+ messages in thread From: Bjorn Helgaas @ 2021-07-29 18:42 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM 2021-07-29 18:42 ` [PATCH v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM Bjorn Helgaas @ 2021-07-30 6:04 ` Hannes Reinecke 0 siblings, 0 replies; 13+ messages in thread From: Hannes Reinecke @ 2021-07-30 6:04 UTC (permalink / raw) To: Bjorn Helgaas, Heiner Kallweit; +Cc: linux-pci, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas ` (2 preceding siblings ...) 2021-07-29 18:42 ` [PATCH v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM Bjorn Helgaas @ 2021-07-29 18:42 ` Bjorn Helgaas 2021-07-30 6:07 ` Hannes Reinecke 2021-08-09 18:15 ` Qian Cai 2021-07-29 18:42 ` [PATCH v2 5/6] PCI/VPD: Don't check Large Resource Item Names for validity Bjorn Helgaas ` (2 subsequent siblings) 6 siblings, 2 replies; 13+ messages in thread From: Bjorn Helgaas @ 2021-07-29 18:42 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size 2021-07-29 18:42 ` [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size Bjorn Helgaas @ 2021-07-30 6:07 ` Hannes Reinecke 2021-08-09 18:15 ` Qian Cai 1 sibling, 0 replies; 13+ messages in thread From: Hannes Reinecke @ 2021-07-30 6:07 UTC (permalink / raw) To: Bjorn Helgaas, Heiner Kallweit; +Cc: linux-pci, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size 2021-07-29 18:42 ` [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size Bjorn Helgaas 2021-07-30 6:07 ` Hannes Reinecke @ 2021-08-09 18:15 ` Qian Cai 2021-08-09 18:46 ` Bjorn Helgaas 1 sibling, 1 reply; 13+ messages in thread From: Qian Cai @ 2021-08-09 18:15 UTC (permalink / raw) To: Bjorn Helgaas, Heiner Kallweit Cc: Hannes Reinecke, linux-pci, Bjorn Helgaas, Saeed Mahameed, Leon Romanovsky 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; > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size 2021-08-09 18:15 ` Qian Cai @ 2021-08-09 18:46 ` Bjorn Helgaas 2021-08-09 18:57 ` Heiner Kallweit 0 siblings, 1 reply; 13+ messages in thread From: Bjorn Helgaas @ 2021-08-09 18:46 UTC (permalink / raw) To: Qian Cai Cc: Heiner Kallweit, Hannes Reinecke, linux-pci, Bjorn Helgaas, Saeed Mahameed, Leon Romanovsky 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; } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size 2021-08-09 18:46 ` Bjorn Helgaas @ 2021-08-09 18:57 ` Heiner Kallweit 0 siblings, 0 replies; 13+ messages in thread From: Heiner Kallweit @ 2021-08-09 18:57 UTC (permalink / raw) To: Bjorn Helgaas, Qian Cai Cc: Hannes Reinecke, linux-pci, Bjorn Helgaas, Saeed Mahameed, Leon Romanovsky 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; > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 5/6] PCI/VPD: Don't check Large Resource Item Names for validity 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas ` (3 preceding siblings ...) 2021-07-29 18:42 ` [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size Bjorn Helgaas @ 2021-07-29 18:42 ` Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 6/6] PCI/VPD: Allow access to valid parts of VPD if some is invalid Bjorn Helgaas 2021-08-02 22:29 ` [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas 6 siblings, 0 replies; 13+ messages in thread From: Bjorn Helgaas @ 2021-07-29 18:42 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 6/6] PCI/VPD: Allow access to valid parts of VPD if some is invalid 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas ` (4 preceding siblings ...) 2021-07-29 18:42 ` [PATCH v2 5/6] PCI/VPD: Don't check Large Resource Item Names for validity Bjorn Helgaas @ 2021-07-29 18:42 ` Bjorn Helgaas 2021-08-02 22:29 ` [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas 6 siblings, 0 replies; 13+ messages in thread From: Bjorn Helgaas @ 2021-07-29 18:42 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas ` (5 preceding siblings ...) 2021-07-29 18:42 ` [PATCH v2 6/6] PCI/VPD: Allow access to valid parts of VPD if some is invalid Bjorn Helgaas @ 2021-08-02 22:29 ` Bjorn Helgaas 6 siblings, 0 replies; 13+ messages in thread From: Bjorn Helgaas @ 2021-08-02 22:29 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Hannes Reinecke, linux-pci, 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-08-09 18:57 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-29 18:42 [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 1/6] PCI/VPD: Correct diagnostic for VPD read failure Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 2/6] PCI/VPD: Check Resource Item Names against those valid for type Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 3/6] PCI/VPD: Treat initial 0xff as missing EEPROM Bjorn Helgaas 2021-07-30 6:04 ` Hannes Reinecke 2021-07-29 18:42 ` [PATCH v2 4/6] PCI/VPD: Reject resource tags with invalid size Bjorn Helgaas 2021-07-30 6:07 ` Hannes Reinecke 2021-08-09 18:15 ` Qian Cai 2021-08-09 18:46 ` Bjorn Helgaas 2021-08-09 18:57 ` Heiner Kallweit 2021-07-29 18:42 ` [PATCH v2 5/6] PCI/VPD: Don't check Large Resource Item Names for validity Bjorn Helgaas 2021-07-29 18:42 ` [PATCH v2 6/6] PCI/VPD: Allow access to valid parts of VPD if some is invalid Bjorn Helgaas 2021-08-02 22:29 ` [PATCH v2 0/6] PCI/VPD: pci_vpd_size() cleanups Bjorn Helgaas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.