| Message ID | 20190424095218.GB16450@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e025da3d7aa4770bb1d1b3b0aa7cc4da1744852d |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcm80211: potential NULL dereference in brcmf_cfg80211_vndr_cmds_dcmd_handler() | expand |
Dan Carpenter <dan.carpenter@oracle.com> wrote: > If "ret_len" is negative then it could lead to a NULL dereference. > > The "ret_len" value comes from nl80211_vendor_cmd(), if it's negative > then we don't allocate the "dcmd_buf" buffer. Then we pass "ret_len" to > brcmf_fil_cmd_data_set() where it is cast to a very high u32 value. > Most of the functions in that call tree check whether the buffer we pass > is NULL but there are at least a couple places which don't such as > brcmf_dbg_hex_dump() and brcmf_msgbuf_query_dcmd(). We memcpy() to and > from the buffer so it would result in a NULL dereference. > > The fix is to change the types so that "ret_len" can't be negative. (If > we memcpy() zero bytes to NULL, that's a no-op and doesn't cause an > issue). > > Fixes: 1bacb0487d0e ("brcmfmac: replace cfg80211 testmode with vendor command") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Patch applied to wireless-drivers-next.git, thanks. e025da3d7aa4 brcm80211: potential NULL dereference in brcmf_cfg80211_vndr_cmds_dcmd_handler()
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c index 8eff2753abad..d493021f6031 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c @@ -35,9 +35,10 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy, struct brcmf_if *ifp; const struct brcmf_vndr_dcmd_hdr *cmdhdr = data; struct sk_buff *reply; - int ret, payload, ret_len; + unsigned int payload, ret_len; void *dcmd_buf = NULL, *wr_pointer; u16 msglen, maxmsglen = PAGE_SIZE - 0x100; + int ret; if (len < sizeof(*cmdhdr)) { brcmf_err("vendor command too short: %d\n", len); @@ -65,7 +66,7 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy, brcmf_err("oversize return buffer %d\n", ret_len); ret_len = BRCMF_DCMD_MAXLEN; } - payload = max(ret_len, len) + 1; + payload = max_t(unsigned int, ret_len, len) + 1; dcmd_buf = vzalloc(payload); if (NULL == dcmd_buf) return -ENOMEM;
If "ret_len" is negative then it could lead to a NULL dereference. The "ret_len" value comes from nl80211_vendor_cmd(), if it's negative then we don't allocate the "dcmd_buf" buffer. Then we pass "ret_len" to brcmf_fil_cmd_data_set() where it is cast to a very high u32 value. Most of the functions in that call tree check whether the buffer we pass is NULL but there are at least a couple places which don't such as brcmf_dbg_hex_dump() and brcmf_msgbuf_query_dcmd(). We memcpy() to and from the buffer so it would result in a NULL dereference. The fix is to change the types so that "ret_len" can't be negative. (If we memcpy() zero bytes to NULL, that's a no-op and doesn't cause an issue). Fixes: 1bacb0487d0e ("brcmfmac: replace cfg80211 testmode with vendor command") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)