Skip to contentSkip to conversation

staging: rtl8723bs: fix two remote frame-handling bugs

PATCH In review Shayaun Nejad 2 participants 4 messages May 12

Subsystems (2)
Stable branch Staging
lore
Share & tools
lore

Shayaun Nejad

author
#0 lore raw

cover staging: rtl8723bs: fix two remote frame-handling bugs

Fix two rtl8723bs receive-side bugs reachable while handling remote 802.11 management frames.

The first patch fixes a use-after-free in validate_80211w_mgmt(), where decryptor() can release the receive frame and return NULL before the caller reuses cached pointers into that frame.

The second patch bounds the combined SUPP_RATES and EXT_SUPP_RATES IE lengths copied from beacon/probe response data into the 16-byte support_rate[] stack buffer in rtw_check_beacon_data().

Both issues were found by Kuzushi + deep-audit (Sonnet 4.6) and manually verified against mainline.

Shayaun Nejad (2):
staging: rtl8723bs: fix use-after-free in validate_80211w_mgmt after
decryptor()
staging: rtl8723bs: bound SUPP_RATES IE length in
rtw_check_beacon_data

 drivers/staging/rtl8723bs/core/rtw_ap.c   | 6 +++++-
 drivers/staging/rtl8723bs/core/rtw_recv.c | 9 +++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH 1/2 staging: rtl8723bs: fix use-after-free in validate_80211w_mgmt after decryptor()

decryptor() can release precv_frame and return NULL when protected management frame decryption fails.

validate_80211w_mgmt() still uses ptr and pattrib saved from that frame for two memcpy() calls before checking the returned frame pointer.

Check the returned frame before any further access, then refresh ptr and pattrib from the returned frame.

Fixes 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc [email protected]
Signed-off-by Shayaun Nejad <[email protected]>
---
 drivers/staging/rtl8723bs/core/rtw_recv.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/staging/rtl8723bs/core/rtw_recv.c +7 -2
index f78194d508..0e1d248d8f 100644
@@ -1433,6 +1433,13 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
 			if (!mgmt_DATA)
 				goto validate_80211w_fail;
 			precv_frame = decryptor(adapter, precv_frame);
+			if (!precv_frame) {
+				kfree(mgmt_DATA);
+				goto validate_80211w_fail;
+			}
+
+			pattrib = &precv_frame->u.hdr.attrib;
+			ptr = precv_frame->u.hdr.rx_data;
 			/* save actual management data frame body */
 			memcpy(mgmt_DATA, ptr + pattrib->hdrlen + pattrib->iv_len, data_len);
 			/* overwrite the iv field */

@@ -1440,8 +1447,6 @@ static signed int validate_80211w_mgmt(struct adapter *adapter, union recv_frame
 			/* remove the iv and icv length */
 			pattrib->pkt_len = pattrib->pkt_len - pattrib->iv_len - pattrib->icv_len;
 			kfree(mgmt_DATA);
-			if (!precv_frame)
-				goto validate_80211w_fail;
 		} else if (is_multicast_ether_addr(GetAddr1Ptr(ptr)) &&
 			(subtype == WIFI_DEAUTH || subtype == WIFI_DISASSOC)) {
 			signed int BIP_ret = _SUCCESS;
Signature · 1 line
2.43.0
author
#2 lore raw

PATCH 2/2 staging: rtl8723bs: bound SUPP_RATES IE length in rtw_check_beacon_data

In reply to Shayaun Nejad

rtw_check_beacon_data() copies SUPP_RATES and EXT_SUPP_RATES IE payloads into a 16-byte support_rate[] buffer.

The IE lengths are used directly, so oversized rate IEs can overflow the stack buffer.

Clamp the supported rates copy and the combined extended supported rates copy to NDIS_802_11_LENGTH_RATES_EX.

Fixes 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc [email protected]
Signed-off-by Shayaun Nejad <[email protected]>
---
 drivers/staging/rtl8723bs/core/rtw_ap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/staging/rtl8723bs/core/rtw_ap.c +5 -1
index 4b40124110..363ecb02b5 100644
@@ -873,6 +873,7 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
 		       &ie_len,
 		       (pbss_network->ie_length - _BEACON_IE_OFFSET_));
 	if (p) {
+		ie_len = min_t(uint, ie_len, NDIS_802_11_LENGTH_RATES_EX);
 		memcpy(support_rate, p + 2, ie_len);
 		support_rate_num = ie_len;
 	}

@@ -882,8 +883,11 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
 		       WLAN_EID_EXT_SUPP_RATES,
 		       &ie_len,
 		       pbss_network->ie_length - _BEACON_IE_OFFSET_);
-	if (p)
+	if (p && support_rate_num < NDIS_802_11_LENGTH_RATES_EX) {
+		ie_len = min_t(uint, ie_len,
+			       NDIS_802_11_LENGTH_RATES_EX - support_rate_num);
 		memcpy(support_rate + support_rate_num, p + 2, ie_len);
+	}
 
 	network_type = rtw_check_network_type(support_rate, channel);
Signature · 1 line
2.43.0
#3 lore raw

Re: [PATCH 2/2] staging: rtl8723bs: bound SUPP_RATES IE length in rtw_check_beacon_data

Quoted 21 lines · Shayaun Nejad

rtw_check_beacon_data() copies SUPP_RATES and EXT_SUPP_RATES IE payloads into a 16-byte support_rate[] buffer.

The IE lengths are used directly, so oversized rate IEs can overflow the stack buffer.

Clamp the supported rates copy and the combined extended supported rates copy to NDIS_802_11_LENGTH_RATES_EX.

Fixes 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc [email protected]
Signed-off-by Shayaun Nejad <[email protected]>
---
 drivers/staging/rtl8723bs/core/rtw_ap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/staging/rtl8723bs/core/rtw_ap.c +0 -0
index 4b40124110..363ecb02b5 100644

drivers/staging/rtl8723bs/core/rtw_ap.c int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf, int len) · line 876

Quoted 4 lines
drivers/staging/rtl8723bs/core/rtw_ap.c +1 -0
@@ -873,6 +873,7 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
 		       &ie_len,
 		       (pbss_network->ie_length - _BEACON_IE_OFFSET_));
 	if (p) {
+		ie_len = min_t(uint, ie_len, NDIS_802_11_LENGTH_RATES_EX);

These days we would use umin()

drivers/staging/rtl8723bs/core/rtw_ap.c int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf, int len) · line 878

Quoted 2 lines
drivers/staging/rtl8723bs/core/rtw_ap.c +0 -0
 		memcpy(support_rate, p + 2, ie_len);
 		support_rate_num = ie_len;

support_rate_num is set here. We know from the min_t() that it's less <= NDIS_802_11_LENGTH_RATES_EX.

drivers/staging/rtl8723bs/core/rtw_ap.c int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf, int len) · line 886

Quoted 7 lines
drivers/staging/rtl8723bs/core/rtw_ap.c +1 -1
 	}

@@ -882,8 +883,11 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
 		       WLAN_EID_EXT_SUPP_RATES,
 		       &ie_len,
 		       pbss_network->ie_length - _BEACON_IE_OFFSET_);
-	if (p)
+	if (p && support_rate_num < NDIS_802_11_LENGTH_RATES_EX) {

We know that support_rate_num <= NDIS_802_11_LENGTH_RATES_EX. Allowing == NDIS_802_11_LENGTH_RATES_EX is okay because memcpy() of zero bytes is a no-op.

drivers/staging/rtl8723bs/core/rtw_ap.c int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf, int len) · line 888

Quoted 2 lines
drivers/staging/rtl8723bs/core/rtw_ap.c +2 -0
+		ie_len = min_t(uint, ie_len,
+			       NDIS_802_11_LENGTH_RATES_EX - support_rate_num);

Use umin() here too.

Otherwise the patch is fine.

regards,
dan carpenter

Quoted 8 lines
drivers/staging/rtl8723bs/core/rtw_ap.c +1 -0
 		memcpy(support_rate + support_rate_num, p + 2, ie_len);
+	}
 
 	network_type = rtw_check_network_type(support_rate, channel);
Signature · 1 line
2.43.0