[PATCH v4 2/3] staging: rtl8723bs: fix OOB reads in IE loops in issue_assocreq() and join_cmd_hdl()

[Alexandru Hossu <hossu.alexandru@gmail.com>]

Five out-of-bounds read paths in the IE parsing loops of
issue_assocreq() and join_cmd_hdl():

1. Missing IE header bounds checks (both functions).

   Both loops advance by pIE->length + 2 per iteration but only guard
   on i < ie_length.  When the buffer ends with a single element_id
   byte and no length byte, the loop reads pIE->length from one byte
   past the end of the buffer.  Even when both header bytes are in
   bounds, pIE->length can extend the data window past ie_length,
   silently passing a truncated IE to handler functions.  Add two
   guards at the top of each loop: break if fewer than sizeof(*pIE)
   bytes remain, and break if the declared IE payload extends past
   ie_length.

2. Vendor-specific OUI comparison reads 4 bytes past a possibly short
   IE payload (both functions).

   For WLAN_EID_VENDOR_SPECIFIC, the code calls memcmp(pIE->data,
   OUI, 4) on RTW_WPA_OUI, WMM_OUI, and WPS_OUI without first
   verifying that pIE->length is at least 4.  A short IE at the end
   of the frame causes the memcmp to read into adjacent frame data.
   Add pIE->length >= 4 guard before the comparisons.

3. HT Capability IE memcpy reads sizeof(struct HT_caps_element) bytes
   from an IE that may be shorter (issue_assocreq only).

   The WLAN_EID_HT_CAPABILITY handler copies:

     memcpy(&pmlmeinfo->HT_caps, pIE->data, sizeof(struct HT_caps_element));

   If pIE->length < sizeof(struct HT_caps_element), the memcpy reads
   beyond the end of the IE payload into adjacent frame data.  Add a
   minimum length check and skip the IE if it is too short.

4. rtw_set_ie called with untrusted pIE->length for HT Capability
   (issue_assocreq only).

   After the memcpy the code passes pIE->length directly to
   rtw_set_ie() as the IE body length.  If pIE->length exceeds
   sizeof(struct HT_caps_element), rtw_set_ie copies that many bytes
   from pmlmeinfo->HT_caps, reading past the end of the struct into
   adjacent fields.  Use sizeof(struct HT_caps_element) instead.

5. HT Operation IE accessed without minimum length check (join_cmd_hdl
   only).

   The WLAN_EID_HT_OPERATION handler casts pIE->data to
   struct HT_info_element * and reads pht_info->infos[0] (offset 1)
   without verifying pIE->length >= sizeof(struct HT_info_element).
   A zero- or one-byte HT Operation IE causes an out-of-bounds read.
   Add a minimum length check and break if the IE is too short.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
Changes in v4:
  - Add pIE->length >= 4 guard before the 4-byte OUI memcmps in the
    WLAN_EID_VENDOR_SPECIFIC cases of both functions (sashiko review of v3).
  - In issue_assocreq() WLAN_EID_HT_CAPABILITY: add minimum length check
    (pIE->length < sizeof(struct HT_caps_element)) and use
    sizeof(struct HT_caps_element) instead of pIE->length in rtw_set_ie()
    to prevent OOB reads past the HT_caps struct (sashiko review of v3).
  - In join_cmd_hdl() WLAN_EID_HT_OPERATION: add minimum length check
    (pIE->length < sizeof(struct HT_info_element)) before casting pIE->data
    to struct HT_info_element * and reading infos[0] (sashiko review of v3).

Changes in v3:
  - No code changes from v2.

Changes in v2:
  - Add IE loop header and payload bounds checks for issue_assocreq()
    and join_cmd_hdl().

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 30 ++++++++++++++-----
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 5f00fe282d1b..0c130d0f9a48 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -2925,13 +2925,18 @@ void issue_assocreq(struct adapter *padapter)
 
 	/* vendor specific IE, such as WPA, WMM, WPS */
 	for (i = sizeof(struct ndis_802_11_fix_ie); i < pmlmeinfo->network.ie_length;) {
+		if (i + sizeof(*pIE) > pmlmeinfo->network.ie_length)
+			break;
 		pIE = (struct ndis_80211_var_ie *)(pmlmeinfo->network.ies + i);
+		if (i + sizeof(*pIE) + pIE->length > pmlmeinfo->network.ie_length)
+			break;
 
 		switch (pIE->element_id) {
 		case WLAN_EID_VENDOR_SPECIFIC:
-			if ((!memcmp(pIE->data, RTW_WPA_OUI, 4)) ||
+			if (pIE->length >= 4 &&
+			    ((!memcmp(pIE->data, RTW_WPA_OUI, 4)) ||
 					(!memcmp(pIE->data, WMM_OUI, 4)) ||
-					(!memcmp(pIE->data, WPS_OUI, 4))) {
+					(!memcmp(pIE->data, WPS_OUI, 4)))) {
 				vs_ie_length = pIE->length;
 				if ((!padapter->registrypriv.wifi_spec) && (!memcmp(pIE->data, WPS_OUI, 4))) {
 					/* Commented by Kurt 20110629
@@ -2953,8 +2958,10 @@ void issue_assocreq(struct adapter *padapter)
 		case WLAN_EID_HT_CAPABILITY:
 			if (padapter->mlmepriv.htpriv.ht_option) {
 				if (!(is_ap_in_tkip(padapter))) {
+					if (pIE->length < sizeof(struct HT_caps_element))
+						break;
 					memcpy(&(pmlmeinfo->HT_caps), pIE->data, sizeof(struct HT_caps_element));
-					pframe = rtw_set_ie(pframe, WLAN_EID_HT_CAPABILITY, pIE->length, (u8 *)(&(pmlmeinfo->HT_caps)), &(pattrib->pktlen));
+					pframe = rtw_set_ie(pframe, WLAN_EID_HT_CAPABILITY, sizeof(struct HT_caps_element), (u8 *)(&(pmlmeinfo->HT_caps)), &(pattrib->pktlen));
 				}
 			}
 			break;
@@ -2967,7 +2974,7 @@ void issue_assocreq(struct adapter *padapter)
 			break;
 		}
 
-		i += (pIE->length + 2);
+		i += sizeof(*pIE) + pIE->length;
 	}
 
 	if (pmlmeinfo->assoc_AP_vendor == HT_IOT_PEER_REALTEK)
@@ -5318,11 +5325,15 @@ u8 join_cmd_hdl(struct adapter *padapter, u8 *pbuf)
 
 	/* sizeof(struct ndis_802_11_fix_ie) */
 	for (i = _FIXED_IE_LENGTH_; i < pnetwork->ie_length;) {
+		if (i + sizeof(*pIE) > pnetwork->ie_length)
+			break;
 		pIE = (struct ndis_80211_var_ie *)(pnetwork->ies + i);
+		if (i + sizeof(*pIE) + pIE->length > pnetwork->ie_length)
+			break;
 
 		switch (pIE->element_id) {
 		case WLAN_EID_VENDOR_SPECIFIC:/* Get WMM IE. */
-			if (!memcmp(pIE->data, WMM_OUI, 4))
+			if (pIE->length >= 4 && !memcmp(pIE->data, WMM_OUI, 4))
 				WMM_param_handler(padapter, pIE);
 			break;
 
@@ -5335,7 +5346,12 @@ u8 join_cmd_hdl(struct adapter *padapter, u8 *pbuf)
 
 			/* spec case only for cisco's ap because cisco's ap issue assoc rsp using mcs rate @40MHz or @20MHz */
 			{
-				struct HT_info_element *pht_info = (struct HT_info_element *)(pIE->data);
+				struct HT_info_element *pht_info;
+
+				if (pIE->length < sizeof(struct HT_info_element))
+					break;
+
+				pht_info = (struct HT_info_element *)(pIE->data);
 
 				if (pnetwork->configuration.ds_config <= 14) {
 					if ((pregpriv->bw_mode & 0x0f) > CHANNEL_WIDTH_20)
@@ -5366,7 +5382,7 @@ u8 join_cmd_hdl(struct adapter *padapter, u8 *pbuf)
 			break;
 		}
 
-		i += (pIE->length + 2);
+		i += sizeof(*pIE) + pIE->length;
 	}
 
 	/* check channel, bandwidth, offset and switch */
-- 
2.53.0