On Thu, Jul 29, 2021 at 08:10:25AM +0200, Heiner Kallweit wrote:
> On 29.07.2021 01:42, Bjorn Helgaas wrote:
> > On Fri, Jul 16, 2021 at 12:16:55AM +0200, Heiner Kallweit wrote:
> >> On 15.07.2021 23:59, Bjorn Helgaas wrote:
> >>> From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>>
> >>> A missing VPD EEPROM typically reads as either all 0xff or all zeroes.
> >>> Both cases lead to invalid VPD resource items. A 0xff tag would be a Large
> >>> Resource with length 0xffff (65535). That's invalid because VPD can only
> >>> be 32768 bytes, limited by the size of the address register in the VPD
> >>> Capability.
> >>>
> >>> A VPD that reads as all zeroes is also invalid because a 0x00 tag is a
> >>> Small Resource with length 0, which would result in an item of length 1.
> >>> This isn't explicitly illegal in PCIe r5.0, sec 6.28, but the format is
> >>> derived from PNP ISA, which *does* say "a small resource data type may be
> >>> 2-8 bytes in size" (Plug and Play ISA v1.0a, sec 6.2.2.
> >>>
> >>> Check for these invalid tags and return VPD size of zero if we find them.
> >>> If they occur at the beginning of VPD, assume it's the result of a missing
> >>> EEPROM.
> >>>
> >>> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>> ---
> >>> drivers/pci/vpd.c | 36 +++++++++++++++++++++++++++---------
> >>> 1 file changed, 27 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> >>> index 9b54dd95e42c..9c2744d79b53 100644
> >>> --- a/drivers/pci/vpd.c
> >>> +++ b/drivers/pci/vpd.c
> >>> @@ -77,11 +77,7 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >>>
> >>> 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;
> >>> - }
> >>> + size_t size;
> >>>
> >>> if (header[0] & PCI_VPD_LRDT) {
> >>> /* Large Resource Data Type Tag */
> >>> @@ -96,8 +92,16 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >>> off + 1);
> >>> return 0;
> >>> }
> >>> - off += PCI_VPD_LRDT_TAG_SIZE +
> >>> - pci_vpd_lrdt_size(header);
> >>> + 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.
> >>> + */
> >>
> >> I'm not fully convinced. Instead of checking for a "no VPD EPROM" read (00/ff)
> >> directly, we now do it indirectly based on the internal tag structure.
> >> We have pci_vpd_lrdt_size() et al to encapsulate the internal structure.
> >> IMO the code is harder to understand now.
> >
> > I don't quite follow. Previously we checked for 0x00 data
> > ("if (!header[0] && !off)"), but we didn't check directly for 0xff.
> >
> > If we read 0xff, we took the PCI_VPD_LRDT case, but it wouldn't match
> > ID_STRING, RO_DATA, or RW_DATA, so we'd fall out and check again
> > against ID_STRING, RO_DATA, and RW_DATA, and take the "invalid
> > %s VPD tag" error path because it doesn't match any.
> >
> > This results in failure for any large resource except ID_STRING,
> > RO_DATA, and RW_DATA, regardless of the size.
> >
> > My proposed code catches a different set of invalid things.
> > "size > PCI_VPD_MAX_SIZE" will catch any large resource headers with
> > length 0x8001 through 0xffff.
> >
> > Possibly it should actually check for "off + size > PCI_VPD_MAX_SIZE"
> > so e.g., it would catch a 0x20 byte resource starting at 0x7ff0.
> >
>
> For handling the 0xff case w/o additional overhead the following is pending:
> https://patchwork.kernel.org/project/linux-pci/patch/8de8c906-9284-93b9-bb44-4ffdc3470740@xxxxxxxxx/
Makes sense, I'll add that in the middle and post a v2 so you can see
what you think.
> As far as I can see the VPD structure hasn't changed since it was
> introduced in PCI 2.2. This was how many years ago?
> Instead of adding code at least my personal objective would be
> to make support for such legacy features as simple and maintainable
> as possible.
>
> >>> + 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);
> >>> @@ -105,14 +109,28 @@ static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
> >>> }
> >>> } else {
> >>> /* Short Resource Data Type Tag */
> >>> - off += PCI_VPD_SRDT_TAG_SIZE +
> >>> - pci_vpd_srdt_size(header);
> >>> tag = pci_vpd_srdt_tag(header);
> >>> + size = pci_vpd_srdt_size(header);
> >>> +
> >>> + /*
> >>> + * Missing EEPROM may read as 0x00. A small item
> >>> + * must be at least 2 bytes.
> >>> + */
> >>> + if (size == 0)
> >>> + goto error;
> >>> +
> >>> + off += PCI_VPD_SRDT_TAG_SIZE + size;
> >>> if (tag == PCI_VPD_STIN_END) /* End tag descriptor */
> >>> return off;
> >>> }
> >>> }
> >>> return 0;
> >>> +
> >>> +error:
> >>> + pci_info(dev, "invalid VPD tag %#04x at offset %zu%s\n",
> >>> + header[0], off, off == 0 ?
> >>> + "; assume missing optional EEPROM" : "");
> >>> + return 0;
> >>> }
> >>>
> >>> /*
> >>>
> >>
>
