[PATCH v3 0/2] staging: rtl8723bs: fix OOB write in HT_caps_handler and OOB read in OnAssocRsp

[PATCH v3 0/2] staging: rtl8723bs: fix OOB write in HT_caps_handler and OOB read in OnAssocRsp

[Alexandru Hossu]

v3, addressing Dan Carpenter's and Luka Gejak's feedback on v2.

Greg, please drop your patch and take this one instead.

Changes from v2:
 - 1/2: switch from min_t() to umin() (Dan Carpenter)
 - 1/2: keep truncation approach rather than reverting to early
   return; early return bypasses HT_caps_enable = 1, silently
   disabling HT mode for APs that append extra bytes to the HT
   Capabilities IE (Luka Gejak, AI review)
 - 1/2: expand commit message to document the early return tradeoff
 - 1/2: add changelog with links to AI review and Greg's v1 reply

2/2 is unchanged from v2.

Alexandru Hossu (2):
  staging: rtl8723bs: fix OOB write in HT_caps_handler()
  staging: rtl8723bs: fix OOB read in OnAssocRsp() IE loop

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c  | 4 ++++
 drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.53.0

[PATCH v3 1/2] staging: rtl8723bs: fix OOB write in HT_caps_handler()

[Alexandru Hossu]

HT_caps_handler() iterates pIE->length bytes and writes into
HT_caps.u.HT_cap[], which is a fixed 26-byte array (sizeof struct
HT_caps_element). Because pIE->length is a raw u8 from an over-the-air
802.11 AssocResponse frame and is never validated, a malicious AP can
set it up to 255, causing up to 229 bytes of out-of-bounds writes into
adjacent fields of struct mlme_ext_info.

Truncate the iteration count to the size of HT_caps.u.HT_cap using
umin() so that data from a longer-than-expected IE is silently ignored
rather than written out of bounds, preserving interoperability with APs
that pad the element. An early return on oversized IEs was considered
but rejected: it would bypass the pmlmeinfo->HT_caps_enable = 1
assignment that precedes the loop, silently disabling HT mode for APs
that append extra bytes to the HT Capabilities IE.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
Changes in v3:
- Use umin() instead of min_t() (Dan Carpenter)
- Keep truncation approach; early return would bypass
  HT_caps_enable = 1, disabling HT mode for vendor-padded IEs
  (Luka Gejak, AI review)
- Expand commit message to document the early return tradeoff
- Add changelog

AI review (flagged early return as regression-inducing):
  https://sashiko.dev/#/patchset/2026041408-grill-mahogany-d1e3%40gregkh
Greg KH v1 reply (requested truncation over early return):
  https://lore.kernel.org/linux-staging/2026042630-tightness-runner-2121@gregkh/

 drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
index e0d73c267786..dd34f229df12 100644
--- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
+++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
@@ -936,7 +936,8 @@ void HT_caps_handler(struct adapter *padapter, struct ndis_80211_var_ie *pIE)
 
 	pmlmeinfo->HT_caps_enable = 1;
 
-	for (i = 0; i < (pIE->length); i++) {
+	for (i = 0; i < umin(pIE->length,
+			     sizeof(pmlmeinfo->HT_caps.u.HT_cap)); i++) {
 		if (i != 2) {
 			/* Commented by Albert 2010/07/12 */
 			/* Got the endian issue here. */
-- 
2.53.0

Re: [PATCH v3 1/2] staging: rtl8723bs: fix OOB write in HT_caps_handler()

[Luka Gejak]

On April 28, 2026 11:16:20 AM GMT+02:00, Alexandru Hossu <hossu.alexandru@gmail.com> wrote:
>HT_caps_handler() iterates pIE->length bytes and writes into
>HT_caps.u.HT_cap[], which is a fixed 26-byte array (sizeof struct
>HT_caps_element). Because pIE->length is a raw u8 from an over-the-air
>802.11 AssocResponse frame and is never validated, a malicious AP can
>set it up to 255, causing up to 229 bytes of out-of-bounds writes into
>adjacent fields of struct mlme_ext_info.
>
>Truncate the iteration count to the size of HT_caps.u.HT_cap using
>umin() so that data from a longer-than-expected IE is silently ignored
>rather than written out of bounds, preserving interoperability with APs
>that pad the element. An early return on oversized IEs was considered
>but rejected: it would bypass the pmlmeinfo->HT_caps_enable = 1
>assignment that precedes the loop, silently disabling HT mode for APs
>that append extra bytes to the HT Capabilities IE.
>
>Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
>Cc: stable@vger.kernel.org
>Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
>---
>Changes in v3:
>- Use umin() instead of min_t() (Dan Carpenter)
>- Keep truncation approach; early return would bypass
>  HT_caps_enable = 1, disabling HT mode for vendor-padded IEs
>  (Luka Gejak, AI review)
>- Expand commit message to document the early return tradeoff
>- Add changelog
>
>AI review (flagged early return as regression-inducing):
>  https://sashiko.dev/#/patchset/2026041408-grill-mahogany-d1e3%40gregkh
>Greg KH v1 reply (requested truncation over early return):
>  https://lore.kernel.org/linux-staging/2026042630-tightness-runner-2121@gregkh/
>
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
>index e0d73c267786..dd34f229df12 100644
>--- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
>+++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
>@@ -936,7 +936,8 @@ void HT_caps_handler(struct adapter *padapter, struct ndis_80211_var_ie *pIE)
> 
> 	pmlmeinfo->HT_caps_enable = 1;
> 
>-	for (i = 0; i < (pIE->length); i++) {
>+	for (i = 0; i < umin(pIE->length,
>+			     sizeof(pmlmeinfo->HT_caps.u.HT_cap)); i++) {
> 		if (i != 2) {
> 			/* Commented by Albert 2010/07/12 */
> 			/* Got the endian issue here. */

LGTM,

Reviewed-by: Luka Gejak <luka.gejak@linux.dev>

Best regards,
Luka Gejak

[PATCH v3 2/2] staging: rtl8723bs: fix OOB read in OnAssocRsp() IE loop

[Alexandru Hossu]

The IE parsing loop in OnAssocRsp() advances by (pIE->length + 2) each
iteration but only guards on i < pkt_len. When a malicious AP sends an
AssocResponse whose last IE has only one byte remaining in the frame
(the element_id byte lands at pkt_len-1), the loop reads pIE->length
from pframe[pkt_len], which is one byte past the allocated receive buffer.

Additionally, even when the header bytes are in bounds, pIE->length
itself can extend the data window beyond pkt_len, silently passing a
truncated IE to the handler functions.

Add two guards at the top of the loop body:
  1. Break if fewer than sizeof(*pIE) bytes remain (can't read header).
  2. Break if the IE's declared data extends past pkt_len.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index c646dc2a1741..68ce422305ed 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1406,7 +1406,11 @@ unsigned int OnAssocRsp(struct adapter *padapter, union recv_frame *precv_frame)
 	/* to handle HT, WMM, rate adaptive, update MAC reg */
 	/* for not to handle the synchronous IO in the tasklet */
 	for (i = (6 + WLAN_HDR_A3_LEN); i < pkt_len;) {
+		if (i + sizeof(*pIE) > pkt_len)
+			break;
 		pIE = (struct ndis_80211_var_ie *)(pframe + i);
+		if (i + sizeof(*pIE) + pIE->length > pkt_len)
+			break;
 
 		switch (pIE->element_id) {
 		case WLAN_EID_VENDOR_SPECIFIC:
-- 
2.53.0

Re: [PATCH v3 2/2] staging: rtl8723bs: fix OOB read in OnAssocRsp() IE loop

[Luka Gejak]

On April 28, 2026 11:16:21 AM GMT+02:00, Alexandru Hossu <hossu.alexandru@gmail.com> wrote:
>The IE parsing loop in OnAssocRsp() advances by (pIE->length + 2) each
>iteration but only guards on i < pkt_len. When a malicious AP sends an
>AssocResponse whose last IE has only one byte remaining in the frame
>(the element_id byte lands at pkt_len-1), the loop reads pIE->length
>from pframe[pkt_len], which is one byte past the allocated receive buffer.
>
>Additionally, even when the header bytes are in bounds, pIE->length
>itself can extend the data window beyond pkt_len, silently passing a
>truncated IE to the handler functions.
>
>Add two guards at the top of the loop body:
>  1. Break if fewer than sizeof(*pIE) bytes remain (can't read header).
>  2. Break if the IE's declared data extends past pkt_len.
>
>Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
>Cc: stable@vger.kernel.org
>Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
>---
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
>index c646dc2a1741..68ce422305ed 100644
>--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
>+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
>@@ -1406,7 +1406,11 @@ unsigned int OnAssocRsp(struct adapter *padapter, union recv_frame *precv_frame)
> 	/* to handle HT, WMM, rate adaptive, update MAC reg */
> 	/* for not to handle the synchronous IO in the tasklet */
> 	for (i = (6 + WLAN_HDR_A3_LEN); i < pkt_len;) {
>+		if (i + sizeof(*pIE) > pkt_len)
>+			break;
> 		pIE = (struct ndis_80211_var_ie *)(pframe + i);
>+		if (i + sizeof(*pIE) + pIE->length > pkt_len)
>+			break;
> 
> 		switch (pIE->element_id) {
> 		case WLAN_EID_VENDOR_SPECIFIC:

LGTM,

Reviewed-by: Luka Gejak <luka.gejak@linux.dev>

Best regards,
Luka Gejak

Re: [PATCH v3 0/2] staging: rtl8723bs: fix OOB write in HT_caps_handler and OOB read in OnAssocRsp

[Greg KH]

On Tue, Apr 28, 2026 at 11:16:19AM +0200, Alexandru Hossu wrote:
> v3, addressing Dan Carpenter's and Luka Gejak's feedback on v2.
> 
> Greg, please drop your patch and take this one instead.

What patch should I drop?  What is the git id of it?

And what about these review comments:
	https://sashiko.dev/#/patchset/20260428091621.739680-1-hossu.alexandru@gmail.com

And have you tested this on real hardware?

thanks,

greg k-h

[PATCH v4 0/2] staging: rtl8723bs: fix OOB write and read in HT_caps_handler and OnAssocRsp

[Alexandru Hossu]

v4, addressing the sashiko review comments on v3.

Regarding your questions:

The two patches to drop from your tree are the ones applied from v2:

  41a866092f09 ("staging: rtl8723bs: fix OOB write in HT_caps_handler()")
  e36c54247447 ("staging: rtl8723bs: fix OOB read in OnAssocRsp() IE loop")

v4 supersedes both.

Regarding hardware: I do not have rtl8723bs hardware available.  The
patches are derived from reading the code, cross-checking against the
802.11 spec, and comparing against the existing HT_info_handler() guard
pattern in the same file.

What changed in v4:

Patch 1 (HT_caps_handler):
  The v3 umin() loop bounded the write side correctly, but three macros
  that run after the loop access pIE->data[0] and pIE->data[1]
  unconditionally.  If pIE->length is 0 or 1 those reads go out of
  bounds.  Added if (pIE->length < 2) return; placed after
  HT_caps_enable = 1 so that HT negotiation is not regressed.

Patch 2 (OnAssocRsp):
  Two additional issues found by sashiko:
  - The fixed-field reads (capability, status, AID) at
    pframe + WLAN_HDR_A3_LEN + {0,2,4} run without any minimum frame
    length check.  Added if (pkt_len < WLAN_HDR_A3_LEN + 6) return _FAIL.
  - The WMM OUI comparison (memcmp of 6 bytes) ran without checking
    pIE->length >= 6.  An IE with length < 6 at the end of the packet
    caused the memcmp to read into adjacent frame data.  Added
    pIE->length >= 6 guard.

Alexandru Hossu (2):
  staging: rtl8723bs: fix OOB write and read in HT_caps_handler()
  staging: rtl8723bs: fix OOB reads in OnAssocRsp() IE parsing

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c  | 10 +++++++++-
 drivers/staging/rtl8723bs/core/rtw_wlan_util.c |  6 +++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.53.0

[PATCH v4 1/2] staging: rtl8723bs: fix OOB write and read in HT_caps_handler()

[Alexandru Hossu]

HT_caps_handler() iterates over pIE->length bytes and writes into
HT_caps.u.HT_cap[], a fixed array of sizeof(struct HT_caps_element)
bytes.  pIE->length is a raw u8 from an over-the-air 802.11
Association Response frame and is never validated before the loop.  A
malicious AP can set it to 255, writing up to 229 bytes past the end
of the array into adjacent fields of struct mlme_ext_info.

Additionally, after the loop the function calls three macros that
unconditionally read pIE->data[0] and pIE->data[1]:

  GET_HT_CAPABILITY_ELE_LDPC_CAP(pIE->data)  -- reads data[0], bit 0
  GET_HT_CAPABILITY_ELE_TX_STBC(pIE->data)   -- reads data[0], bit 7
  GET_HT_CAPABILITY_ELE_RX_STBC(pIE->data)   -- reads data[1], bits 0-1

If a malicious AP sends an HT Capabilities IE with pIE->length less
than 2, both bytes the macros need are outside the IE payload,
causing an out-of-bounds read.

Fix both issues:
  - Set HT_caps_enable = 1 first so HT negotiation is not regressed.
  - Return early if pIE->length < 2 to protect the macro reads.
  - Use umin() in the loop to bound the write side.

The parallel HT_info_handler() already guards against oversized IEs.
This patch applies the same discipline to HT_caps_handler().

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 < 2 guard after HT_caps_enable = 1 to prevent OOB
    reads from GET_HT_CAPABILITY_ELE_LDPC_CAP/TX_STBC/RX_STBC macros
    that access pIE->data[0] and pIE->data[1] unconditionally.
    Caught by sashiko review of v3.
  - Use umin() in the loop bound to cap writes at
    sizeof(HT_caps.u.HT_cap) without bypassing HT_caps_enable.

Changes in v3:
  - Switch from min_t() to umin() (Dan Carpenter).
  - Keep truncation approach rather than early return so HT_caps_enable
    is always set before the length check (Luka Gejak, AI review).

Changes in v2:
  - Replace early return before HT_caps_enable = 1 with umin()
    truncation so HT mode is not disabled for APs with oversized IEs.

 drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
index 6a7c09db4cd9..98aa50357e96 100644
--- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
+++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
@@ -936,7 +936,11 @@ void HT_caps_handler(struct adapter *padapter, struct ndis_80211_var_ie *pIE)
 
 	pmlmeinfo->HT_caps_enable = 1;
 
-	for (i = 0; i < (pIE->length); i++) {
+	if (pIE->length < 2)
+		return;
+
+	for (i = 0; i < umin(pIE->length,
+			     sizeof(pmlmeinfo->HT_caps.u.HT_cap)); i++) {
 		if (i != 2) {
 			/* Commented by Albert 2010/07/12 */
 			/* Got the endian issue here. */
-- 
2.53.0

[PATCH v4 2/2] staging: rtl8723bs: fix OOB reads in OnAssocRsp() IE parsing

[Alexandru Hossu]

Three out-of-bounds read paths in OnAssocRsp():

1. Missing minimum frame length check before fixed field reads.

   Before entering the IE loop the function reads capability, status,
   and AID from fixed offsets relative to pframe + WLAN_HDR_A3_LEN
   (at offsets +0, +2, and +4 respectively, so 6 bytes total).  There
   is no check that pkt_len is large enough to cover these fields.  A
   malicious AP can send a truncated Association Response frame shorter
   than WLAN_HDR_A3_LEN + 6 bytes, causing out-of-bounds reads and
   loading garbage into MLME state variables.

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 < pkt_len.  When the last IE has only one byte left in
   the frame, the loop reads pIE->length from pframe[pkt_len], one
   byte past the receive buffer.  Even when the header bytes are in
   bounds, pIE->length can point the data window past pkt_len, silently
   passing a truncated IE to the handler functions.

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) without checking that pIE->length is at least 6.
   An attacker can craft a vendor-specific IE at the end of the frame
   with pIE->length smaller than 6.  The existing IE bounds check only
   confirms the declared payload fits within pkt_len, not that it is
   large enough for the 6-byte OUI comparison.

Fix all three:
  - Return _FAIL immediately if pkt_len < WLAN_HDR_A3_LEN + 6.
  - Add two guards in the IE loop: break if fewer than sizeof(*pIE)
    bytes remain, and break if the declared IE payload extends past
    pkt_len.
  - Guard the WMM OUI comparison with pIE->length >= 6.

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 pkt_len < WLAN_HDR_A3_LEN + 6 check before reading the three
    fixed fields (capability, status, AID) to prevent OOB reads from
    truncated frames. Caught by sashiko review of v3.
  - Add pIE->length >= 6 guard before the 6-byte WMM OUI memcmp to
    prevent reading past a short IE payload. Caught by sashiko.

Changes in v2:
  - Add IE header bounds check: break if i + sizeof(*pIE) > pkt_len.
  - Add IE payload bounds check: break if the declared IE data extends
    past pkt_len.

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 5f00fe282d1b..84cc814f069c 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1379,6 +1379,9 @@ unsigned int OnAssocRsp(struct adapter *padapter, union recv_frame *precv_frame)
 
 	timer_delete_sync(&pmlmeext->link_timer);
 
+	if (pkt_len < WLAN_HDR_A3_LEN + 6)
+		return _FAIL;
+
 	/* status */
 	status = le16_to_cpu(*(__le16 *)(pframe + WLAN_HDR_A3_LEN + 2));
 	if (status > 0) {
@@ -1400,11 +1403,16 @@ unsigned int OnAssocRsp(struct adapter *padapter, union recv_frame *precv_frame)
 	/* to handle HT, WMM, rate adaptive, update MAC reg */
 	/* for not to handle the synchronous IO in the tasklet */
 	for (i = (6 + WLAN_HDR_A3_LEN); i < pkt_len;) {
+		if (i + sizeof(*pIE) > pkt_len)
+			break;
 		pIE = (struct ndis_80211_var_ie *)(pframe + i);
+		if (i + sizeof(*pIE) + pIE->length > pkt_len)
+			break;
 
 		switch (pIE->element_id) {
 		case WLAN_EID_VENDOR_SPECIFIC:
-			if (!memcmp(pIE->data, WMM_PARA_OUI, 6))	/* WMM */
+			if (pIE->length >= 6 &&
+			    !memcmp(pIE->data, WMM_PARA_OUI, 6))	/* WMM */
 				WMM_param_handler(padapter, pIE);
 			break;
 
-- 
2.53.0