Bug 254479
| Summary: | Kernel remote heap overflow in Realtek RTL8188SU/RTL8191SU/RTL8192SU Wifi Cards USB driver | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Tommaso <cutesmilee.research> |
| Component: | wireless | Assignee: | freebsd-wireless (Nobody) <wireless> |
| Status: | Closed Not A Bug | ||
| Severity: | Affects Many People | CC: | adrian, markj |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
Was this analysis based on a crash you hit, or the output of some static analysis? It is copying the mbuf contents into a buffer of size RSU_TXBUFSZ, not of size sizeof(struct r92s_tx_desc). (In reply to Mark Johnston from comment #1) oh, haven't noticed that, this was found by static analysis, also i don't know if it can cause an issue, but i've noticed that in the same function, while other if statements that call m_freem() are locked, the first one isn't. (In reply to Tommaso from comment #2) looking at other implementation of rum_raw_xmit() in other drivers, seems like a lock should be taken, since all of the take one / make sure sc is locked. (In reply to Tommaso from comment #3) static int rsu_raw_xmit(struct ieee80211_node *ni, struct mbuf *m, const struct ieee80211_bpf_params *params) { struct ieee80211com *ic = ni->ni_ic; struct rsu_softc *sc = ic->ic_softc; struct rsu_data *bf; /* prevent management frames from being sent if we're not ready */ if (!sc->sc_running) { // no lock is taken m_freem(m); return (ENETDOWN); } RSU_LOCK(sc); // locks bf = rsu_getbuf(sc); if (bf == NULL) { m_freem(m); RSU_UNLOCK(sc); // unlocks only after the if and the free return (ENOBUFS); } if (rsu_tx_start(sc, ni, m, bf) != 0) { m_freem(m); rsu_freebuf(sc, bf); RSU_UNLOCK(sc); // same here return (EIO); } RSU_UNLOCK(sc); // unlocks if no error occurred return (0); } for example in rum driver a lock is taken: static int rum_raw_xmit(struct ieee80211_node *ni, struct mbuf *m, const struct ieee80211_bpf_params *params) { struct rum_softc *sc = ni->ni_ic->ic_softc; int ret; RUM_LOCK(sc); // lock taken before checking sc_running value /* prevent management frames from being sent if we're not ready */ if (!sc->sc_running) { ret = ENETDOWN; goto bad; } if (sc->tx_nfree < RUM_TX_MINFREE) { ret = EIO; goto bad; } if (params == NULL) { /* * Legacy path; interpret frame contents to decide * precisely how to send the frame. */ if ((ret = rum_tx_mgt(sc, m, ni)) != 0) goto bad; } else { /* * Caller supplied explicit parameters to use in * sending the frame. */ if ((ret = rum_tx_raw(sc, m, ni, params)) != 0) goto bad; } RUM_UNLOCK(sc); return 0; bad: RUM_UNLOCK(sc); m_freem(m); return ret; } Whether the lock is held doesn't really matter. At this point the driver owns the mbuf. The lock only protects driver state, so if, say, it updates a descriptor to point to the mbuf buffer, we'd want to ensure that the pointer is cleared before the mbuf is freed or handed off to other layers. But the actual m_freem() call does not require any synchronization. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=453d8a7ee2fc862f3a5e98185d57c8ad05cbc047 commit 453d8a7ee2fc862f3a5e98185d57c8ad05cbc047 Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2021-03-22 18:34:31 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2021-03-22 18:34:44 +0000 rsu: add KASSERT to document maximum mbuf size in rsu_tx_start PR: 254479 Reviewed by: markj Sponsored by: The FreeBSD Foundation sys/dev/usb/wlan/if_rsu.c | 1 + 1 file changed, 1 insertion(+) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=559fb03f96961084a754388db5f4ffdc9df92002 commit 559fb03f96961084a754388db5f4ffdc9df92002 Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2021-03-22 18:34:31 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-02-09 17:28:42 +0000 rsu: add KASSERT to document maximum mbuf size in rsu_tx_start PR: 254479 Reviewed by: markj Sponsored by: The FreeBSD Foundation (cherry picked from commit 453d8a7ee2fc862f3a5e98185d57c8ad05cbc047) sys/dev/usb/wlan/if_rsu.c | 1 + 1 file changed, 1 insertion(+) |
rsu_raw_xmit() in the last if statement calls rsu_tx_start(), taking a user-controlled mbuf as parameter. at the end of the function m_copydata() is called, and it copies the user-controlled mbuf with the length of the packet / the length of the mbuf (which isn't checked), the smaller size is taken (the user can provide a big payload), and the mbuf gets copied to the TX Descriptor struct (struct r92s_tx_desc) which is 32 bytes. these vulnerabilities are only for Realtek RTL8188SU/RTL8191SU/RTL8192SU wifi cards (that are connected via USB?). vulnerable code: static int rsu_tx_start(struct rsu_softc *sc, struct ieee80211_node *ni, struct mbuf *m0, struct rsu_data *data) { struct ieee80211vap *vap = ni->ni_vap; struct ieee80211_frame *wh; struct ieee80211_key *k = NULL; struct r92s_tx_desc *txd; uint8_t type; int prio = 0; uint8_t which; int hasqos; int xferlen; int qid; [...] xferlen = sizeof(*txd) + m0->m_pkthdr.len; m_copydata(m0, 0, m0->m_pkthdr.len, (caddr_t)&txd[1]); // <- heap overflow here data->buflen = xferlen; data->ni = ni; data->m = m0; STAILQ_INSERT_TAIL(&sc->sc_tx_pending[which], data, next); /* start transfer, if any */ usbd_transfer_start(sc->sc_xfer[which]); return (0); }