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

[Luka Gejak]

On Thu Apr 16, 2026 at 6:36 PM CEST, Dan Carpenter wrote:
> 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.
Hi Dan,
I wasn't aware of that. I thought in such cases you should follow the 
pattern of existing code. I will make sure to note that in future 
reviews.
Best regards,
Luka Gejak
>
> 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;