[PATCH 0/5] staging: rtl8723bs: fix multiple OOB reads in IE and frame parsing

[PATCH 0/5] staging: rtl8723bs: fix multiple OOB reads in IE and frame parsing

[Muhammad Bilal]

This series fixes five out-of-bounds read and buffer overflow bugs
in the rtl8723bs staging driver where length or offset fields from
untrusted wireless frames are used without validating that enough
bytes remain in the buffer.

Patches 1-4 are reachable remotely through crafted management frames
(beacons, probe responses, scan results). Patch 5 is reachable
locally via raw frame injection on a monitor-mode interface.

Impact:

  1/5  OOB read of IE length byte and OUI data in rtw_get_wps_ie()
  2/5  OOB read plus stack buffer overflow via attacker-controlled
       memcpy length in rtw_get_wps_attr()
  3/5  OOB read of action frame category and action bytes
  4/5  OOB read of next-IE length in the non-matching fallthrough path
  5/5  skb->len underflow after unchecked skb_pull in monitor TX path

All fixes are single-hunk bounds checks following the same pattern
already used in the sibling functions in these files.

Tested: CONFIG_RTL8723BS=m with CONFIG_KASAN=y, checkpatch clean.

Muhammad Bilal (5):
  staging: rtl8723bs: fix OOB read in rtw_get_wps_ie()
  staging: rtl8723bs: fix OOB read / stack overflow in
    rtw_get_wps_attr()
  staging: rtl8723bs: fix OOB read in rtw_action_frame_parse()
  staging: rtl8723bs: fix OOB read in rtw_restruct_wmm_ie()
  staging: rtl8723bs: fix skb->len underflow in monitor TX path

 drivers/staging/rtl8723bs/core/rtw_ieee80211.c    | 15 ++++++++++++++-
 drivers/staging/rtl8723bs/core/rtw_mlme.c         |  3 +++
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 13 ++++++++++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

-- 
2.55.0

[PATCH 1/5] staging: rtl8723bs: fix OOB read in rtw_get_wps_ie()

[Muhammad Bilal]

rtw_get_wps_ie() walks a series of information elements taken
directly from received/associated wireless management frames
(beacons, probe responses, scan results). The loop condition only
checks "cnt < in_len" before reading in_ie[cnt + 1] (the IE length
byte) and before a 4-byte memcmp() at &in_ie[cnt + 2], with no check
that those offsets are actually within in_len.

A malicious or malformed IE blob (e.g. a truncated vendor-specific
IE placed near the end of the buffer) can therefore make this
function read past the end of in_ie by up to several bytes, both in
the loop condition path and via memcpy(wps_ie, &in_ie[cnt],
in_ie[cnt + 1] + 2) when a spurious match occurs.

The sibling helpers rtw_get_sec_ie() and rtw_get_wapi_ie() in this
same file already perform the equivalent "cnt + 2 > in_len" /
"cnt + 2 + in_ie[cnt + 1] > in_len" checks, added to those two
functions by commit 1463ca3ec660 ("staging: rtl8723bs: fix OOB reads
in rtw_get_sec_ie(), rtw_get_wapi_ie(), and rtw_get_wps_attr()").
rtw_get_wps_ie() was simply never brought in line with them. Add the
same checks here, plus a length check before the 4-byte OUI memcmp.

Fixes: 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index 54f805a6b5ce..2fb5863dbeef 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -668,9 +668,15 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8 *wps_ie, uint *wps_ielen)
 	cnt = 0;
 
 	while (cnt < in_len) {
+		if (cnt + 2 > in_len)
+			break;
+		if (cnt + 2 + in_ie[cnt + 1] > in_len)
+			break;
+
 		eid = in_ie[cnt];
 
-		if ((eid == WLAN_EID_VENDOR_SPECIFIC) && (!memcmp(&in_ie[cnt + 2], wps_oui, 4))) {
+		if ((eid == WLAN_EID_VENDOR_SPECIFIC) && (in_ie[cnt + 1] >= 4) &&
+		    (!memcmp(&in_ie[cnt + 2], wps_oui, 4))) {
 			wpsie_ptr = &in_ie[cnt];
 
 			if (wps_ie)
-- 
2.55.0

[PATCH 2/5] staging: rtl8723bs: fix OOB read / stack overflow in rtw_get_wps_attr()

[Muhammad Bilal]

rtw_get_wps_attr() walks WPS attributes inside a WPS IE taken from
a wireless management frame. For each candidate attribute it only
checks that the fixed 4-byte attribute header (2-byte ID + 2-byte
length) fits inside the IE:

	if (attr_ptr + 4 > wps_ie + wps_ielen)
		break;
	u16 attr_id = get_unaligned_be16(attr_ptr);
	u16 attr_data_len = get_unaligned_be16(attr_ptr + 2);
	u16 attr_len = attr_data_len + 4;

attr_data_len (and therefore attr_len) is read directly from the
wire and is never checked against the remaining bytes in the IE
before being used as the size of:

	memcpy(buf_attr, attr_ptr, attr_len);

Since attr_len is fully attacker controlled (0 to 65535+4), this is
both a heap OOB read of wps_ie, and, more seriously, a stack buffer
overflow at several call sites where buf_attr is a single-byte
stack variable, e.g. rtw_get_wps_attr_content()'s callers passing
WPS_ATTR_SELECTED_REGISTRAR into a stack "u8 sr"/"u8
selected_registrar" (drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c,
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c). A crafted WPS IE in a
beacon or probe response processed during scanning can therefore
smash the stack of the parsing thread.

rtw_get_wps_attr_content() itself has no independent length check
and simply trusts the attr_len it gets back from rtw_get_wps_attr(),
so fixing the bound here also fixes that caller.

The "attr_ptr + 4 > wps_ie + wps_ielen" header check above was added
by commit 1463ca3ec6601 ("staging: rtl8723bs: fix OOB reads in
rtw_get_sec_ie(), rtw_get_wapi_ie(), and rtw_get_wps_attr()"), which
bounded the fixed header but never extended the check to cover the
variable-length attribute data that follows it. Add that missing
check before attr_len is used as a memcpy() length or accepted as a
match.

Fixes: 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index 2fb5863dbeef..be374d222c55 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -733,6 +733,10 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_att
 		u16 attr_data_len = get_unaligned_be16(attr_ptr + 2);
 		u16 attr_len = attr_data_len + 4;
 
+		/* Reject attributes whose claimed length runs past the IE */
+		if (attr_ptr + attr_len > wps_ie + wps_ielen)
+			break;
+
 		if (attr_id == target_attr_id) {
 			target_attr_ptr = attr_ptr;
 
-- 
2.55.0

[PATCH 3/5] staging: rtl8723bs: fix OOB read in rtw_action_frame_parse()

[Muhammad Bilal]

rtw_action_frame_parse() takes a frame_len parameter but never
actually checks it before indexing into the frame body:

	const u8 *frame_body = frame + sizeof(struct ieee80211_hdr_3addr);
	...
	c = frame_body[0];
	...
	a = frame_body[1];

frame_body already points 24 bytes (sizeof(struct
ieee80211_hdr_3addr)) into frame, so reading frame_body[0] and
frame_body[1] requires frame_len >= 26. A management action frame
shorter than that (e.g. exactly 24 bytes, the minimum a malicious
peer can send) causes a 1-2 byte out-of-bounds read.

This is reachable from rtw_cfg80211_monitor_if_xmit_entry() and
cfg80211_rtw_mgmt_tx() in ioctl_cfg80211.c, both of which pass
attacker/user-influenced frame buffers and lengths straight through.

Add the missing length check before frame_body is dereferenced.

Fixes: 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index be374d222c55..e02b54131633 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -1145,6 +1145,9 @@ int rtw_action_frame_parse(const u8 *frame, u32 frame_len, u8 *category, u8 *act
 	u8 c;
 	u8 a = ACT_PUBLIC_MAX;
 
+	if (frame_len < sizeof(struct ieee80211_hdr_3addr) + 2)
+		return false;
+
 	fc = le16_to_cpu(((struct ieee80211_hdr_3addr *)frame)->frame_control);
 
 	if ((fc & (IEEE80211_FCTL_FTYPE | IEEE80211_FCTL_STYPE)) !=
-- 
2.55.0

[PATCH 4/5] staging: rtl8723bs: fix OOB read in rtw_restruct_wmm_ie()

[Muhammad Bilal]

rtw_restruct_wmm_ie() scans in_ie for a WMM IE with:

	while (i < in_len) {
		...
		if (i + 5 < in_len && in_ie[i] == 0xDD && ...) {
			...
			break;
		}
		i += (in_ie[i + 1] + 2); /* to the next IE element */
	}

When the "i + 5 < in_len" match check fails simply because i is
within 5 bytes of the end of the buffer (i.e. no WMM IE was found
near the tail of in_ie), execution falls through to
"i += (in_ie[i + 1] + 2)", which reads in_ie[i + 1]. If i == in_len
- 1 at that point, this is a 1-byte out-of-bounds read of an
attacker-influenced IE buffer built from association/scan data.

Commit a75281626fc8f ("staging: rtl8723bs: fix potential
out-of-bounds read in rtw_restruct_wmm_ie") added the "i + 5 <
in_len" guard to the match condition itself, but did not add an
equivalent guard before the fallthrough advance, so the same class
of OOB read remained reachable through the non-matching path.

Add an explicit bounds check before advancing to the next IE.

Fixes: 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
index 1196ec011455..7bdc5fe6dc8a 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
@@ -1980,6 +1980,9 @@ int rtw_restruct_wmm_ie(struct adapter *adapter, u8 *in_ie, u8 *out_ie, uint in_
 			break;
 		}
 
+		if (i + 1 >= in_len)
+			break;
+
 		i += (in_ie[i + 1] + 2); /*  to the next IE element */
 	}
 
-- 
2.55.0

[PATCH 5/5] staging: rtl8723bs: fix skb->len underflow in monitor TX path

[Muhammad Bilal]

rtw_cfg80211_monitor_if_xmit_entry() strips a radiotap header with
skb_pull(skb, rtap_len), then immediately dereferences the 802.11
header fields (frame_control, addr1, addr2) without checking that
skb->len is still large enough to contain a struct ieee80211_hdr
(24 bytes).

Further down, it calls:

	skb_pull(skb, dot11_hdr_len + qos_len + snap_len -
			sizeof(src_mac_addr) * 2);

again with no check that skb->len covers this amount first. Plain
skb_pull() does not itself validate the requested length against
skb->len; on a too-short injected frame this makes skb->len
underflow to a huge unsigned value, after which skb->data and the
following memcpy()s operate on a corrupted skb.

This function is reachable by writing a raw frame to a monitor-mode
network device, which does not require elevated privileges beyond
being able to create/use a monitor-mode interface (CAP_NET_RAW).

Add explicit skb->len checks before dereferencing the 802.11 header
and before each skb_pull(), bailing out via the existing "fail"
error path on any mismatch.

Fixes: 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 6a97afd89dc7..eac1b6ac4c67 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -2034,10 +2034,15 @@ static netdev_tx_t rtw_cfg80211_monitor_if_xmit_entry(struct sk_buff *skb, struc
 	/* Skip the ratio tap header */
 	skb_pull(skb, rtap_len);
 
+	if (unlikely(skb->len < sizeof(struct ieee80211_hdr)))
+		goto fail;
+
 	dot11_hdr = (struct ieee80211_hdr *)skb->data;
 	frame_control = le16_to_cpu(dot11_hdr->frame_control);
 	/* Check if the QoS bit is set */
 	if ((frame_control & IEEE80211_FCTL_FTYPE) == IEEE80211_FTYPE_DATA) {
+		int pull_len;
+
 		/* Check if this ia a Wireless Distribution System (WDS) frame
 		 * which has 4 MAC addresses
 		 */
@@ -2046,13 +2051,19 @@ static netdev_tx_t rtw_cfg80211_monitor_if_xmit_entry(struct sk_buff *skb, struc
 		if ((frame_control & 0x0300) == 0x0300)
 			dot11_hdr_len += 6;
 
+		if (unlikely(skb->len < dot11_hdr_len + qos_len))
+			goto fail;
+
 		memcpy(dst_mac_addr, dot11_hdr->addr1, sizeof(dst_mac_addr));
 		memcpy(src_mac_addr, dot11_hdr->addr2, sizeof(src_mac_addr));
 
 		/* Skip the 802.11 header, QoS (if any) and SNAP, but leave spaces for
 		 * two MAC addresses
 		 */
-		skb_pull(skb, dot11_hdr_len + qos_len + snap_len - sizeof(src_mac_addr) * 2);
+		pull_len = dot11_hdr_len + qos_len + snap_len - sizeof(src_mac_addr) * 2;
+		if (unlikely(pull_len < 0 || skb->len < pull_len))
+			goto fail;
+		skb_pull(skb, pull_len);
 		pdata = (unsigned char *)skb->data;
 		memcpy(pdata, dst_mac_addr, sizeof(dst_mac_addr));
 		memcpy(pdata + sizeof(dst_mac_addr), src_mac_addr, sizeof(src_mac_addr));
-- 
2.55.0