Skip to contentSkip to conversation

staging: rtl8723bs: fix xmit_frame/xmit_buf leaks on mgnt-frame error paths

PATCH Superseded Cong Nguyen 2 participants 3 messages Jul 14 – Jul 15 Staging

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 staging: rtl8723bs: fix xmit_frame/xmit_buf leaks on mgnt-frame error paths

issue_beacon(), issue_probersp() and issue_asocrsp() obtain a management xmit_frame together with its xmit_buf from the driver's fixed-size management-TX pools via alloc_mgtxmitframe(). On the normal path the frame is handed to dump_mgntframe(), which transfers ownership and eventually returns both objects to their pools (the frame and, for beacons, the buf in rtl8723bs_mgnt_xmit(); other bufs via the pending-xmitbuf/TX-completion path).

Several error/edge paths return early after a successful alloc_mgtxmitframe() but before dump_mgntframe(), so ownership is never transferred and neither object is freed:

- issue_beacon(): beacon larger than 512 bytes
- issue_probersp(): cur_network->ie_length > MAX_IE_SZ
- issue_probersp(): kzalloc() of the SSID scratch buffer fails
- issue_asocrsp(): pkt_type is neither ASSOCRSP nor REASSOCRSP

Because alloc_mgtxmitframe() removes the frame and buf from their free lists (list_del_init) without placing them on any pending list, an orphaned pair is on no list and referenced by nobody, so it is only reclaimed at driver teardown. Repeated hits progressively exhaust the management-TX pools until alloc_mgtxmitframe() returns NULL and the interface can no longer send beacons or probe/assoc responses.

Free the frame and buffer on these paths, matching the existing correct error handling in issue_assocreq().

---
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 22 ++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c +17 -5
index 8cf3c14a0dd7..354acd7ceb6f 100644
@@ -2196,8 +2196,11 @@ void issue_beacon(struct adapter *padapter, int timeout_ms)
 
 	spin_unlock_bh(&pmlmepriv->bcn_update_lock);
 
-	if ((pattrib->pktlen + TXDESC_SIZE) > 512)
+	if ((pattrib->pktlen + TXDESC_SIZE) > 512) {
+		rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+		rtw_free_xmitframe(pxmitpriv, pmgntframe);
 		return;
+	}
 
 	pattrib->last_txcmdsz = pattrib->pktlen;
 
@@ -2258,8 +2261,11 @@ void issue_probersp(struct adapter *padapter, unsigned char *da, u8 is_valid_p2p
 	pattrib->pktlen = pattrib->hdrlen;
 	pframe += pattrib->hdrlen;
 
-	if (cur_network->ie_length > MAX_IE_SZ)
+	if (cur_network->ie_length > MAX_IE_SZ) {
+		rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+		rtw_free_xmitframe(pxmitpriv, pmgntframe);
 		return;
+	}
 
 	if ((pmlmeinfo->state & 0x03) == WIFI_FW_AP_STATE) {
 		pwps_ie = rtw_get_wps_ie(cur_network->ies + _FIXED_IE_LENGTH_,
@@ -2309,8 +2315,11 @@ void issue_probersp(struct adapter *padapter, unsigned char *da, u8 is_valid_p2p
 				sizeof(struct ieee80211_hdr_3addr);
 
 			buf = kzalloc(MAX_IE_SZ, GFP_ATOMIC);
-			if (!buf)
+			if (!buf) {
+				rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+				rtw_free_xmitframe(pxmitpriv, pmgntframe);
 				return;
+			}
 
 			ssid_ie = rtw_get_ie(ies + _FIXED_IE_LENGTH_, WLAN_EID_SSID, &ssid_ielen,
 					     (pframe - ies) - _FIXED_IE_LENGTH_);
@@ -2689,10 +2698,13 @@ void issue_asocrsp(struct adapter *padapter, unsigned short status, struct sta_i
 
 	SetSeqNum(pwlanhdr, pmlmeext->mgnt_seq);
 	pmlmeext->mgnt_seq++;
-	if ((pkt_type == WIFI_ASSOCRSP) || (pkt_type == WIFI_REASSOCRSP))
+	if ((pkt_type == WIFI_ASSOCRSP) || (pkt_type == WIFI_REASSOCRSP)) {
 		SetFrameSubType(pwlanhdr, pkt_type);
-	else
+	} else {
+		rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+		rtw_free_xmitframe(pxmitpriv, pmgntframe);
 		return;
+	}
 
 	pattrib->hdrlen = sizeof(struct ieee80211_hdr_3addr);
 	pattrib->pktlen += pattrib->hdrlen;
Signature · 1 line
2.25.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 24 lines · Cong Nguyen

issue_beacon(), issue_probersp() and issue_asocrsp() obtain a management xmit_frame together with its xmit_buf from the driver's fixed-size management-TX pools via alloc_mgtxmitframe(). On the normal path the frame is handed to dump_mgntframe(), which transfers ownership and eventually returns both objects to their pools (the frame and, for beacons, the buf in rtl8723bs_mgnt_xmit(); other bufs via the pending-xmitbuf/TX-completion path).

Several error/edge paths return early after a successful alloc_mgtxmitframe() but before dump_mgntframe(), so ownership is never transferred and neither object is freed:

- issue_beacon(): beacon larger than 512 bytes
- issue_probersp(): cur_network->ie_length > MAX_IE_SZ
- issue_probersp(): kzalloc() of the SSID scratch buffer fails
- issue_asocrsp(): pkt_type is neither ASSOCRSP nor REASSOCRSP

Because alloc_mgtxmitframe() removes the frame and buf from their free lists (list_del_init) without placing them on any pending list, an orphaned pair is on no list and referenced by nobody, so it is only reclaimed at driver teardown. Repeated hits progressively exhaust the management-TX pools until alloc_mgtxmitframe() returns NULL and the interface can no longer send beacons or probe/assoc responses.

Quoted 5 lines

Free the frame and buffer on these paths, matching the existing correct error handling in issue_assocreq().

---

Looks reasonable. Please add a Fixes tag.

regards,
dan carpenter

#2 lore raw

PATCH v2 1/1 staging: rtl8723bs: fix xmit_frame/xmit_buf leaks on mgnt-frame error paths

In reply to Cong Nguyen

issue_beacon(), issue_probersp() and issue_asocrsp() obtain a management xmit_frame together with its xmit_buf from the driver's fixed-size management-TX pools via alloc_mgtxmitframe(). On the normal path the frame is handed to dump_mgntframe(), which transfers ownership and eventually returns both objects to their pools (the frame and, for beacons, the buf in rtl8723bs_mgnt_xmit(); other bufs via the pending-xmitbuf/TX-completion path).

Several error/edge paths return early after a successful alloc_mgtxmitframe() but before dump_mgntframe(), so ownership is never transferred and neither object is freed:

- issue_beacon(): beacon larger than 512 bytes
- issue_probersp(): cur_network->ie_length > MAX_IE_SZ
- issue_probersp(): kzalloc() of the SSID scratch buffer fails
- issue_asocrsp(): pkt_type is neither ASSOCRSP nor REASSOCRSP

Because alloc_mgtxmitframe() removes the frame and buf from their free lists (list_del_init) without placing them on any pending list, an orphaned pair is on no list and referenced by nobody, so it is only reclaimed at driver teardown. Repeated hits progressively exhaust the management-TX pools until alloc_mgtxmitframe() returns NULL and the interface can no longer send beacons or probe/assoc responses.

Free the frame and buffer on these paths, matching the existing correct error handling in issue_assocreq().

Fixes 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")

---
v2:
- Add Fixes tag (Dan Carpenter)

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 22 ++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c +17 -5
index 8cf3c14a0dd7..354acd7ceb6f 100644
@@ -2196,8 +2196,11 @@ void issue_beacon(struct adapter *padapter, int timeout_ms)
 
 	spin_unlock_bh(&pmlmepriv->bcn_update_lock);
 
-	if ((pattrib->pktlen + TXDESC_SIZE) > 512)
+	if ((pattrib->pktlen + TXDESC_SIZE) > 512) {
+		rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+		rtw_free_xmitframe(pxmitpriv, pmgntframe);
 		return;
+	}
 
 	pattrib->last_txcmdsz = pattrib->pktlen;
 
@@ -2258,8 +2261,11 @@ void issue_probersp(struct adapter *padapter, unsigned char *da, u8 is_valid_p2p
 	pattrib->pktlen = pattrib->hdrlen;
 	pframe += pattrib->hdrlen;
 
-	if (cur_network->ie_length > MAX_IE_SZ)
+	if (cur_network->ie_length > MAX_IE_SZ) {
+		rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+		rtw_free_xmitframe(pxmitpriv, pmgntframe);
 		return;
+	}
 
 	if ((pmlmeinfo->state & 0x03) == WIFI_FW_AP_STATE) {
 		pwps_ie = rtw_get_wps_ie(cur_network->ies + _FIXED_IE_LENGTH_,
@@ -2309,8 +2315,11 @@ void issue_probersp(struct adapter *padapter, unsigned char *da, u8 is_valid_p2p
 				sizeof(struct ieee80211_hdr_3addr);
 
 			buf = kzalloc(MAX_IE_SZ, GFP_ATOMIC);
-			if (!buf)
+			if (!buf) {
+				rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+				rtw_free_xmitframe(pxmitpriv, pmgntframe);
 				return;
+			}
 
 			ssid_ie = rtw_get_ie(ies + _FIXED_IE_LENGTH_, WLAN_EID_SSID, &ssid_ielen,
 					     (pframe - ies) - _FIXED_IE_LENGTH_);
@@ -2689,10 +2698,13 @@ void issue_asocrsp(struct adapter *padapter, unsigned short status, struct sta_i
 
 	SetSeqNum(pwlanhdr, pmlmeext->mgnt_seq);
 	pmlmeext->mgnt_seq++;
-	if ((pkt_type == WIFI_ASSOCRSP) || (pkt_type == WIFI_REASSOCRSP))
+	if ((pkt_type == WIFI_ASSOCRSP) || (pkt_type == WIFI_REASSOCRSP)) {
 		SetFrameSubType(pwlanhdr, pkt_type);
-	else
+	} else {
+		rtw_free_xmitbuf(pxmitpriv, pmgntframe->pxmitbuf);
+		rtw_free_xmitframe(pxmitpriv, pmgntframe);
 		return;
+	}
 
 	pattrib->hdrlen = sizeof(struct ieee80211_hdr_3addr);
 	pattrib->pktlen += pattrib->hdrlen;
Signature · 1 line
2.25.1