Prevent panic due to invalid PCI VPD data

[se]

Summary

Michael Jung <mikej at paymentallianceintl.com> reported a kernel panic when fetching VPD date from an Intel X520 10 Gbit/s LAN adapter. The command used was "pciconf -lV" and it can be executed by an unprivileged user.

The cause of this is panic is that bogus VPD data is read from this device, which is not detected as invalid by the current consistency checks in pci_read_vpd().

The panic was caused by an "ident string" of more than 255 bytes (actually 0x6200) whose length was stored into a byte size struct element (vpe_datalen) in pci_list_vpd(). Since elements of more than 255 bytes cannot be copied out (and the standard asks for ident strings of at most 32 bytes), it seems appropriate to reject the VPD data if the length found exceeds 255 bytes.

This review adds further tests that should help detect invalid VPD data. I prefer to delete the bogus data instead of returning them and have therefore made more tests return a state of -2 (and have modified the condition for the checksum check to also reject the VPD if there was no RV record - it is mandatory according to the standard).

I want to offer those stricter checks and return values for review, they are not strictly required, the panic can be prevented by rejecting length > 256 alone. I had added them before the actual cause of the panic was understood. But I think these extra checks and more strict rejection of invalid VPD data gives higher confidence in data that passes all these checks.

Test Plan

Apply patch and verify that valid VPD data can still be obtained and that "pciconf -lV" does no longer panic the affected system with a dual X520 LAN adapter.

[imp]

This seems OK, but I'm not familiar enough with the underlying problem to know if it solves it.
So please seek other approval. I'll add jhb as well, since he's done a lot with this code.

sys/dev/pci/pci.c

1112

Do we need these? I know it may be a bit beyond the scope, but it's time to ask this :)

1121–1125

Do we still need this?

[se]

Analysis of the issue took longer (more iterations of data sampling with slighty modified trace printfs inserted) than I had expected.
In the end, everything done in pci_read_vpd() was consistent, but the assumption that the function that had called pci_read_vpd() could deal with the data returned was false.
The easiest and best solution that prevents the panic is to only return data to the caller that can be processed, there.
And that can easily be achieved by limiting the length of the value passed as ident to at most 255 bytes.

The other values have a length limited by dflen and thus always <= 255 since only a single VPD byte is used to set that variable.
The "keyword" consists of 2 bytes that can be expected to be upper case ASCII characters, but AFAIK this is not strictly required, just common practice.
Therefore I'd think that testing the keywords or associated values for being valid ASCII chars might be too strict.

I have a few more diagnostic messages for unexpected cases, which would have simplified the analysis of this issue, had then already been there.

sys/dev/pci/pci.c

1112

Without having tested the need of these initializers, they are most probably required since only very complex flow analysis will be able to verify that they are indeed never accessed without prior assignment.
The reason is that the state machine partially enforces an order of execution of the different switch(state) alternatives. E.g. states 0, then 1, then 0, then 2, then optionally 3, then 0 again, ...
State 0 -> 2 (for name==0x10) will set alloc=8 and off=0 together with state=2, but the case for state==2 is parallel to state==0 from the PoV of the compiler and only analysis that the initial path to state 2 always goes through the code that assigns to off and alloc is done by static code analysis tools like Fortify, but I'm not sure whether the optimization pass in LLVM or GCC will simulate execution of all possible paths through all possible sequences of state machine states.

1121–1125

This was the essential tool to see a trace of the VPD data being fetched and to exclude many other potential issues with the data.
I replaced the printf with a pci_printf to see the device name in the log, since there is no other information in the log that allows to identify the device being read.

[jhb]

Generally looks good to me.

sys/dev/pci/pci.c

1144

I'm not sure if the spec actually requires the ident to be first?

1161

These checks seem sensible, and if I understand correctly, the second one fixes the reported panic?

[se]

sys/dev/pci/pci.c

1144

Yes, the order of tags is 0x82 (large tag 0x02), then 0x90 (optional, large tag 0x10), 0x91 (large tag 0x11), and finally 0x78 (small tag 0x0f with no data).

Appendix I of the PCI-3.0 standard specifies the format thus:

The Identifier String (02h) tag is the first VPD tag and provides the product name of the device. One VPD-R (10h) tag is used as a header for the read-only keywords, and one VPD- W (11h) tag is used as a header for the read-write keywords. The VPD-R list (including tag and length) must checksum to zero. The storage component containing the read/write data is a non-volatile device that will retain the data when powered off. Attempts to write the read-only data will be executed as a no-op. The last tag must be the End Tag (0Fh).

Further details are in chapter I.3.1.:

Large resource type Identifier String Tag (02h)
This tag is the first item in the VPD storage component. It contains the name of the add-in card in alphanumeric characters.

Large resource type VPD-R Tag (010h)
This tag contains the read only VPD keywords for an add-in card.

Large resource type VPD-W Tag (011h)
This tag contains the read/write VPD keywords for an add-in card.

Small resource type End Tag (0Fh)
This tag identifies the end of VPD in the storage component.

(There was a different format pre PCI-2.2, i.e. some 25 years ago. PCI-2.2 defined VPD already identical to 3.0, only the example data provided contains an error which caused the current code to panic the kernel, when I tested it using the example data - see the DUMMY data in review D34268.)

The RW tag is used to mark writable space up to the end marker, the read-only data follows the ident and precedes the (optional) writeable data, which will extend throughout the writable space up to the end tag. If there is no writable data, the RV tag (checksum) may cover the unused rest of the flash memory - only its first byte is considered for the checksum, the rest of its data is to be ignored. The RV element is mandatory (not checked by the committed code). It must come last in the read-only section and ends the checksummed data. Writable data has no checksum and must come after all read-only data.

But I have meanwhile created https://reviews.freebsd.org/D34268 to implement parsing this sequence of tags and elements in a much more straight forward way, without the need for a complex state machine. Since there are so many other violations of the specified structure that are not detected by the way too flexible/lenient state machine, and adding checks for them would make it even more complex, I'd rather replace it with code that is easy to follow and to compare with the specification.

Therefore (i.e., since so many other non-conformancies are not detected, anyway) I'd like to reduce this patch to the bare minimum, the limit of 255 significant characters in the ident. This is way longer than required, and it prevents the panic that was the reason to look at this code.

1161

Yes - the length value will be stored in an uint8_t in pci_user.c, limiting the valid range to 0..255.
And 255 is way longer than required: The ident is meant to be displayed to the user in vendor specific tools and I have seen advice to limit it to 32 characters (and to drop any excess characters if there is limited space to display the ident). The example in the PCI standard uses a 33 character ident, though.