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@xxxxxxxxx>
>> ---
>> 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
>>
>>
