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

[Bjorn Helgaas <helgaas@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> 
> + 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;
 }