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

Luka Gejak
2026-04-16T17:44:24Z

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@vger.kernel.org
>> Reviewed-by: Luka Gejak <luka.gejak@linux.dev>
>> Signed-off-by: Delene Tchio Romuald <delenetchior1@gmail.com>
>> ---
>> 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;