From nobody Sun Feb 20 21:36:26 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id AF31619D3A57; Sun, 20 Feb 2022 21:36:26 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4K1zJp4V1xz3ljL; Sun, 20 Feb 2022 21:36:26 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1645392986; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=A7k6HNOQzAwFN/JWMchXdlDNoT6Be6cT7Fqa543lyQU=; b=GPh+ZAQP3NekcThNfK0V/dO1cRjzMbj0PKLMH2HWXwDVo6NgtcG3tix6W/zWfh1F9uV+j4 ZtqczQ+OYbE/aQhJL7Jfb8+oNWX1FeJCrYr/P4SOO7M8yDjQPk6wqq1RxujdfVmnbbjB2z qiCQIvIX2KABtvYPV6oywR4Jq55IZITa13sJJNNNYtQezGJ1LQ7oH1XLMCF4UGJbzTsV0M 3gjhFxYulbSztgYiRhyFkAlhblk8f3gh3cpCO+Kwl+PMQbt+RCGS1ZM/HuNAf5wLkhVMXO F3reSj/YW5iKIhbsJbiD+am8qtSLZo5bEZQdoH+pqKp7vc9iJdSWxyQv88i2Gg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 79AD024181; Sun, 20 Feb 2022 21:36:26 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 21KLaQ60093843; Sun, 20 Feb 2022 21:36:26 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 21KLaQCq093842; Sun, 20 Feb 2022 21:36:26 GMT (envelope-from git) Date: Sun, 20 Feb 2022 21:36:26 GMT Message-Id: <202202202136.21KLaQCq093842@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: =?utf-8?Q?Stefan E=C3=9Fer?= Subject: git: f01c863337f7 - main - dev/pci: fix potential panic due to bogus VPD data List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: se X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: f01c863337f7b097d03069daee359b6b5ecd0279 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1645392986; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=A7k6HNOQzAwFN/JWMchXdlDNoT6Be6cT7Fqa543lyQU=; b=BAQzvZ79BZnTNBWOl5T56z17JoL9SG/cUqVXIffNML+Boe46mCrLuPxZpcaOfYNraNNmdQ gbGxC99BMF6NceuA8n+zJxIPjA9TiKKjye4WEw+9Wk+tF0cEzzQSOtGIABCy3uDiEaOefs LbmJrfG+86rmDQGZGY730a/cTf0C6ieN7aiPUcpVg8Ec0OY28ndbbyHGi5+opTAHI75Zot 4Pr/IAUD8MPj+HQiFkmEss5HkLgXD/8r+CWfoRoRQW/EnM5AjPEcC3Ak/sClYgnOUKjDgR k/QCqB3xNbT9hs5OI0uzWwnzDCIlU2VCZ/0ySKFLWaXG6vt5PGa7T3TrSruOow== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1645392986; a=rsa-sha256; cv=none; b=lghThNNBg1Ra+IvKeR91xbw7jRxRIXwIpoxo7r1Ssg0jzWZSHXJkEp7ziBKnRKH7RYJpeQ kBqziLWTRpB/MUshWZKPPCCKGuhKJb02eRGUnfHN6Pe0kIt2+6N86DaF5e4dNux2ljNbdq A2RUfIhgMYrdXXJZLsemaJ/XQxYA+5Hgj6HtjdOC+xA2hx1URXSjpPXPtYtyVSdeKe+INY C1Q9Vy8POSsurvtVck718JDDVffzZBGyCLQaQhzln251WJoHB2ml3uov8mEs2pPY7oGTeP tipGLdEFshWi2nD76XUUl4ycMmfbotOLVkKn13e2jZ/r9DR/1J02TMSp1wHVmg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by se: URL: https://cgit.FreeBSD.org/src/commit/?id=f01c863337f7b097d03069daee359b6b5ecd0279 commit f01c863337f7b097d03069daee359b6b5ecd0279 Author: Stefan Eßer AuthorDate: 2022-02-20 21:07:35 +0000 Commit: Stefan Eßer CommitDate: 2022-02-20 21:36:04 +0000 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 --- sys/dev/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++------ sys/dev/pci/pci_user.c | 9 +++++---- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c index ecef65477137..df71076d24c9 100644 --- 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 a5f849e85c2d..dc65b35a0b3e 100644 --- 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);