[PATCH v4 1/3] staging: rtl8723bs: fix OOB reads in update_beacon_info() and bwmode_update_check()

[Alexandru Hossu]

Four out-of-bounds read paths in Beacon IE processing:

1. Unsigned underflow in len computation.

   update_beacon_info() computes:

     len = pkt_len - (_BEACON_IE_OFFSET_ + WLAN_HDR_A3_LEN);

   where len is unsigned int.  If pkt_len is smaller than
   _BEACON_IE_OFFSET_ + WLAN_HDR_A3_LEN (36 bytes), the subtraction
   wraps to a very large value, causing the IE loop to iterate over
   memory far beyond the receive buffer.  Add an early return when
   pkt_len is too small.

2. IE header and payload may extend past the packet end.

   The IE loop advances by pIE->length + 2 per iteration but only
   guards on i < len.  When the last IE has only one byte left in
   the frame, the loop reads pIE->length from pframe[len], one byte
   past the receive buffer.  Even when the header bytes are in bounds,
   pIE->length can point the data window past len, silently passing a
   truncated IE to handler functions.  Add two guards: break if fewer
   than sizeof(*pIE) bytes remain, and break if the declared IE payload
   extends past len.

3. WMM OUI comparison reads 6 bytes past a possibly short IE payload.

   For WLAN_EID_VENDOR_SPECIFIC, the code calls
   memcmp(pIE->data, WMM_PARA_OUI, 6) before checking
   pIE->length == WLAN_WMM_LEN.  An IE with pIE->length < 6 causes
   memcmp to read into adjacent frame data.  Swap the condition so the
   length check comes first.

4. bwmode_update_check() missing minimum IE length check.

   bwmode_update_check() rejects IEs longer than
   sizeof(struct HT_info_element) but accepts any shorter length,
   including zero.  After the check it casts pIE->data to
   struct HT_info_element * and reads infos[0] (offset 1), which is
   out of bounds when pIE->length is 0 or 1.  Change the guard from
   > to != to require the IE to be exactly the expected size.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Alexandru Hossu <hossu.alexandru@xxxxxxxxx>
---
Changes in v4:
  - Add pkt_len < _BEACON_IE_OFFSET_ + WLAN_HDR_A3_LEN guard before the
    len subtraction to prevent unsigned underflow (sashiko review of v3).
  - Swap WLAN_EID_VENDOR_SPECIFIC condition: check pIE->length ==
    WLAN_WMM_LEN before memcmp to avoid reading 6 bytes from a short IE
    payload (sashiko review of v3).
  - Fix bwmode_update_check(): change > sizeof(struct HT_info_element) to
    != sizeof(struct HT_info_element) to also reject IEs shorter than the
    expected size, preventing the read of infos[0] on a zero-length IE
    (sashiko review of v3).

Changes in v3:
  - No code changes from v2.

Changes in v2:
  - Add IE loop header and payload bounds checks in update_beacon_info().
  - Use sizeof(*pIE) + pIE->length instead of pIE->length + 2 for
    consistency with the sizeof(*pIE) guards (Dan Carpenter).

 drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
index 6a7c09db4cd9..7ccfaa538ebb 100644
--- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
+++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
@@ -850,7 +850,7 @@ static void bwmode_update_check(struct adapter *padapter, struct ndis_80211_var_
 	if (phtpriv->ht_option == false)
 		return;
 
-	if (pIE->length > sizeof(struct HT_info_element))
+	if (pIE->length != sizeof(struct HT_info_element))
 		return;
 
 	pHT_info = (struct HT_info_element *)pIE->data;
@@ -1286,15 +1286,23 @@ void update_beacon_info(struct adapter *padapter, u8 *pframe, uint pkt_len, stru
 	unsigned int len;
 	struct ndis_80211_var_ie *pIE;
 
+	if (pkt_len < _BEACON_IE_OFFSET_ + WLAN_HDR_A3_LEN)
+		return;
+
 	len = pkt_len - (_BEACON_IE_OFFSET_ + WLAN_HDR_A3_LEN);
 
 	for (i = 0; i < len;) {
+		if (i + sizeof(*pIE) > len)
+			break;
 		pIE = (struct ndis_80211_var_ie *)(pframe + (_BEACON_IE_OFFSET_ + WLAN_HDR_A3_LEN) + i);
+		if (i + sizeof(*pIE) + pIE->length > len)
+			break;
 
 		switch (pIE->element_id) {
 		case WLAN_EID_VENDOR_SPECIFIC:
 			/* to update WMM parameter set while receiving beacon */
-			if (!memcmp(pIE->data, WMM_PARA_OUI, 6) && pIE->length == WLAN_WMM_LEN)	/* WMM */
+			if (pIE->length == WLAN_WMM_LEN &&
+			    !memcmp(pIE->data, WMM_PARA_OUI, 6))	/* WMM */
 				if (WMM_param_handler(padapter, pIE))
 					report_wmm_edca_update(padapter);
 
@@ -1314,7 +1322,7 @@ void update_beacon_info(struct adapter *padapter, u8 *pframe, uint pkt_len, stru
 			break;
 		}
 
-		i += (pIE->length + 2);
+		i += sizeof(*pIE) + pIE->length;
 	}
 }
 
-- 
2.53.0