[PATCH] staging: rtl8723bs: validate WPS attribute lengths

[PATCH] staging: rtl8723bs: validate WPS attribute lengths

[Laxman Acharya Padhya]

rtw_get_wps_attr() checks that the four-byte attribute header fits in the
WPS information element, but trusts the payload length from that header
when copying the attribute and advancing to the next one. A malformed
attribute can therefore make the driver read beyond a received management
frame. Storing the total attribute length in u16 also allows the addition
of the header size to wrap.

rtw_get_wps_attr_content() also copies the full payload without knowing
the destination size. Its callers copy the Selected Registrar attribute
into one-byte objects, so an oversized payload can overwrite the stack
even when the payload itself fits inside the information element.

Store the total attribute length in u32 and reject attributes extending
past the information element. Add destination lengths to the copy helpers
and reject attributes that do not fit before copying them.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Laxman Acharya Padhya <acharyalaxman8848@gmail.com>

diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
index 863ddf846..4889c7247 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
@@ -702,11 +702,13 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8 *wps_ie, uint *wps_ielen)
  * @wps_ielen: Length limit from wps_ie
  * @target_attr_id: The attribute ID of WPS attribute to search
  * @buf_attr: If not NULL and the WPS attribute is found, WPS attribute will be copied to the buf starting from buf_attr
+ * @buf_attr_len: Length of buf_attr
  * @len_attr: If not NULL and the WPS attribute is found, will set to the length of the entire WPS attribute
  *
  * Returns: the address of the specific WPS attribute found, or NULL
  */
-u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_attr, u32 *len_attr)
+u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id,
+		     u8 *buf_attr, u32 buf_attr_len, u32 *len_attr)
 {
 	u8 *attr_ptr = NULL;
 	u8 *target_attr_ptr = NULL;
@@ -732,13 +734,19 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_att
 			break;
 		u16 attr_id = get_unaligned_be16(attr_ptr);
 		u16 attr_data_len = get_unaligned_be16(attr_ptr + 2);
-		u16 attr_len = attr_data_len + 4;
+		u32 attr_len = attr_data_len + 4;
+
+		if (attr_len > wps_ie + wps_ielen - attr_ptr)
+			break;
 
 		if (attr_id == target_attr_id) {
 			target_attr_ptr = attr_ptr;
 
-			if (buf_attr)
+			if (buf_attr) {
+				if (attr_len > buf_attr_len)
+					return NULL;
 				memcpy(buf_attr, attr_ptr, attr_len);
+			}
 
 			if (len_attr)
 				*len_attr = attr_len;
@@ -757,26 +765,35 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_att
  * @wps_ielen: Length limit from wps_ie
  * @target_attr_id: The attribute ID of WPS attribute to search
  * @buf_content: If not NULL and the WPS attribute is found, WPS attribute content will be copied to the buf starting from buf_content
+ * @buf_content_len: Length of buf_content
  * @len_content: If not NULL and the WPS attribute is found, will set to the length of the WPS attribute content
  *
  * Returns: the address of the specific WPS attribute content found, or NULL
  */
-u8 *rtw_get_wps_attr_content(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_content, uint *len_content)
+u8 *rtw_get_wps_attr_content(u8 *wps_ie, uint wps_ielen, u16 target_attr_id,
+			     u8 *buf_content, uint buf_content_len,
+			     uint *len_content)
 {
 	u8 *attr_ptr;
 	u32 attr_len;
+	u32 content_len;
 
 	if (len_content)
 		*len_content = 0;
 
-	attr_ptr = rtw_get_wps_attr(wps_ie, wps_ielen, target_attr_id, NULL, &attr_len);
+	attr_ptr = rtw_get_wps_attr(wps_ie, wps_ielen, target_attr_id, NULL, 0,
+				    &attr_len);
 
 	if (attr_ptr && attr_len) {
-		if (buf_content)
-			memcpy(buf_content, attr_ptr + 4, attr_len - 4);
+		content_len = attr_len - 4;
+		if (buf_content) {
+			if (content_len > buf_content_len)
+				return NULL;
+			memcpy(buf_content, attr_ptr + 4, content_len);
+		}
 
 		if (len_content)
-			*len_content = attr_len - 4;
+			*len_content = content_len;
 
 		return attr_ptr + 4;
 	}
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index a443b3530..ab620231c 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1113,7 +1113,11 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame)
 			if (pmlmepriv->wps_beacon_ie) {
 				u8 selected_registrar = 0;
 
-				rtw_get_wps_attr_content(pmlmepriv->wps_beacon_ie, pmlmepriv->wps_beacon_ie_len, WPS_ATTR_SELECTED_REGISTRAR, &selected_registrar, NULL);
+				rtw_get_wps_attr_content(pmlmepriv->wps_beacon_ie,
+							 pmlmepriv->wps_beacon_ie_len,
+							 WPS_ATTR_SELECTED_REGISTRAR,
+							 &selected_registrar,
+							 sizeof(selected_registrar), NULL);
 
 				if (!selected_registrar) {
 					status = WLAN_STATUS_AP_UNABLE_TO_HANDLE_NEW_STA;
@@ -2116,7 +2120,9 @@ void issue_beacon(struct adapter *padapter, int timeout_ms)
 			wps_ie = rtw_get_wps_ie(pmgntframe->buf_addr+TXDESC_OFFSET+sizeof(struct ieee80211_hdr_3addr)+_BEACON_IE_OFFSET_,
 				pattrib->pktlen-sizeof(struct ieee80211_hdr_3addr)-_BEACON_IE_OFFSET_, NULL, &wps_ielen);
 			if (wps_ie && wps_ielen > 0)
-				rtw_get_wps_attr_content(wps_ie,  wps_ielen, WPS_ATTR_SELECTED_REGISTRAR, (u8 *)(&sr), NULL);
+				rtw_get_wps_attr_content(wps_ie, wps_ielen,
+							 WPS_ATTR_SELECTED_REGISTRAR,
+							 &sr, sizeof(sr), NULL);
 			if (sr != 0)
 				set_fwstate(pmlmepriv, WIFI_UNDER_WPS);
 			else
diff --git a/drivers/staging/rtl8723bs/include/ieee80211.h b/drivers/staging/rtl8723bs/include/ieee80211.h
index 39ee139f1..114d73a91 100644
--- a/drivers/staging/rtl8723bs/include/ieee80211.h
+++ b/drivers/staging/rtl8723bs/include/ieee80211.h
@@ -735,8 +735,11 @@ int rtw_parse_wpa2_ie(u8 *wpa_ie, int wpa_ie_len, int *group_cipher, int *pairwi
 void rtw_get_sec_ie(u8 *in_ie, uint in_len, u8 *rsn_ie, u16 *rsn_len, u8 *wpa_ie, u16 *wpa_len);
 
 u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8 *wps_ie, uint *wps_ielen);
-u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_attr, u32 *len_attr);
-u8 *rtw_get_wps_attr_content(u8 *wps_ie, uint wps_ielen, u16 target_attr_id, u8 *buf_content, uint *len_content);
+u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16 target_attr_id,
+		     u8 *buf_attr, u32 buf_attr_len, u32 *len_attr);
+u8 *rtw_get_wps_attr_content(u8 *wps_ie, uint wps_ielen, u16 target_attr_id,
+			     u8 *buf_content, uint buf_content_len,
+			     uint *len_content);
 
 /**
  * for_each_ie - iterate over continuous IEs
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 967cd1b34..60c27b4fd 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -251,7 +251,9 @@ struct cfg80211_bss *rtw_cfg80211_inform_bss(struct adapter *padapter, struct wl
 		wpsie = rtw_get_wps_ie(pnetwork->network.ies + _FIXED_IE_LENGTH_, pnetwork->network.ie_length - _FIXED_IE_LENGTH_, NULL, &wpsielen);
 
 		if (wpsie && wpsielen > 0)
-			psr = rtw_get_wps_attr_content(wpsie, wpsielen, WPS_ATTR_SELECTED_REGISTRAR, (u8 *)(&sr), NULL);
+			psr = rtw_get_wps_attr_content(wpsie, wpsielen,
+						       WPS_ATTR_SELECTED_REGISTRAR,
+						       &sr, sizeof(sr), NULL);
 
 		if (sr != 0) {
 			/* it means under processing WPS */
-- 
2.51.2

Re: [PATCH] staging: rtl8723bs: validate WPS attribute lengths

[Greg Kroah-Hartman]

On Sat, Aug 01, 2026 at 11:31:11PM +0545, Laxman Acharya Padhya wrote:
> rtw_get_wps_attr() checks that the four-byte attribute header fits in the
> WPS information element, but trusts the payload length from that header
> when copying the attribute and advancing to the next one. A malformed
> attribute can therefore make the driver read beyond a received management
> frame. Storing the total attribute length in u16 also allows the addition
> of the header size to wrap.
> 
> rtw_get_wps_attr_content() also copies the full payload without knowing
> the destination size. Its callers copy the Selected Registrar attribute
> into one-byte objects, so an oversized payload can overwrite the stack
> even when the payload itself fits inside the information element.
> 
> Store the total attribute length in u32 and reject attributes extending
> past the information element. Add destination lengths to the copy helpers
> and reject attributes that do not fit before copying them.
> 
> Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Laxman Acharya Padhya <acharyalaxman8848@gmail.com>

How was this found and tested?

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: validate WPS attribute lengths

[Laxman Acharya Padhya]

Hi Greg,

This was found during an OpenAI Codex-assisted static
review. Codex helped identify the unchecked length and draft the initial
change; I then traced the call sites. I should have disclosed that
assistance in the original submission.

The reachable path is through received scan results:
rtw_cfg80211_inform_bss() parses a WPS IE and copies the Selected Registrar
payload into the one-byte stack variable `sr`. The outer IE is validated,
but the inner attribute length was not. An oversized payload can therefore
overwrite `sr`. An attribute length of 0xffff also makes the old u16
attr_len calculation wrap to 3, after which attr_len - 4 underflows before
memcpy().

I ran git diff --check, strict checkpatch (no errors or warnings), and a
Docker ARM64 W=1 build with CONFIG_RTL8723BS=m. I did not test on hardware
or inject a malformed frame, so testing was compile and static analysis
only. I will include the appropriate Assisted-by information and improve
runtime testing before any revision.

Thanks,
Laxman

On Sun, 2 Aug 2026 at 13:03, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Aug 01, 2026 at 11:31:11PM +0545, Laxman Acharya Padhya wrote:
> > rtw_get_wps_attr() checks that the four-byte attribute header fits in the
> > WPS information element, but trusts the payload length from that header
> > when copying the attribute and advancing to the next one. A malformed
> > attribute can therefore make the driver read beyond a received management
> > frame. Storing the total attribute length in u16 also allows the addition
> > of the header size to wrap.
> >
> > rtw_get_wps_attr_content() also copies the full payload without knowing
> > the destination size. Its callers copy the Selected Registrar attribute
> > into one-byte objects, so an oversized payload can overwrite the stack
> > even when the payload itself fits inside the information element.
> >
> > Store the total attribute length in u32 and reject attributes extending
> > past the information element. Add destination lengths to the copy helpers
> > and reject attributes that do not fit before copying them.
> >
> > Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Laxman Acharya Padhya <acharyalaxman8848@gmail.com>
>
> How was this found and tested?
>
> thanks,
>
> greg k-h

Re: [PATCH] staging: rtl8723bs: validate WPS attribute lengths

[Greg Kroah-Hartman]

On Sun, Aug 02, 2026 at 01:57:09PM +0545, Laxman Acharya Padhya wrote:
> Hi Greg,
> 
> This was found during an OpenAI Codex-assisted static
> review. Codex helped identify the unchecked length and draft the initial
> change; I then traced the call sites. I should have disclosed that
> assistance in the original submission.

Please do so.

> The reachable path is through received scan results:
> rtw_cfg80211_inform_bss() parses a WPS IE and copies the Selected Registrar
> payload into the one-byte stack variable `sr`. The outer IE is validated,
> but the inner attribute length was not. An oversized payload can therefore
> overwrite `sr`. An attribute length of 0xffff also makes the old u16
> attr_len calculation wrap to 3, after which attr_len - 4 underflows before
> memcpy().
> 
> I ran git diff --check, strict checkpatch (no errors or warnings), and a
> Docker ARM64 W=1 build with CONFIG_RTL8723BS=m. I did not test on hardware
> or inject a malformed frame, so testing was compile and static analysis
> only. I will include the appropriate Assisted-by information and improve
> runtime testing before any revision.

Please test stuff like this on real hardware, so we know you didn't
break anything.

thanks,

greg k-h