Re: [PATCH 1/5] PCI/VPD: Refactor pci_vpd_size

[Heiner Kallweit <hkallweit1@gmail.com>]

On 13.07.2021 22:25, Bjorn Helgaas wrote:
> [+cc Hannes]
> 
> On Thu, May 13, 2021 at 10:58:40PM +0200, Heiner Kallweit wrote:
>> The only Short Resource Data Type tag is the end tag. This allows to
>> remove the generic SRDT tag handling and the code be significantly
>> simplified.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/pci/vpd.c | 46 ++++++++++++----------------------------------
>>  1 file changed, 12 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
>> index 26bf7c877..ecdce170f 100644
>> --- a/drivers/pci/vpd.c
>> +++ b/drivers/pci/vpd.c
>> @@ -73,50 +73,28 @@ EXPORT_SYMBOL(pci_write_vpd);
>>  static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>>  {
>>  	size_t off = 0;
>> -	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
>> +	u8 header[3];	/* 1 byte tag, 2 bytes length */
>>  
>>  	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;
>>  		}
>>  
>> -		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, "invalid large VPD tag %02x size at offset %zu",
>> -						 tag, off + 1);
>> -					return 0;
>> -				}
>> -				off += PCI_VPD_LRDT_TAG_SIZE +
>> -					pci_vpd_lrdt_size(header);
>> -			}
>> -		} else {
>> -			/* Short Resource Data Type Tag */
>> -			off += PCI_VPD_SRDT_TAG_SIZE +
>> -				pci_vpd_srdt_size(header);
>> -			tag = pci_vpd_srdt_tag(header);
>> -		}
>> -
>> -		if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
>> -			return off;
>> +		if (header[0] == PCI_VPD_SRDT_END)
>> +			return off + PCI_VPD_SRDT_TAG_SIZE;
> 
> This makes the code beautiful.  But I think pci_vpd_size() is too
> picky even now, and this patch makes it more so.
> 
> I don't know why pci_vpd_size() currently checks the tags for
> ID_STRING, RO_DATA, and RW_DATA.  That seems too aggressive to me.
> 

It checks for these tags (+ the end tag) because it's the only ones
defined for VPD by the PCI spec.

> I think these data items originally came from PNP ISA, and it defines
> several other tags.  Of course, that wasn't for PCI devices, but a
> Google search for '"invalid" "vpd tag" "at offset"' does find several
> cases where VPD contains things that look like PNP ISA data items.
> 

Right, the tag format is based on PNP ISA. But PCI spec explicitly
lists the supported tags.
I tried to do the same search and found:
- "invalid short vpd tag 00" and "invalid large tag 7f"
   Both are symptom of a missing optional VPD EEPROM.
- "ixgbe 0000:0b:00.0: invalid short VPD tag 06 at offset 4" and a
  similar message for igb
  I didn't see any response explaining what causes this issue.
  My personal guess: Some OEM provided invalid VPD EEPROM content.
  Offset 4 is the first character of the ID string. The message
  indicates that the ID tag declares an empty ID. That would be weird.

> I think we should compute the VPD size by iterating through it looking
> only at the type (small or large) and the data item length until we
> find the End Tag.
> 

Still I didn't see any example of a rejected valid VPD image.
Not checking for supported tags increases he risk that we interpret
a random byte as tag and read beyond the VPD end, what is known to
cause a freeze on some devices.

> This code originally came from 104daa71b396 ("PCI: Determine actual
> VPD size on first access"), so I added Hannes in case there was some
> reason we do the extra validation.
> 
>> -		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
>> -		    (tag != PCI_VPD_LTIN_RO_DATA) &&
>> -		    (tag != PCI_VPD_LTIN_RW_DATA)) {
>> -			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
>> -				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
>> -				 tag, off);
>> +		if (header[0] != PCI_VPD_LRDT_ID_STRING &&
>> +		    header[0] != PCI_VPD_LRDT_RO_DATA &&
>> +		    header[0] != PCI_VPD_LRDT_RW_DATA) {
>> +			pci_warn(dev, "invalid VPD tag %02x at offset %zu", header[0], off);
>>  			return 0;
>>  		}
>> +
>> +		if (pci_read_vpd(dev, off + 1, 2, header + 1) != 2)
>> +			return 0;
>> +
>> +		off += PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(header);
>>  	}
>>  	return 0;
>>  }
>> -- 
>> 2.31.1
>>
>>