[PATCH v7 2/2] staging: rtl8723bs: fix missing frame length checks in OnAuth() and OnAuthClient()

[Alexandru Hossu]

Four out-of-bounds read paths caused by missing frame length guards:

1. OnAuth() reads GetAddr2Ptr (pframe + 10) without verifying the frame
   is at least WLAN_HDR_A3_LEN bytes long.

   The first operation on pframe after the AP-state guard is
   GetAddr2Ptr(pframe), which reads 6 bytes at offset 10..15 (Addr2).
   If the received frame is shorter than WLAN_HDR_A3_LEN (24 bytes),
   this reads past the end of the frame buffer.  Add:
     if (len < WLAN_HDR_A3_LEN) goto auth_fail;

2. OnAuth() reads the algorithm and sequence fields at pframe +
   WLAN_HDR_A3_LEN + offset + {0,2} without verifying that those
   offsets are within the frame.

   offset is 0 for an open-system frame and 4 for a WEP-encapsulated
   frame.  The reads at offset+0 and offset+2 are both 2-byte, so the
   last byte accessed is at WLAN_HDR_A3_LEN + offset + 3.  A crafted
   short frame causes an out-of-bounds read.  Add:
     if (len < WLAN_HDR_A3_LEN + offset + 4) goto auth_fail;

3. OnAuthClient() calls get_da(pframe) without verifying the frame is
   at least WLAN_HDR_A3_LEN bytes long.

   get_da() inspects the ToDs and FrDs bits in Frame Control (bytes
   0..1) and returns either Addr1 (bytes 4..9) or Addr3 (bytes 16..21).
   A frame shorter than WLAN_HDR_A3_LEN (24 bytes) causes an
   out-of-bounds read in either case.  Add:
     if (pkt_len < WLAN_HDR_A3_LEN) goto authclnt_fail;

4. OnAuthClient() reads the sequence field at pframe + WLAN_HDR_A3_LEN
   + offset + 2 and the status field at offset + 4 without verifying
   those offsets are within the frame.

   offset is 0 for open-system and 4 for WEP.  The status read at
   offset+4 is 2 bytes, so the last byte accessed is at
   WLAN_HDR_A3_LEN + offset + 5.  Add:
     if (pkt_len < WLAN_HDR_A3_LEN + offset + 6) goto authclnt_fail;

Note: a previous version of this patch claimed that the signed/unsigned
mismatch in the rtw_get_ie() limit parameter caused an out-of-bounds
scan when pkt_len < WLAN_HDR_A3_LEN + _AUTH_IE_OFFSET_.  This is
incorrect: rtw_get_ie() declares its limit as signed int, so the
wrapped unsigned value is reinterpreted as a large negative number,
which is immediately caught by the if (limit < 2) return NULL; guard
inside rtw_get_ie().  The actual out-of-bounds reads are the four
direct pframe dereferences listed above.

OnAssocRsp() was already fixed by a separate series.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Alexandru Hossu <hossu.alexandru@xxxxxxxxx>
---
Changes in v7:
  - Add frame length checks for OnAuth(): guard before GetAddr2Ptr (len <
    WLAN_HDR_A3_LEN) and guard before algorithm/seq reads (len <
    WLAN_HDR_A3_LEN + offset + 4) (sashiko review of v6).
  - Correct commit message: remove incorrect claim that rtw_get_ie()
    unsigned underflow causes OOB scan; rtw_get_ie() uses signed int
    limit and returns NULL when limit < 2 (sashiko review of v6).

Changes in v6:
  - Add frame length checks for OnAuthClient(): guard before get_da()
    (pkt_len < WLAN_HDR_A3_LEN) and guard before seq/status reads
    (pkt_len < WLAN_HDR_A3_LEN + offset + 6).
  - Correct commit message: OnAssocRsp() was already fixed in a
    separate series.

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index dd3c94d314d8..b42eab61d8a8 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -687,6 +687,9 @@ unsigned int OnAuth(struct adapter *padapter, union recv_frame *precv_frame)
 	if ((pmlmeinfo->state&0x03) != WIFI_FW_AP_STATE)
 		return _FAIL;
 
+	if (len < WLAN_HDR_A3_LEN)
+		goto auth_fail;
+
 	sa = GetAddr2Ptr(pframe);
 
 	auth_mode = psecuritypriv->dot11AuthAlgrthm;
@@ -709,6 +712,9 @@ unsigned int OnAuth(struct adapter *padapter, union recv_frame *precv_frame)
 		offset = 4;
 	}
 
+	if (len < WLAN_HDR_A3_LEN + offset + 4)
+		goto auth_fail;
+
 	algorithm = le16_to_cpu(*(__le16 *)((SIZE_PTR)pframe + WLAN_HDR_A3_LEN + offset));
 	seq	= le16_to_cpu(*(__le16 *)((SIZE_PTR)pframe + WLAN_HDR_A3_LEN + offset + 2));
 
@@ -860,6 +866,9 @@ unsigned int OnAuthClient(struct adapter *padapter, union recv_frame *precv_fram
 	u8 *pframe = precv_frame->u.hdr.rx_data;
 	uint pkt_len = precv_frame->u.hdr.len;
 
+	if (pkt_len < WLAN_HDR_A3_LEN)
+		goto authclnt_fail;
+
 	/* check A1 matches or not */
 	if (memcmp(myid(&(padapter->eeprompriv)), get_da(pframe), ETH_ALEN))
 		return _SUCCESS;
@@ -869,6 +878,9 @@ unsigned int OnAuthClient(struct adapter *padapter, union recv_frame *precv_fram
 
 	offset = (GetPrivacy(pframe)) ? 4 : 0;
 
+	if (pkt_len < WLAN_HDR_A3_LEN + offset + 6)
+		goto authclnt_fail;
+
 	seq	= le16_to_cpu(*(__le16 *)((SIZE_PTR)pframe + WLAN_HDR_A3_LEN + offset + 2));
 	status	= le16_to_cpu(*(__le16 *)((SIZE_PTR)pframe + WLAN_HDR_A3_LEN + offset + 4));
 
-- 
2.53.0