staging: rtl8723bs: fix multiple OOB reads in IE and frame parsing

v2 of the series fixing 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.

Changes since v1:

- Dropped patch 1/5 ("fix OOB read in rtw_get_wps_ie()") - did not apply.
- Dropped patch 5/5 ("fix skb->len underflow in monitor TX path").

Muhammad Bilal (3):
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()

 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 7 +++++++
 drivers/staging/rtl8723bs/core/rtw_mlme.c      | 3 +++
 2 files changed, 10 insertions(+)

2.55.0

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.

---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 4 ++++
 1 file changed, 4 insertions(+)

index 2fb5863dbeef..be374d222c55 100644
@@ -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

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.

---
 drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 3 +++
 1 file changed, 3 insertions(+)

index be374d222c55..e02b54131633 100644
@@ -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

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.

---
 drivers/staging/rtl8723bs/core/rtw_mlme.c | 3 +++
 1 file changed, 3 insertions(+)

index 1196ec011455..7bdc5fe6dc8a 100644
@@ -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

ping.

Context-less pings are not really helpful :(

Apologies for the context-less ping, that was on me.

For reference, this is the v2 series (sent Jul 28) fixing three OOB read / stack overflow bugs in rtl8723bs where length or offset fields from untrusted wireless frames are used without validating that enough bytes remain in the buffer:

1/3 fix OOB read / stack overflow in rtw_get_wps_attr() 2/3 fix OOB read in rtw_action_frame_parse() 3/3 fix OOB read in rtw_restruct_wmm_ie()

It's been about four weeks with no review comments. Just checking whether the series is still on your radar, or if there's anything you'd like changed before it can be applied.

Thanks,
Muhammad Bilal

Please do not top-post.

It's in my queue to review, it's the middle of the merge window right now and nothing can be done until after -rc1 happens.

Also, have you tested this on real hardware?

thanks,

greg k-h

Hi,

Yes, I have tested this patch series on a real device with the RTL8723BS SDIO wireless adapter.

The device was tested connecting to access points, handling management and action frames, and verifying normal network traffic without issues or regressions.

Thanks,
Muhammad Bilal