Re: [PATCH 4/5] PCI/VPD: Don't check Large Resource types for validity

[Bjorn Helgaas <helgaas@kernel.org>]

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