On Fri, Jul 16, 2021 at 12:16:55AM +0200, Heiner Kallweit wrote:
> 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.
I don't quite follow. Previously we checked for 0x00 data
("if (!header[0] && !off)"), but we didn't check directly for 0xff.
If we read 0xff, we took the PCI_VPD_LRDT case, but it wouldn't match
ID_STRING, RO_DATA, or RW_DATA, so we'd fall out and check again
against ID_STRING, RO_DATA, and RW_DATA, and take the "invalid
%s VPD tag" error path because it doesn't match any.
This results in failure for any large resource except ID_STRING,
RO_DATA, and RW_DATA, regardless of the size.
My proposed code catches a different set of invalid things.
"size > PCI_VPD_MAX_SIZE" will catch any large resource headers with
length 0x8001 through 0xffff.
Possibly it should actually check for "off + size > PCI_VPD_MAX_SIZE"
so e.g., it would catch a 0x20 byte resource starting at 0x7ff0.
> > + 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;
> > }
> >
> > /*
> >
>
