[PATCH] brcm80211: potential NULL dereference in brcmf_cfg80211_vndr_cmds_dcmd_handler()

[PATCH] brcm80211: potential NULL dereference in brcmf_cfg80211_vndr_cmds_dcmd_handler()

[Dan Carpenter]

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(-)

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;
-- 
2.18.0

[PATCH] brcm80211: potential NULL dereference in brcmf_cfg80211_vndr_cmds_dcmd_handler()

[Dan Carpenter]

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(-)

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;
-- 
2.18.0

Re: [PATCH] brcm80211: potential NULL dereference in brcmf_cfg80211_vndr_cmds_dcmd_handler()

[Kalle Valo]

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()

-- 
https://patchwork.kernel.org/patch/10914427/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] brcm80211: potential NULL dereference in brcmf_cfg80211_vndr_cmds_dcmd_handler()

[Kalle Valo]

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()

-- 
https://patchwork.kernel.org/patch/10914427/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches