On 15.07.2021 23:59, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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;
> }
>
> /*
>
