Re: [PATCH v4 3/5] staging: rtl8723bs: fix out-of-bounds read in portctrl()

[Dan Carpenter]

On Wed, Apr 15, 2026 at 07:54:59PM +0100, Delene Tchio Romuald wrote:
> In portctrl(), when 802.1X port control is enabled and a non-EAPOL
> frame is received, the ether_type is read from the LLC header
> without verifying that the frame actually contains enough bytes to
> hold the MAC header, IV and the LLC header plus two bytes of
> ether_type. For sufficiently short frames, the memcpy() that loads
> be_tmp reads past the end of the receive buffer.
> 
> An attacker within WiFi radio range can exploit this by sending a
> crafted short frame. No authentication is required.
> 
> Validate the frame length before dereferencing the LLC header; drop
> the frame if it is too short.
> 
> Found by reviewing length validation in the receive path.
> Not tested on hardware.
> 
> Fixes: 554c0a3abf216 ("staging: Add rtl8723bs sdio wifi driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: Luka Gejak <luka.gejak@xxxxxxxxx>
> Signed-off-by: Delene Tchio Romuald <delenetchior1@xxxxxxxxx>
> ---
> v4: add Fixes: tag and Cc: stable (Dan Carpenter); carry Luka Gejak's
>     Reviewed-by.
> v3: rebased on staging-next; sent as numbered series with proper
>     Cc from get_maintainer.pl.
> v2: rebased on staging-next (v1 was based on v7.0-rc6 and did not
>     apply).
> 
>  drivers/staging/rtl8723bs/core/rtw_recv.c | 28 +++++++++++++++--------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index 00b69571bbb83..c0a1c2ab710ee 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -539,17 +539,25 @@ static union recv_frame *portctrl(struct adapter *adapter, union recv_frame *pre
>  
>  			prtnframe = precv_frame;
>  
> -			/* get ether_type */
> -			ptr = ptr + pfhdr->attrib.hdrlen + pfhdr->attrib.iv_len + LLC_HEADER_LENGTH;
> -			memcpy(&be_tmp, ptr, 2);
> -			ether_type = ntohs(be_tmp);
> -
> -			if (ether_type == eapol_type)
> -				prtnframe = precv_frame;
> -			else {
> -				/* free this frame */
> -				rtw_free_recvframe(precv_frame, &adapter->recvpriv.free_recv_queue);
> +			/* Ensure frame has LLC header and ether_type */
> +			if (pfhdr->len < pattrib->hdrlen +
> +			    pattrib->iv_len + LLC_HEADER_LENGTH + 2) {
> +				rtw_free_recvframe(precv_frame,
> +						   &adapter->recvpriv.free_recv_queue);
>  				prtnframe = NULL;

I feel like it's sort of weird to write this as a pfhdr->len < condition.
I feel like the untrusted part of the condition is the pattrib->hdrlen
stuff and normally you would put the untrusted parts on the left.  I
kind of see what you're saying that the packet is too small, but to me
I see it as the hdrlen is too big...  But, also since you found the bug
then you get to choose the style on this, so do which ever way you feel
is best.

It would be better if instead of setting "prtnframe = NULL;" here,
you just did "return NULL;" instead.  You've followed the pattern of
the existing code, but the rule is that if the function has a 100 lines
of bad style code, you should add 1 line of good style even if it's
inconsistent.

It makes the code slightly better and it makes the diff a lot smaller
and clearer.

regards,
dan carpenter

diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
index f78194d508df..9cedca1bd83a 100644
--- a/drivers/staging/rtl8723bs/core/rtw_recv.c
+++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
@@ -531,6 +531,13 @@ static union recv_frame *portctrl(struct adapter *adapter, union recv_frame *pre
 			/* only accept EAPOL frame */
 
 			prtnframe = precv_frame;
+			/* Ensure frame has LLC header and ether_type */
+			if (pfhdr->len < pattrib->hdrlen +
+			    pattrib->iv_len + LLC_HEADER_LENGTH + 2) {
+				rtw_free_recvframe(precv_frame,
+						   &adapter->recvpriv.free_recv_queue);
+				return NULL;
+			}
 
 			/* get ether_type */
 			ptr = ptr + pfhdr->attrib.hdrlen + pfhdr->attrib.iv_len + LLC_HEADER_LENGTH;