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

[PATCH v2] 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. Propagating the negative error code from skb_copy_bits() if it 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     | 25 +++++++++++++++----
 .../staging/rtl8723bs/include/xmit_osdep.h    |  2 +-
 drivers/staging/rtl8723bs/os_dep/xmit_linux.c | 12 ++++++---
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 21690857fd62..135bc5432be6 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;
@@ -627,8 +629,12 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
 	struct qos_priv *pqospriv = &pmlmepriv->qospriv;
 	signed int res = _SUCCESS;
 
+	signed int ret;
+
 	_rtw_open_pktfile(pkt, &pktfile);
-	_rtw_pktfile_read(&pktfile, (u8 *)&etherhdr, ETH_HLEN);
+	ret = _rtw_pktfile_read(&pktfile, (u8 *)&etherhdr, ETH_HLEN);
+	if (ret < 0)
+		return ret;
 
 	pattrib->ether_type = ntohs(etherhdr.h_proto);
 
@@ -655,7 +661,9 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
 
 		u8 tmp[24];
 
-		_rtw_pktfile_read(&pktfile, &tmp[0], 24);
+		ret = _rtw_pktfile_read(&pktfile, &tmp[0], 24);
+		if (ret < 0)
+			return ret;
 
 		pattrib->dhcp_pkt = 0;
 		if (pktfile.pkt_len > 282) {/* MINIMUM_DHCP_PACKET_SIZE) { */
@@ -1040,6 +1048,8 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, struct
 	s32 bmcst = is_multicast_ether_addr(pattrib->ra);
 	s32 res = _SUCCESS;
 
+	signed int ret;
+
 	if (!pxmitframe->buf_addr)
 		return _FAIL;
 
@@ -1054,7 +1064,9 @@ 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);
+	ret = _rtw_pktfile_read(&pktfile, NULL, pattrib->pkt_hdrlen);
+	if (ret < 0)
+		return ret;
 
 	frg_inx = 0;
 	frg_len = pxmitpriv->frag_len - 4;/* 2346-4 = 2342 */
@@ -1096,6 +1108,9 @@ 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)
+			return mem_sz;
+
 		pframe += mem_sz;
 
 		if ((pattrib->icv_len > 0) && (pattrib->bswenc)) {
diff --git a/drivers/staging/rtl8723bs/include/xmit_osdep.h b/drivers/staging/rtl8723bs/include/xmit_osdep.h
index 8704dced593a..b68277f3759b 100644
--- a/drivers/staging/rtl8723bs/include/xmit_osdep.h
+++ b/drivers/staging/rtl8723bs/include/xmit_osdep.h
@@ -35,7 +35,7 @@ void rtw_os_xmit_resource_free(struct adapter *padapter, struct xmit_buf *pxmitb
 
 extern uint rtw_remainder_len(struct pkt_file *pfile);
 extern void _rtw_open_pktfile(struct sk_buff *pkt, struct pkt_file *pfile);
-extern uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);
+extern int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);
 extern signed int rtw_endofpktfile(struct pkt_file *pfile);
 
 extern void rtw_os_pkt_complete(struct adapter *padapter, struct sk_buff *pkt);
diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
index 944b9c724b32..5b9b959e23ca 100644
--- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
@@ -21,15 +21,19 @@ void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
 	pfile->cur_buffer = pfile->buf_start;
 }
 
-uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
+int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
 {
-	uint	len = 0;
+	int ret;
+	int len = 0;
 
 	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) {
+		ret = skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
+		if (ret < 0)
+			return ret;
+	}
 
 	pfile->cur_addr += len;
 	pfile->pkt_len -= len;
-- 
2.43.0

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

[Andy Shevchenko]

On Tue, Jan 20, 2026 at 10:22:41PM +0900, Minu Jin wrote:
> The function _rtw_pktfile_read() incorrectly updated the file pointer
> even when skb_copy_bits() failed.
> 
> This patch fixes the issue by:
> 
>     1. Propagating the negative error code from skb_copy_bits() if it 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>
> ---

Ick, and what has been changed? Where is the changelog?

...

>  	struct qos_priv *pqospriv = &pmlmepriv->qospriv;
>  	signed int res = _SUCCESS;

>  
> +	signed int ret;

No way. This should be attached to the definition block, id est no blank lines
should be in between.

> s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, struct

>  	s32 bmcst = is_multicast_ether_addr(pattrib->ra);
>  	s32 res = _SUCCESS;
>  
> +	signed int ret;

Ditto.

...

>  extern uint rtw_remainder_len(struct pkt_file *pfile);
>  extern void _rtw_open_pktfile(struct sk_buff *pkt, struct pkt_file *pfile);
> -extern uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);
> +extern int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen);

Drop 'extern' at the same time. Or even better, submit a followup patch to drop
all of them at once.

...

Also you can submit the patches to convert types:

uint --> unsigned int

signed int --> int // this one is optional, signed int longer, but still well
		   // known standard.  etc...

...

> -uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
> +int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
>  {
> -	uint	len = 0;
> +	int ret;
> +	int len = 0;

Reversed xmas tree ordering?

-- 
With Best Regards,
Andy Shevchenko

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

[Dan Carpenter]

On Tue, Jan 20, 2026 at 10:22:41PM +0900, Minu Jin wrote:
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 21690857fd62..135bc5432be6 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;

Eventually we'll want to return an error code here too.  Make it easy on
our future selves and do it this way:

	ret = _rtw_pktfile_read(ppktfile, (unsigned char *)&etherhdr, ETH_HLEN);
	if (ret < 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;
> @@ -627,8 +629,12 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
>  	struct qos_priv *pqospriv = &pmlmepriv->qospriv;
>  	signed int res = _SUCCESS;
>  
> +	signed int ret;

Don't put a blank line in the middle of the declaration block.  Just do
"int ret;".  Everyone knows "int" is signed.  Don't follow the local
style when the local style is wrong.  Also only use s32 when it's part
of a networking or hardware spec.  Just use int.  Do people imagine that
we'll change int to be unsigned?

Same comments everywhere.

> +
>  	_rtw_open_pktfile(pkt, &pktfile);
> -	_rtw_pktfile_read(&pktfile, (u8 *)&etherhdr, ETH_HLEN);
> +	ret = _rtw_pktfile_read(&pktfile, (u8 *)&etherhdr, ETH_HLEN);
> +	if (ret < 0)
> +		return ret;
>  
>  	pattrib->ether_type = ntohs(etherhdr.h_proto);
>  

[ snip ]

>  extern void rtw_os_pkt_complete(struct adapter *padapter, struct sk_buff *pkt);
> diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> index 944b9c724b32..5b9b959e23ca 100644
> --- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> @@ -21,15 +21,19 @@ void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
>  	pfile->cur_buffer = pfile->buf_start;
>  }
>  
> -uint _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
> +int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, uint rlen)
>  {
> -	uint	len = 0;
> +	int ret;
> +	int len = 0;

No need to change the type of len any more.

regards,
dan carpenter

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

[Andy Shevchenko]

On Tue, Jan 20, 2026 at 04:59:29PM +0300, Dan Carpenter wrote:
> On Tue, Jan 20, 2026 at 10:22:41PM +0900, Minu Jin wrote:

...

> > +	signed int ret;
> 
> Don't put a blank line in the middle of the declaration block.  Just do
> "int ret;".  Everyone knows "int" is signed.  Don't follow the local
> style when the local style is wrong.  Also only use s32 when it's part
> of a networking or hardware spec.  Just use int.  Do people imagine that
> we'll change int to be unsigned?

Not in a far past we changed char to be unsigned :-)

*Yes I know that it's a bit different case.

-- 
With Best Regards,
Andy Shevchenko

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

[Dan Carpenter]

On Tue, Jan 20, 2026 at 04:03:37PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 20, 2026 at 04:59:29PM +0300, Dan Carpenter wrote:
> > On Tue, Jan 20, 2026 at 10:22:41PM +0900, Minu Jin wrote:
> 
> ...
> 
> > > +	signed int ret;
> > 
> > Don't put a blank line in the middle of the declaration block.  Just do
> > "int ret;".  Everyone knows "int" is signed.  Don't follow the local
> > style when the local style is wrong.  Also only use s32 when it's part
> > of a networking or hardware spec.  Just use int.  Do people imagine that
> > we'll change int to be unsigned?
> 
> Not in a far past we changed char to be unsigned :-)
> 
> *Yes I know that it's a bit different case.

Heh.

char was always unsigned on s390 and the s390 devs were really militant
about avoiding declaring variables as "unsigned char" so they'd write
all their arch/s390/ code to look like:

	if (char_variable == 255) {

I guess they were annoyed at the rest of the world who declared their
variables as "char" when it should have been "signed char".  They stuck
to their guns and defeated the rest of us in the end.  It's some kind of
life lesson or potentially a message of hope?  :P

regards,
dan carpenter

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

[Greg KH]

On Tue, Jan 20, 2026 at 05:28:33PM +0300, Dan Carpenter wrote:
> On Tue, Jan 20, 2026 at 04:03:37PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 20, 2026 at 04:59:29PM +0300, Dan Carpenter wrote:
> > > On Tue, Jan 20, 2026 at 10:22:41PM +0900, Minu Jin wrote:
> > 
> > ...
> > 
> > > > +	signed int ret;
> > > 
> > > Don't put a blank line in the middle of the declaration block.  Just do
> > > "int ret;".  Everyone knows "int" is signed.  Don't follow the local
> > > style when the local style is wrong.  Also only use s32 when it's part
> > > of a networking or hardware spec.  Just use int.  Do people imagine that
> > > we'll change int to be unsigned?
> > 
> > Not in a far past we changed char to be unsigned :-)
> > 
> > *Yes I know that it's a bit different case.
> 
> Heh.
> 
> char was always unsigned on s390 and the s390 devs were really militant
> about avoiding declaring variables as "unsigned char" so they'd write
> all their arch/s390/ code to look like:
> 
> 	if (char_variable == 255) {
> 
> I guess they were annoyed at the rest of the world who declared their
> variables as "char" when it should have been "signed char".  They stuck
> to their guns and defeated the rest of us in the end.  It's some kind of
> life lesson or potentially a message of hope?  :P

s390 always is 5-10 years ahead of where everyone else will eventually
get to.  It's fun to watch people slowly realize this :)

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

[Greg KH]

On Tue, Jan 20, 2026 at 10:22:41PM +0900, Minu Jin wrote:
> The function _rtw_pktfile_read() incorrectly updated the file pointer
> even when skb_copy_bits() failed.
> 
> This patch fixes the issue by:
> 
>     1. Propagating the negative error code from skb_copy_bits() if it 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     | 25 +++++++++++++++----
>  .../staging/rtl8723bs/include/xmit_osdep.h    |  2 +-
>  drivers/staging/rtl8723bs/os_dep/xmit_linux.c | 12 ++++++---
>  3 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 21690857fd62..135bc5432be6 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;

No error being returned?  Why not?

thanks,

greg k-h