Re: [PATCH v3 1/5] staging: rtl8723bs: fix heap buffer overflow in recvframe_defrag()

[Dan Carpenter <error27@gmail.com>]

On Sun, Apr 05, 2026 at 11:15:44AM +0100, Delene Tchio Romuald wrote:
> In recvframe_defrag(), a memcpy() copies fragment data into the
> reassembly buffer before recvframe_put() validates that the buffer
> has sufficient space. If the total reassembled payload exceeds the
> receive buffer capacity, this results in a heap buffer overflow.
> 
> An attacker within WiFi radio range can exploit this by sending
> crafted 802.11 fragmented frames. No authentication is required.
> 
> Add a bounds check before the memcpy() to verify that the fragment
> payload fits within the remaining buffer space, using the same error
> handling pattern already present in the function.
> 
> Found by reviewing memory operations in the driver and tracing
> buffer pointer manipulation through rtw_recv.h inline helpers.
> Not tested on hardware.
> 
> Signed-off-by: Delene Tchio Romuald <delenetchior1@gmail.com>
> ---
> 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)
>  - Removed Cc: stable (will be added by maintainer)

Wait, what?  That used to be a thing in net, I don't know if it still is
a rule there but so far as I know, it's not a rule in staging.

I made checkpatch complain if you CC stable but you don't have a Fixes
tag.  Could that be what you're talking about?  A bunch of people have
asked for Fixes tags on these patches as well.  Please, do that?  :P

> 
>  drivers/staging/rtl8723bs/core/rtw_recv.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index f78194d508dfc..717e0594d983a 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -1132,7 +1132,13 @@ static union recv_frame *recvframe_defrag(struct adapter *adapter,
>  		/* append  to first fragment frame's tail (if privacy frame, pull the ICV) */
>  		recvframe_pull_tail(prframe, pfhdr->attrib.icv_len);

There is no error checking on these pull calls.  They're supposed
to return NULL if pfhdr->attrib.icv_len is too big.  (There is a similar
caller earlier on the line before).

>  
> -		/* memcpy */
> +		/* Verify the receiving buffer has enough space for the fragment */
> +		if (pnfhdr->len > (uint)(pfhdr->rx_end - pfhdr->rx_tail)) {

Remove the unnecessary cast.

Otherwise it looks good to me.

regards,
dan carpenter

> +			rtw_free_recvframe(prframe, pfree_recv_queue);
> +			rtw_free_recvframe_queue(defrag_q, pfree_recv_queue);
> +			return NULL;
> +		}
> +
>  		memcpy(pfhdr->rx_tail, pnfhdr->rx_data, pnfhdr->len);
>  
>  		recvframe_put(prframe, pnfhdr->len);
> -- 
> 2.43.0
>