dev/pci: fix potential panic due to bogus VPD data

A panic has been observed on a system with a Intel X520 dual LAN
device. The panic is caused by a KASSERT() noticing that the amount
of VPD data copied out to the pciconf command does not match the
amount of data read from the device.

The cause of the size mismatch was VPD data that started with 0x82,
the VPD tag that indicates that a VPD ident follows, but with a length
of more than 255 characters, which happens to be the maximum ident
size supported by the API between kernel and the pciconf program.
The data provided did not resemble an actual VPD identifier, and it
can be assumed that the initial tag value 0x82 happens to be there
by accident.

An ident size of 255 far exceeds the sensible length of that data
element, which is in the order of at most 30 to 40 bytes.

This patch adds several consitstency checks to the VPD parser, the
most critical being that ident lengths of more than 255 bytes are
rejected. Other checks reject VPD with more than one ident tag or
with an empty (zero length) ident string.

This patch prevents the panic that occured when "pciconf -lV" was
executed on the affected system.

During the anaylsis of the issue and the VPD code it has been
found that the VPD parser uses a state machine that accepts tags
in any order and combination. This is a bad match for the actual
VPD data, which has a very simple structure that can be parsed
with a non-recursive direct descent parser (which always knows
exactly which token to expect next).

A review fpr a much simpler VPD parser that performs many more
consistency checks and rejects invalid VPD has been proposed in
review https://reviews.freebsd.org/D34268.

Reported by:	mikej at paymentallianceintl.com (Michael Jung)
Approved by:	jhb
MFC after:	3 days
Differential Revision:	https://reviews.freebsd.org/D34255

diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
index ecef65477137dc4f9251f0b7b949c229e23758f2..df71076d24c9bcb5917e70e59b9bcda0ffa8aa6b 100644 (file)
--- a/sys/dev/pci/pci.c
+++ b/sys/dev/pci/pci.c
@@ -1095,6 +1095,7 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg)
        int alloc, off;         /* alloc/off for RO/W arrays */
        int cksumvalid;
        int dflen;
+       int firstrecord;
        uint8_t byte;
        uint8_t byte2;
 
@@ -1110,14 +1111,16 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg)
        alloc = off = 0;        /* shut up stupid gcc */
        dflen = 0;              /* shut up stupid gcc */
        cksumvalid = -1;
+       firstrecord = 1;
        while (state >= 0) {
                if (vpd_nextbyte(&vrs, &byte)) {
+                       pci_printf(cfg, "VPD read timed out\n");
                        state = -2;
                        break;
                }
 #if 0
-               printf("vpd: val: %#x, off: %d, bytesinval: %d, byte: %#hhx, " \
-                   "state: %d, remain: %d, name: %#x, i: %d\n", vrs.val,
+               pci_printf(cfg, "vpd: val: %#x, off: %d, bytesinval: %d, byte: "
+                   "%#hhx, state: %d, remain: %d, name: %#x, i: %d\n", vrs.val,
                    vrs.off, vrs.bytesinval, byte, state, remain, name, i);
 #endif
                switch (state) {
@@ -1138,6 +1141,15 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg)
                                remain = byte & 0x7;
                                name = (byte >> 3) & 0xf;
                        }
+                       if (firstrecord) {
+                               if (name != 0x2) {
+                                       pci_printf(cfg, "VPD data does not " \
+                                           "start with ident (%#x)\n", name);
+                                       state = -2;
+                                       break;
+                               }
+                               firstrecord = 0;
+                       }
                        if (vrs.off + remain - vrs.bytesinval > 0x8000) {
                                pci_printf(cfg,
                                    "VPD data overflow, remain %#x\n", remain);
@@ -1146,6 +1158,19 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg)
                        }
                        switch (name) {
                        case 0x2:       /* String */
+                               if (cfg->vpd.vpd_ident != NULL) {
+                                       pci_printf(cfg,
+                                           "duplicate VPD ident record\n");
+                                       state = -2;
+                                       break;
+                               }
+                               if (remain > 255) {
+                                       pci_printf(cfg,
+                                           "VPD ident length %d exceeds 255\n",
+                                           remain);
+                                       state = -2;
+                                       break;
+                               }
                                cfg->vpd.vpd_ident = malloc(remain + 1,
                                    M_DEVBUF, M_WAITOK);
                                i = 0;
@@ -1171,7 +1196,8 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg)
                                state = 5;
                                break;
                        default:        /* Invalid data, abort */
-                               state = -1;
+                               pci_printf(cfg, "invalid VPD name: %#x\n", name);
+                               state = -2;
                                break;
                        }
                        break;
@@ -1209,8 +1235,7 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg)
                                 * if this happens, we can't trust the rest
                                 * of the VPD.
                                 */
-                               pci_printf(cfg, "bad keyword length: %d\n",
-                                   dflen);
+                               pci_printf(cfg, "invalid VPD RV record");
                                cksumvalid = 0;
                                state = -1;
                                break;
@@ -1326,9 +1351,14 @@ pci_read_vpd(device_t pcib, pcicfgregs *cfg)
                        state = -1;
                        break;
                }
+
+               if (cfg->vpd.vpd_ident == NULL || cfg->vpd.vpd_ident[0] == '\0') {
+                       pci_printf(cfg, "no valid vpd ident found\n");
+                       state = -2;
+               }
        }
 
-       if (cksumvalid == 0 || state < -1) {
+       if (cksumvalid <= 0 || state < -1) {
                /* read-only data bad, clean up */
                if (cfg->vpd.vpd_ros != NULL) {
                        for (off = 0; cfg->vpd.vpd_ros[off].value; off++)

diff --git a/sys/dev/pci/pci_user.c b/sys/dev/pci/pci_user.c
index a5f849e85c2daeae82bf96c6cff191bf113bbc86..dc65b35a0b3e8dbec1a85626eabe4cf8a93d7c65 100644 (file)
--- a/sys/dev/pci/pci_user.c
+++ b/sys/dev/pci/pci_user.c
@@ -562,7 +562,7 @@ pci_list_vpd(device_t dev, struct pci_list_vpd_io *lvio)
 {
        struct pci_vpd_element vpd_element, *vpd_user;
        struct pcicfg_vpd *vpd;
-       size_t len;
+       size_t len, datalen;
        int error, i;
 
        vpd = pci_fetch_vpd_list(dev);
@@ -593,16 +593,17 @@ pci_list_vpd(device_t dev, struct pci_list_vpd_io *lvio)
         * Copyout the identifier string followed by each keyword and
         * value.
         */
+       datalen = strlen(vpd->vpd_ident);
+       KASSERT(datalen <= 255, ("invalid VPD ident length"));
        vpd_user = lvio->plvi_data;
        vpd_element.pve_keyword[0] = '\0';
        vpd_element.pve_keyword[1] = '\0';
        vpd_element.pve_flags = PVE_FLAG_IDENT;
-       vpd_element.pve_datalen = strlen(vpd->vpd_ident);
+       vpd_element.pve_datalen = datalen;
        error = copyout(&vpd_element, vpd_user, sizeof(vpd_element));
        if (error)
                return (error);
-       error = copyout(vpd->vpd_ident, vpd_user->pve_data,
-           strlen(vpd->vpd_ident));
+       error = copyout(vpd->vpd_ident, vpd_user->pve_data, datalen);
        if (error)
                return (error);
        vpd_user = PVE_NEXT_LEN(vpd_user, vpd_element.pve_datalen);