diff mbox series

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

Message ID 20210715215959.2014576-5-helgaas@kernel.org (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI/VPD: pci_vpd_size() cleanups | expand

Commit Message

Bjorn Helgaas July 15, 2021, 9:59 p.m. UTC 
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(-)

Comments

Hannes Reinecke July 16, 2021, 6:03 a.m. UTC | #1 
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
Bjorn Helgaas July 28, 2021, 11:46 p.m. UTC | #2 
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 mbox series

Patch

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);