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@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.
>
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;
> }
>
