* [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss
@ 2026-09-20 14:28 Adi Prasan
2026-09-20 15:37 ` Greg KH
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Adi Prasan @ 2026-09-20 14:28 UTC (permalink / raw)
To: gregkh; +Cc: linux-staging, linux-kernel, error27, Adi Prasan
The buffer bound check in rtw_cfg80211_inform_bss() only verifies
that bssinf_len (ie_length + header size) does not exceed
MAX_BSSINFO_LEN (1000 bytes), but network.ies[] is only MAX_IE_SZ
(768) bytes. This allows ie_length values up to ~976 bytes to pass
the check while a subsequent memcpy() from network.ies still reads
only 768 valid bytes, and other paths that write to network.ies
consistently cap ie_length to MAX_IE_SZ.
Add an explicit check against MAX_IE_SZ so the bound matches the
actual size of network.ies.
Signed-off-by: Adi Prasan <itsadi2409@gmail.com>
---
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 3468d4114f60..27e7b8442d7b 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -219,7 +219,7 @@ struct cfg80211_bss *rtw_cfg80211_inform_bss(struct adapter *padapter, struct wl
struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
bssinf_len = pnetwork->network.ie_length + sizeof(struct ieee80211_hdr_3addr);
- if (bssinf_len > MAX_BSSINFO_LEN)
+ if (bssinf_len > MAX_BSSINFO_LEN || pnetwork->network.ie_length > MAX_IE_SZ)
goto exit;
{
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss
2026-09-20 14:28 [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss Adi Prasan
@ 2026-09-20 15:37 ` Greg KH
2026-09-21 5:13 ` Adi Prasan
2026-09-21 15:23 ` Dan Carpenter
2026-09-21 16:55 ` [PATCH v2] " Adi Prasan
2 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2026-09-20 15:37 UTC (permalink / raw)
To: Adi Prasan; +Cc: linux-staging, linux-kernel, error27
On Sun, Sep 20, 2026 at 02:28:49PM +0000, Adi Prasan wrote:
> The buffer bound check in rtw_cfg80211_inform_bss() only verifies
> that bssinf_len (ie_length + header size) does not exceed
> MAX_BSSINFO_LEN (1000 bytes), but network.ies[] is only MAX_IE_SZ
> (768) bytes. This allows ie_length values up to ~976 bytes to pass
> the check while a subsequent memcpy() from network.ies still reads
> only 768 valid bytes, and other paths that write to network.ies
> consistently cap ie_length to MAX_IE_SZ.
How was this found and tested?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss
2026-09-20 15:37 ` Greg KH
@ 2026-09-21 5:13 ` Adi Prasan
0 siblings, 0 replies; 6+ messages in thread
From: Adi Prasan @ 2026-09-21 5:13 UTC (permalink / raw)
To: gregkh; +Cc: linux-staging, linux-kernel, error27
Hi Greg,
I ran smatch over drivers/staging and it flagged this
function - the check here allows ie_length up to ~976 bytes (1000
minus the 24-byte header), but network.ies[] is only MAX_IE_SZ (768)
bytes, so the memcpy() a few lines down can read past the end of
that array.
I went and checked every place that sets ie_length before it reaches
here - collect_bss_info() in rtw_mlme_ext.c, and the two
H2C_PARAMETERS_ERROR checks nearby - and all of them already clamp it
to MAX_IE_SZ. So this isn't reachable through any current caller, it
was just the local check not matching the actual buffer size. Wanted
to fix it directly rather than rely on every caller continuing to
enforce that cap.
For testing I have build-tested with make M=drivers/staging/rtl8723bs, clean
checkpatch. I don't have the actual hardware to test at runtime, and
since this only tightens a bound that's already unreachable in
practice, there's no behavior change for any existing valid input.
Thanks,
Adi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss
2026-09-20 14:28 [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss Adi Prasan
2026-09-20 15:37 ` Greg KH
@ 2026-09-21 15:23 ` Dan Carpenter
2026-09-21 17:03 ` Adi Prasan
2026-09-21 16:55 ` [PATCH v2] " Adi Prasan
2 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2026-09-21 15:23 UTC (permalink / raw)
To: Adi Prasan; +Cc: gregkh, linux-staging, linux-kernel
On Sun, Sep 20, 2026 at 02:28:49PM +0000, Adi Prasan wrote:
> The buffer bound check in rtw_cfg80211_inform_bss() only verifies
> that bssinf_len (ie_length + header size) does not exceed
> MAX_BSSINFO_LEN (1000 bytes), but network.ies[] is only MAX_IE_SZ
> (768) bytes. This allows ie_length values up to ~976 bytes to pass
> the check while a subsequent memcpy() from network.ies still reads
> only 768 valid bytes, and other paths that write to network.ies
> consistently cap ie_length to MAX_IE_SZ.
>
> Add an explicit check against MAX_IE_SZ so the bound matches the
> actual size of network.ies.
>
> Signed-off-by: Adi Prasan <itsadi2409@gmail.com>
This needs a Fixes tag.
The original code seems like a bounds check on the destination.
Your code adds a separate bounds check on the read buffer.
Why do we even have the MAX_BSSINFO_LEN limit? What's that based
on? 1000 seems like a very suspicious number to me. It's a normal
enough number for humans, but it's a strange number when we're adding
up struct sizes. Do we ever need the whole buffer? (These questions
are basically rephrasing the same question. I'm assuming everyone
just feeds them to AI, and I'm trying to learn who to do prompt
engineering).
It wouldn't surprise me if there was a different read check on the
source buffer.
The other question for me is:
304 memcpy(pbuf, pnetwork->network.ies, pnetwork->network.ie_length);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We copy the network.ies entries to pbuf
305 len += pnetwork->network.ie_length;
306
307 *((__le64 *)pbuf) = cpu_to_le64(notify_timestamp);
^^^^^^^^^^^^^^^^^
And then scribble over the first entry. That doesn't make sense.
Should the timestamp go before or after the entries? Review the
git log and other implementations of the the realtek wireless drivers
to check.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss
2026-09-21 15:23 ` Dan Carpenter
@ 2026-09-21 17:03 ` Adi Prasan
0 siblings, 0 replies; 6+ messages in thread
From: Adi Prasan @ 2026-09-21 17:03 UTC (permalink / raw)
To: error27; +Cc: gregkh, linux-staging, linux-kernel
Hi Dan,
Went and checked for the things you suggested.
Fixes tag: git blame shows this check hasn't been touched since the
original import, 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi
driver"). Added that in v2.
On MAX_BSSINFO_LEN: I couldn't find any rationale for 1000 anywhere
in the history - it's exactly as it was in the 2017 import, no
comment, no commit explaining it. Header (24) + MAX_IE_SZ (768) =
792, so there's already ~200 bytes of slack in the allocation beyond
what ies[] can actually hold. Looks like an arbitrary/conservative
number carried over from wherever this was ported from, not derived
from any struct size in this tree. My patch doesn't touch the
allocation, just tightens the check to match what ies[] can hold.
On the timestamp write - I don't think it's corrupting IE data,
though I get why it looks that way. network.ies[] isn't a pure IE
list despite the name - its declaration comment says "timestamp,
beacon interval, and capability information", and collect_bss_info()
confirms it: it memcpy's straight from the raw frame body right
after the header, so ies[0:8] is the captured TSF, ies[8:10] is
beacon_interval, ies[10:12] is capab_info, and actual variable IEs
start at offset 12 (matches _FIXED_IE_LENGTH_ used elsewhere in this
file). So the memcpy() followed by the timestamp write isn't
scribbling an IE entry - it's replacing the captured TSF (bytes 0-7)
with notify_timestamp = ktime_to_us(ktime_get_boottime()), while
beacon_interval/capab_info/IEs from the original capture stay
untouched. Order doesn't affect the result since it's the same 8
bytes either way.
That said, I'm not certain cfg80211 is fine getting a local boottime
value here instead of the AP's real TSF - if that's actually wrong
I'd like to understand why, I don't have full context on what
cfg80211_inform_bss_frame does with that field internally.
Thanks,
Adi
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss
2026-09-20 14:28 [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss Adi Prasan
2026-09-20 15:37 ` Greg KH
2026-09-21 15:23 ` Dan Carpenter
@ 2026-09-21 16:55 ` Adi Prasan
2 siblings, 0 replies; 6+ messages in thread
From: Adi Prasan @ 2026-09-21 16:55 UTC (permalink / raw)
To: gregkh; +Cc: linux-staging, linux-kernel, error27, Adi Prasan
The buffer bound check in rtw_cfg80211_inform_bss() only verifies
that bssinf_len (ie_length + header size) does not exceed
MAX_BSSINFO_LEN (1000 bytes), but network.ies[] is only MAX_IE_SZ
(768) bytes. This allows ie_length values up to ~976 bytes to pass
the check while a subsequent memcpy() from network.ies still reads
only 768 valid bytes, and other paths that write to network.ies
consistently cap ie_length to MAX_IE_SZ.
Add an explicit check against MAX_IE_SZ so the bound matches the
actual size of network.ies.
Signed-off-by: Adi Prasan <itsadi2409@gmail.com>
Fixes: 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi driver")
---
drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 3468d4114f60..27e7b8442d7b 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -219,7 +219,7 @@ struct cfg80211_bss *rtw_cfg80211_inform_bss(struct adapter *padapter, struct wl
struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
bssinf_len = pnetwork->network.ie_length + sizeof(struct ieee80211_hdr_3addr);
- if (bssinf_len > MAX_BSSINFO_LEN)
+ if (bssinf_len > MAX_BSSINFO_LEN || pnetwork->network.ie_length > MAX_IE_SZ)
goto exit;
{
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-21 17:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-20 14:28 [PATCH] staging: rtl8723bs: fix ie_length bound check in rtw_cfg80211_inform_bss Adi Prasan
2026-09-20 15:37 ` Greg KH
2026-09-21 5:13 ` Adi Prasan
2026-09-21 15:23 ` Dan Carpenter
2026-09-21 17:03 ` Adi Prasan
2026-09-21 16:55 ` [PATCH v2] " Adi Prasan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®