| Message ID | 20210715215959.2014576-5-helgaas@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/VPD: pci_vpd_size() cleanups | expand |
On 7/15/21 11:59 PM, Bjorn Helgaas wrote: > 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. > > Drop the validity checking of Large Resource items. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/vpd.c | 37 ++++++++++++++----------------------- > 1 file changed, 14 insertions(+), 23 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index 9c2744d79b53..d7a4a9f05bd6 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -82,31 +82,22 @@ 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); > - > - /* > - * Missing EEPROM may read as 0xff. > - * Length of 0xffff (65535) cannot be valid > - * because VPD can't be that large. > - */ > - 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); > + 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); > + > + /* > + * Missing EEPROM may read as 0xff. Length of > + * 0xffff (65535) cannot be valid because VPD can't > + * be that large. > + */ > + if (size > PCI_VPD_MAX_SIZE) > + goto error; > + > + off += PCI_VPD_LRDT_TAG_SIZE + size; > } else { > /* Short Resource Data Type Tag */ > tag = pci_vpd_srdt_tag(header); > I'm not entirely happy with this; we really have to rely on well-formed VPD tags for the protocol to work correctly, and that's why I took the cautious approach. But with the check for missing EEPROM I hope we've covered the most common causes for invalid tags. Let's see how it goes. Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On Fri, Jul 16, 2021 at 08:03:45AM +0200, Hannes Reinecke wrote: > On 7/15/21 11:59 PM, Bjorn Helgaas wrote: > > 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. > > > > Drop the validity checking of Large Resource items. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > drivers/pci/vpd.c | 37 ++++++++++++++----------------------- > > 1 file changed, 14 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > > index 9c2744d79b53..d7a4a9f05bd6 100644 > > --- a/drivers/pci/vpd.c > > +++ b/drivers/pci/vpd.c > > @@ -82,31 +82,22 @@ 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); > > - > > - /* > > - * Missing EEPROM may read as 0xff. > > - * Length of 0xffff (65535) cannot be valid > > - * because VPD can't be that large. > > - */ > > - 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); > > + 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); > > + > > + /* > > + * Missing EEPROM may read as 0xff. Length of > > + * 0xffff (65535) cannot be valid because VPD can't > > + * be that large. > > + */ > > + if (size > PCI_VPD_MAX_SIZE) > > + goto error; > > + > > + off += PCI_VPD_LRDT_TAG_SIZE + size; > > } else { > > /* Short Resource Data Type Tag */ > > tag = pci_vpd_srdt_tag(header); > > > I'm not entirely happy with this; we really have to rely on well-formed VPD > tags for the protocol to work correctly, and that's why I took the cautious > approach. But with the check for missing EEPROM I hope we've covered the > most common causes for invalid tags. Let's see how it goes. True. The tags need to be well-formed in the sense of having intelligible lengths so we can identify the beginning and end of each resource. But we don't actually depend on the resource name (ID_STRING, etc) or the content. > 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
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c index 9c2744d79b53..d7a4a9f05bd6 100644 --- a/drivers/pci/vpd.c +++ b/drivers/pci/vpd.c @@ -82,31 +82,22 @@ 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); - - /* - * Missing EEPROM may read as 0xff. - * Length of 0xffff (65535) cannot be valid - * because VPD can't be that large. - */ - 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); + 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); + + /* + * Missing EEPROM may read as 0xff. Length of + * 0xffff (65535) cannot be valid because VPD can't + * be that large. + */ + if (size > PCI_VPD_MAX_SIZE) + goto error; + + off += PCI_VPD_LRDT_TAG_SIZE + size; } else { /* Short Resource Data Type Tag */ tag = pci_vpd_srdt_tag(header);