On 14.07.2021 17:50, Bjorn Helgaas wrote:
> On Tue, Jul 13, 2021 at 11:13:31PM +0200, Heiner Kallweit wrote:
>> 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.
>
> Yes. I guess my point is that I think we should make the minimum
> assumptions required by the spec, and we shouldn't make things fail
> because of minor spec violations that don't get in the way of what
> we're trying to accomplish. Sort of the "be liberal in what you
> accept" idea.
>
> In this case, we're trying to determine the VPD size. We don't need
> to know what data types are in the VPD; we only need to know the size
> of each data item, which is determined by the small/large bit and the
> data item length. The "name" (ID_STRING, RO_DATA, RW_DATA) isn't
> involved at all except that we need to recognize the END tag.
>
> The value of rejecting unrecognized data types is a bit fuzzy. Yes,
> it may keep us from reading beyond the VPD end. But it's no
> guarantee; we can still read beyond the end if the VPD is
> ill-structured (missing END tag, too-large item length, etc).
>
> The kernel itself doesn't depend on any data from VPD, so I don't
> think it should be in the business of validating that VPD only uses
> the approved data item types.
>
>> 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.
>
> If we read a tag that is inconsistent, we *have* to stop because we
> can no longer compute the size. Neither of the trivial cases of a
> missing VPD EEPROM looking like all zeroes or all 0xff can be
> interpreted as valid tags, so it's easy to discard those cases.
>
> Incidentally, I think we may be able to improve our diagnostic
> messages a bit. The "Invalid VPD tag 00, assume missing optional VPD
> EPROM" message is good, but the 0xff case isn't quite as clear.
>
Right, we should handle 0xff together with 0x00. I'll submit a patch
for it.
>> - "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 the 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
>>>>
>>>>
>>
