Re: [PATCH] staging: rtl8723bs: fix unchecked return value of skb_copy_bits

Re: [PATCH] staging: rtl8723bs: fix unchecked return value of skb_copy_bits

[Minu Jin]

Hi Dan,

Thanks for the feedback.
I agree, following the kernel standard is the right way.

I've updated the code to propagate the error codes and fixed the callers.

Thanks,

Minu Jin

[PATCH] staging: rtl8723bs: fix unchecked return value of skb_copy_bits

[Minu Jin]

The function _rtw_pktfile_read() incorrectly updated the file pointer
even when skb_copy_bits() failed.

This patch fixes the issue by:

    1. Returning 0 from _rtw_pktfile_read() if skb_copy_bits() fails,
    preventing internal pointer updates.

    2. Updating all callers to check the return value and handle errors
    appropriately.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 20 ++++++++++++++-----
 drivers/staging/rtl8723bs/os_dep/xmit_linux.c |  6 ++++--
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 21690857fd62..cdc7ef1474be 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -603,11 +603,13 @@ static void set_qos(struct pkt_file *ppktfile, struct pkt_attrib *pattrib)
 	s32 UserPriority = 0;
 
 	_rtw_open_pktfile(ppktfile->pkt, ppktfile);
-	_rtw_pktfile_read(ppktfile, (unsigned char *)&etherhdr, ETH_HLEN);
+	if (_rtw_pktfile_read(ppktfile, (unsigned char *)&etherhdr, ETH_HLEN) == 0)
+		return;
 
 	/*  get UserPriority from IP hdr */
 	if (pattrib->ether_type == 0x0800) {
-		_rtw_pktfile_read(ppktfile, (u8 *)&ip_hdr, sizeof(ip_hdr));
+		if (_rtw_pktfile_read(ppktfile, (u8 *)&ip_hdr, sizeof(ip_hdr)) == 0)
+			return;
 		UserPriority = ip_hdr.tos >> 5;
 	}
 	pattrib->priority = UserPriority;
@@ -628,7 +630,8 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
 	signed int res = _SUCCESS;
 
 	_rtw_open_pktfile(pkt, &pktfile);
-	_rtw_pktfile_read(&pktfile, (u8 *)&etherhdr, ETH_HLEN);
+	if (_rtw_pktfile_read(&pktfile, (u8 *)&etherhdr, ETH_HLEN) == 0)
+		return _FAIL;
 
 	pattrib->ether_type = ntohs(etherhdr.h_proto);
 
@@ -655,7 +658,8 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
 
 		u8 tmp[24];
 
-		_rtw_pktfile_read(&pktfile, &tmp[0], 24);
+		if (_rtw_pktfile_read(&pktfile, &tmp[0], 24) == 0)
+			return _FAIL;
 
 		pattrib->dhcp_pkt = 0;
 		if (pktfile.pkt_len > 282) {/* MINIMUM_DHCP_PACKET_SIZE) { */
@@ -1054,7 +1058,8 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, struct
 	}
 
 	_rtw_open_pktfile(pkt, &pktfile);
-	_rtw_pktfile_read(&pktfile, NULL, pattrib->pkt_hdrlen);
+	if (_rtw_pktfile_read(&pktfile, NULL, pattrib->pkt_hdrlen) == 0)
+		return _FAIL;
 
 	frg_inx = 0;
 	frg_len = pxmitpriv->frag_len - 4;/* 2346-4 = 2342 */
@@ -1096,6 +1101,11 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, struct
 			mem_sz = _rtw_pktfile_read(&pktfile, pframe, mpdu_len);
 		}
 
+		if (mem_sz == 0) {
+			res = _FAIL;
+			goto exit;
+		}
+
 		pframe += mem_sz;
 
 		if ((pattrib->icv_len > 0) && (pattrib->bswenc)) {
diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
index 944b9c724b32..0720aa7cad62 100644
--- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
@@ -28,8 +28,10 @@ uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
 	len =  rtw_remainder_len(pfile);
 	len = (rlen > len) ? len : rlen;
 
-	if (rmem)
-		skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
+	if (rmem) {
+		if (skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len) != 0)
+			return 0;
+	}
 
 	pfile->cur_addr += len;
 	pfile->pkt_len -= len;
-- 
2.43.0

Re: [PATCH] staging: rtl8723bs: fix unchecked return value of skb_copy_bits

[Dan Carpenter]

On Tue, Jan 20, 2026 at 11:50:51AM +0900, Minu Jin wrote:
> diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> index 944b9c724b32..0720aa7cad62 100644
> --- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> @@ -28,8 +28,10 @@ uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
>  	len =  rtw_remainder_len(pfile);
>  	len = (rlen > len) ? len : rlen;
>  
> -	if (rmem)
> -		skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
> +	if (rmem) {
> +		if (skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len) != 0)
> +			return 0;

Zero normally means success.  Propagate the negative error code from
skb_copy_bits().

	ret = skb_copy_bits();
	if (ret)
		return ret;

I understand that this driver already does a lot of crazy stuff, but
it's not a model to follow when we're writing new code.

regards,
dan carpenter