Date: Wed, 21 Jun 2023 18:35:29 GMT From: =?utf-8?Q?Stefan=20E=C3=9Fer?= <se@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 586164cc0951 - main - dev/pci: simplify PCI VPD access functions Message-ID: <202306211835.35LIZTja036613@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by se: URL: https://cgit.FreeBSD.org/src/commit/?id=586164cc095105f82ffd87997c3aa78f5e21a90c commit 586164cc095105f82ffd87997c3aa78f5e21a90c Author: Stefan Eßer <se@FreeBSD.org> AuthorDate: 2023-06-21 17:36:39 +0000 Commit: Stefan Eßer <se@FreeBSD.org> CommitDate: 2023-06-21 17:36:39 +0000 dev/pci: simplify PCI VPD access functions This update contains a rewrite of the VPD parser based on the definition of the structure of the VPD data (ident, R/O resource data, optional R/W data, end tag). The parser it replaces was based on a state machine, with the tags and the parsed data controlling the state changes. The flexibility of this parser is actually not required, and it has caused kernel panics when operating on malformed data. Analysis of the VPD code to make it more robust lead me to believe that it was easier to write a "strict" parser than to restrict the flexible state machine to detect and reject non-well-formed data. A number of restrictions had already been added, but they make the state machine ever more complex and harder to understand. This updated parser has been verified to return identical parsed data as the current implementation for the example VPD data given in the PCI standard and in some actual PCIe VPD data. It is strict in the sense that it detects and rejects any deviation from a well-formed VPD structure. PR: 272018 Approved by: kib MFC after: 4 weeks Differential Revision: https://reviews.freebsd.org/D34268 --- sys/dev/pci/pci.c | 551 ++++++++++++++++++++++++++---------------------------- 1 file changed, 263 insertions(+), 288 deletions(-) diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c index 24348ecf14eb..cbdfb3acec70 100644 --- a/sys/dev/pci/pci.c +++ b/sys/dev/pci/pci.c @@ -1063,6 +1063,7 @@ struct vpd_readstate { uint8_t cksum; }; +/* return 0 and one byte in *data if no read error, -1 else */ static int vpd_nextbyte(struct vpd_readstate *vrs, uint8_t *data) { @@ -1071,7 +1072,7 @@ vpd_nextbyte(struct vpd_readstate *vrs, uint8_t *data) if (vrs->bytesinval == 0) { if (pci_read_vpd_reg(vrs->pcib, vrs->cfg, vrs->off, ®)) - return (ENXIO); + return (-1); vrs->val = le32toh(reg); vrs->off += 4; byte = vrs->val & 0xff; @@ -1087,303 +1088,284 @@ vpd_nextbyte(struct vpd_readstate *vrs, uint8_t *data) return (0); } +/* return 0 on match, -1 and "unget" byte on no match */ +static int +vpd_expectbyte(struct vpd_readstate *vrs, uint8_t expected) +{ + uint8_t data; + + if (vpd_nextbyte(vrs, &data) != 0) + return (-1); + + if (data == expected) + return (0); + + vrs->cksum -= data; + vrs->val = (vrs->val << 8) + data; + vrs->bytesinval++; + return (-1); +} + +/* return size if tag matches, -1 on no match, -2 on read error */ +static int +vpd_read_tag_size(struct vpd_readstate *vrs, uint8_t vpd_tag) +{ + uint8_t byte1, byte2; + + if (vpd_expectbyte(vrs, vpd_tag) != 0) + return (-1); + + if ((vpd_tag & 0x80) == 0) + return (vpd_tag & 0x07); + + if (vpd_nextbyte(vrs, &byte1) != 0) + return (-2); + if (vpd_nextbyte(vrs, &byte2) != 0) + return (-2); + + return ((byte2 << 8) + byte1); +} + +/* (re)allocate buffer in multiples of 8 elements */ +static void* +alloc_buffer(void* buffer, size_t element_size, int needed) +{ + int alloc, new_alloc; + + alloc = roundup2(needed, 8); + new_alloc = roundup2(needed + 1, 8); + if (alloc != new_alloc) { + buffer = reallocf(buffer, + new_alloc * element_size, M_DEVBUF, M_WAITOK | M_ZERO); + } + + return (buffer); +} + +/* read VPD keyword and return element size, return -1 on read error */ +static int +vpd_read_elem_head(struct vpd_readstate *vrs, char keyword[2]) +{ + uint8_t data; + + if (vpd_nextbyte(vrs, &keyword[0]) != 0) + return (-1); + if (vpd_nextbyte(vrs, &keyword[1]) != 0) + return (-1); + if (vpd_nextbyte(vrs, &data) != 0) + return (-1); + + return (data); +} + +/* read VPD data element of given size into allocated buffer */ +static char * +vpd_read_value(struct vpd_readstate *vrs, int size) +{ + int i; + char char1; + char *value; + + value = malloc(size + 1, M_DEVBUF, M_WAITOK); + for (i = 0; i < size; i++) { + if (vpd_nextbyte(vrs, &char1) != 0) { + free(value, M_DEVBUF); + return (NULL); + } + value[i] = char1; + } + value[size] = '\0'; + + return (value); +} + +/* read VPD into *keyword and *value, return length of data element */ +static int +vpd_read_elem_data(struct vpd_readstate *vrs, char keyword[2], char **value, int maxlen) +{ + int len; + + len = vpd_read_elem_head(vrs, keyword); + if (len > maxlen) + return (-1); + *value = vpd_read_value(vrs, len); + + return (len); +} + +/* subtract all data following first byte from checksum of RV element */ static void -pci_read_vpd(device_t pcib, pcicfgregs *cfg) +vpd_fixup_cksum(struct vpd_readstate *vrs, char *rvstring, int len) { - struct vpd_readstate vrs; - int state; - int name; - int remain; int i; - int alloc, off; /* alloc/off for RO/W arrays */ - int cksumvalid; - int dflen; - int firstrecord; - uint8_t byte; - uint8_t byte2; + uint8_t fixup; - /* init vpd reader */ - vrs.bytesinval = 0; - vrs.off = 0; - vrs.pcib = pcib; - vrs.cfg = cfg; - vrs.cksum = 0; + fixup = 0; + for (i = 1; i < len; i++) + fixup += rvstring[i]; + vrs->cksum -= fixup; +} - state = 0; - name = remain = i = 0; /* shut up stupid gcc */ - 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; +/* fetch one read-only element and return size of heading + data */ +static size_t +next_vpd_ro_elem(struct vpd_readstate *vrs, int maxsize) +{ + struct pcicfg_vpd *vpd; + pcicfgregs *cfg; + struct vpd_readonly *vpd_ros; + int len; + + cfg = vrs->cfg; + vpd = &cfg->vpd; + + if (maxsize < 3) + return (-1); + vpd->vpd_ros = alloc_buffer(vpd->vpd_ros, sizeof(*vpd->vpd_ros), vpd->vpd_rocnt); + vpd_ros = &vpd->vpd_ros[vpd->vpd_rocnt]; + maxsize -= 3; + len = vpd_read_elem_data(vrs, vpd_ros->keyword, &vpd_ros->value, maxsize); + if (vpd_ros->value == NULL) + return (-1); + vpd_ros->len = len; + if (vpd_ros->keyword[0] == 'R' && vpd_ros->keyword[1] == 'V') { + vpd_fixup_cksum(vrs, vpd_ros->value, len); + if (vrs->cksum != 0) { + pci_printf(cfg, + "invalid VPD checksum %#hhx\n", vrs->cksum); + return (-1); } -#if 0 - 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) { - case 0: /* item name */ - if (byte & 0x80) { - if (vpd_nextbyte(&vrs, &byte2)) { - state = -2; - break; - } - remain = byte2; - if (vpd_nextbyte(&vrs, &byte2)) { - state = -2; - break; - } - remain |= byte2 << 8; - name = byte & 0x7f; - } else { - 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); - state = -1; - break; - } - 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; - state = 1; - break; - case 0xf: /* End */ - state = -1; - break; - case 0x10: /* VPD-R */ - alloc = 8; - off = 0; - cfg->vpd.vpd_ros = malloc(alloc * - sizeof(*cfg->vpd.vpd_ros), M_DEVBUF, - M_WAITOK | M_ZERO); - state = 2; - break; - case 0x11: /* VPD-W */ - alloc = 8; - off = 0; - cfg->vpd.vpd_w = malloc(alloc * - sizeof(*cfg->vpd.vpd_w), M_DEVBUF, - M_WAITOK | M_ZERO); - state = 5; - break; - default: /* Invalid data, abort */ - pci_printf(cfg, "invalid VPD name: %#x\n", name); - state = -2; - break; - } - break; + } + vpd->vpd_rocnt++; - case 1: /* Identifier String */ - cfg->vpd.vpd_ident[i++] = byte; - remain--; - if (remain == 0) { - cfg->vpd.vpd_ident[i] = '\0'; - state = 0; - } - break; + return (len + 3); +} - case 2: /* VPD-R Keyword Header */ - if (off == alloc) { - cfg->vpd.vpd_ros = reallocf(cfg->vpd.vpd_ros, - (alloc *= 2) * sizeof(*cfg->vpd.vpd_ros), - M_DEVBUF, M_WAITOK | M_ZERO); - } - cfg->vpd.vpd_ros[off].keyword[0] = byte; - if (vpd_nextbyte(&vrs, &byte2)) { - state = -2; - break; - } - cfg->vpd.vpd_ros[off].keyword[1] = byte2; - if (vpd_nextbyte(&vrs, &byte2)) { - state = -2; - break; - } - cfg->vpd.vpd_ros[off].len = dflen = byte2; - if (dflen == 0 && - strncmp(cfg->vpd.vpd_ros[off].keyword, "RV", - 2) == 0) { - /* - * if this happens, we can't trust the rest - * of the VPD. - */ - pci_printf(cfg, "invalid VPD RV record"); - cksumvalid = 0; - state = -1; - break; - } else if (dflen == 0) { - cfg->vpd.vpd_ros[off].value = malloc(1 * - sizeof(*cfg->vpd.vpd_ros[off].value), - M_DEVBUF, M_WAITOK); - cfg->vpd.vpd_ros[off].value[0] = '\x00'; - } else - cfg->vpd.vpd_ros[off].value = malloc( - (dflen + 1) * - sizeof(*cfg->vpd.vpd_ros[off].value), - M_DEVBUF, M_WAITOK); - remain -= 3; - i = 0; - /* keep in sync w/ state 3's transitions */ - if (dflen == 0 && remain == 0) - state = 0; - else if (dflen == 0) - state = 2; - else - state = 3; - break; +/* fetch one writable element and return size of heading + data */ +static size_t +next_vpd_rw_elem(struct vpd_readstate *vrs, int maxsize) +{ + struct pcicfg_vpd *vpd; + pcicfgregs *cfg; + struct vpd_write *vpd_w; + int len; - case 3: /* VPD-R Keyword Value */ - cfg->vpd.vpd_ros[off].value[i++] = byte; - if (strncmp(cfg->vpd.vpd_ros[off].keyword, - "RV", 2) == 0 && cksumvalid == -1) { - if (vrs.cksum == 0) - cksumvalid = 1; - else { - if (bootverbose) - pci_printf(cfg, - "bad VPD cksum, remain %hhu\n", - vrs.cksum); - cksumvalid = 0; - state = -1; - break; - } - } - dflen--; - remain--; - /* keep in sync w/ state 2's transitions */ - if (dflen == 0) - cfg->vpd.vpd_ros[off++].value[i++] = '\0'; - if (dflen == 0 && remain == 0) { - cfg->vpd.vpd_rocnt = off; - cfg->vpd.vpd_ros = reallocf(cfg->vpd.vpd_ros, - off * sizeof(*cfg->vpd.vpd_ros), - M_DEVBUF, M_WAITOK | M_ZERO); - state = 0; - } else if (dflen == 0) - state = 2; - break; + cfg = vrs->cfg; + vpd = &cfg->vpd; - case 4: - remain--; - if (remain == 0) - state = 0; - break; + if (maxsize < 3) + return (-1); + vpd->vpd_w = alloc_buffer(vpd->vpd_w, sizeof(*vpd->vpd_w), vpd->vpd_wcnt); + if (vpd->vpd_w == NULL) { + pci_printf(cfg, "out of memory"); + return (-1); + } + vpd_w = &vpd->vpd_w[vpd->vpd_wcnt]; + maxsize -= 3; + vpd_w->start = vrs->off + 3 - vrs->bytesinval; + len = vpd_read_elem_data(vrs, vpd_w->keyword, &vpd_w->value, maxsize); + if (vpd_w->value == NULL) + return (-1); + vpd_w->len = len; + vpd->vpd_wcnt++; - case 5: /* VPD-W Keyword Header */ - if (off == alloc) { - cfg->vpd.vpd_w = reallocf(cfg->vpd.vpd_w, - (alloc *= 2) * sizeof(*cfg->vpd.vpd_w), - M_DEVBUF, M_WAITOK | M_ZERO); - } - cfg->vpd.vpd_w[off].keyword[0] = byte; - if (vpd_nextbyte(&vrs, &byte2)) { - state = -2; - break; - } - cfg->vpd.vpd_w[off].keyword[1] = byte2; - if (vpd_nextbyte(&vrs, &byte2)) { - state = -2; - break; - } - cfg->vpd.vpd_w[off].len = dflen = byte2; - cfg->vpd.vpd_w[off].start = vrs.off - vrs.bytesinval; - cfg->vpd.vpd_w[off].value = malloc((dflen + 1) * - sizeof(*cfg->vpd.vpd_w[off].value), - M_DEVBUF, M_WAITOK); - remain -= 3; - i = 0; - /* keep in sync w/ state 6's transitions */ - if (dflen == 0 && remain == 0) - state = 0; - else if (dflen == 0) - state = 5; - else - state = 6; - break; + return (len + 3); +} - case 6: /* VPD-W Keyword Value */ - cfg->vpd.vpd_w[off].value[i++] = byte; - dflen--; - remain--; - /* keep in sync w/ state 5's transitions */ - if (dflen == 0) - cfg->vpd.vpd_w[off++].value[i++] = '\0'; - if (dflen == 0 && remain == 0) { - cfg->vpd.vpd_wcnt = off; - cfg->vpd.vpd_w = reallocf(cfg->vpd.vpd_w, - off * sizeof(*cfg->vpd.vpd_w), - M_DEVBUF, M_WAITOK | M_ZERO); - state = 0; - } else if (dflen == 0) - state = 5; - break; +/* free all memory allocated for VPD data */ +static void +vpd_free(struct pcicfg_vpd *vpd) +{ + int i; - default: - pci_printf(cfg, "invalid state: %d\n", state); - state = -1; - break; - } + free(vpd->vpd_ident, M_DEVBUF); + for (i = 0; i < vpd->vpd_rocnt; i++) + free(vpd->vpd_ros[i].value, M_DEVBUF); + free(vpd->vpd_ros, M_DEVBUF); + vpd->vpd_rocnt = 0; + for (i = 0; i < vpd->vpd_wcnt; i++) + free(vpd->vpd_w[i].value, M_DEVBUF); + free(vpd->vpd_w, M_DEVBUF); + vpd->vpd_wcnt = 0; +} - if (cfg->vpd.vpd_ident == NULL || cfg->vpd.vpd_ident[0] == '\0') { - pci_printf(cfg, "no valid vpd ident found\n"); - state = -2; - } +#define VPD_TAG_END ((0x0f << 3) | 0) /* small tag, len == 0 */ +#define VPD_TAG_IDENT (0x02 | 0x80) /* large tag */ +#define VPD_TAG_RO (0x10 | 0x80) /* large tag */ +#define VPD_TAG_RW (0x11 | 0x80) /* large tag */ + +static int +pci_parse_vpd(device_t pcib, pcicfgregs *cfg) +{ + struct vpd_readstate vrs; + int cksumvalid; + int size, elem_size; + + /* init vpd reader */ + vrs.bytesinval = 0; + vrs.off = 0; + vrs.pcib = pcib; + vrs.cfg = cfg; + vrs.cksum = 0; + + /* read VPD ident element - mandatory */ + size = vpd_read_tag_size(&vrs, VPD_TAG_IDENT); + if (size <= 0) { + pci_printf(cfg, "no VPD ident found\n"); + return (0); + } + cfg->vpd.vpd_ident = vpd_read_value(&vrs, size); + if (cfg->vpd.vpd_ident == NULL) { + pci_printf(cfg, "error accessing VPD ident data\n"); + return (0); } - 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++) - free(cfg->vpd.vpd_ros[off].value, M_DEVBUF); - free(cfg->vpd.vpd_ros, M_DEVBUF); - cfg->vpd.vpd_ros = NULL; - } + /* read VPD RO elements - mandatory */ + size = vpd_read_tag_size(&vrs, VPD_TAG_RO); + if (size <= 0) { + pci_printf(cfg, "no read-only VPD data found\n"); + return (0); } - if (state < -1) { - /* I/O error, clean up */ - pci_printf(cfg, "failed to read VPD data.\n"); - if (cfg->vpd.vpd_ident != NULL) { - free(cfg->vpd.vpd_ident, M_DEVBUF); - cfg->vpd.vpd_ident = NULL; + while (size > 0) { + elem_size = next_vpd_ro_elem(&vrs, size); + if (elem_size < 0) { + pci_printf(cfg, "error accessing read-only VPD data\n"); + return (-1); } - if (cfg->vpd.vpd_w != NULL) { - for (off = 0; cfg->vpd.vpd_w[off].value; off++) - free(cfg->vpd.vpd_w[off].value, M_DEVBUF); - free(cfg->vpd.vpd_w, M_DEVBUF); - cfg->vpd.vpd_w = NULL; + size -= elem_size; + } + cksumvalid = (vrs.cksum == 0); + if (!cksumvalid) + return (-1); + + /* read VPD RW elements - optional */ + size = vpd_read_tag_size(&vrs, VPD_TAG_RW); + if (size == -2) + return (-1); + while (size > 0) { + elem_size = next_vpd_rw_elem(&vrs, size); + if (elem_size < 0) { + pci_printf(cfg, "error accessing writeable VPD data\n"); + return (-1); } + size -= elem_size; } + + /* read empty END tag - mandatory */ + size = vpd_read_tag_size(&vrs, VPD_TAG_END); + if (size != 0) { + pci_printf(cfg, "No valid VPD end tag found\n"); + } + return (0); +} + +static void +pci_read_vpd(device_t pcib, pcicfgregs *cfg) +{ + int status; + + status = pci_parse_vpd(pcib, cfg); + if (status < 0) + vpd_free(&cfg->vpd); cfg->vpd.vpd_cached = 1; #undef REG #undef WREG @@ -2777,19 +2759,12 @@ pci_freecfg(struct pci_devinfo *dinfo) { struct devlist *devlist_head; struct pci_map *pm, *next; - int i; devlist_head = &pci_devq; - if (dinfo->cfg.vpd.vpd_reg) { - free(dinfo->cfg.vpd.vpd_ident, M_DEVBUF); - for (i = 0; i < dinfo->cfg.vpd.vpd_rocnt; i++) - free(dinfo->cfg.vpd.vpd_ros[i].value, M_DEVBUF); - free(dinfo->cfg.vpd.vpd_ros, M_DEVBUF); - for (i = 0; i < dinfo->cfg.vpd.vpd_wcnt; i++) - free(dinfo->cfg.vpd.vpd_w[i].value, M_DEVBUF); - free(dinfo->cfg.vpd.vpd_w, M_DEVBUF); - } + if (dinfo->cfg.vpd.vpd_reg) + vpd_free(&dinfo->cfg.vpd); + STAILQ_FOREACH_SAFE(pm, &dinfo->cfg.maps, pm_link, next) { free(pm, M_DEVBUF); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202306211835.35LIZTja036613>