[PATCH v5 0/3] staging: rtl8723bs: improve error handling in _rtw_pktfile_read

[PATCH v5 0/3] staging: rtl8723bs: improve error handling in _rtw_pktfile_read

[Minu Jin]

This patch series improves the error handling and data integrity of 
_rtw_pktfile_read() by preventing unsafe partial reads and ensuring 
proper error propagation.

Previously, _rtw_pktfile_read() allowed partial reads when requested 
data was insufficient, which could lead to incomplete packet parsing 
and potential errors in callers. This series updates the function to 
only update internal pointers when a full read is successful, and 
updates all callers to handle the new error codes correctly.

I do not have the physical hardware to test this, so I have carefully 
reviewed every place where _rtw_pktfile_read() is used in this driver. 
I have also confirmed that the driver builds without any errors or 
warnings with these patches applied.

Changes since v4:
    - Split the single large patch into a 3-patch series as suggested by 
      Greg Kroah-Hartman to ensure each patch performs one logical task.

    - Patch 1: Refactor return type to 'int' and replace non-standard 'uint' 
      parameter with 'unsigned int' for better type consistency and to 
      prepare for error propagation.

    - Patch 2: Add missing error checks and update existing handlers to 
      correctly recognize negative error codes. This includes inserting 
      new checks after _rtw_pktfile_read() and updating existing 
      '== _FAIL' logic to '!= _SUCCESS' to ensure all failures are caught.

    - Patch 3: Implement the core logic to prevent partial reads and 
      return -EINVAL when data is insufficient.

-- 
2.43.0

[PATCH v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Minu Jin]

The current return type of _rtw_pktfile_read() is uint,
which makes it impossible to propagate negative error codes
from internal calls (skb_copy_bits()).

In preparation for returning proper error codes
(eg, skb_copy_bits return -EFAULT when error occurs)
when data is insufficient or copying fails, change the function's
return type to int.

Additionally, update the type of the 'rlen' parameter, 'len' variable
from 'uint' to 'unsigned int' to comply with the kernel coding style
as suggested by Andy Shevchenko.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
 drivers/staging/rtl8723bs/include/xmit_osdep.h |  2 +-
 drivers/staging/rtl8723bs/os_dep/xmit_linux.c  | 14 +++++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8723bs/include/xmit_osdep.h b/drivers/staging/rtl8723bs/include/xmit_osdep.h
index 8704dced593a..880344bffe2f 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);
+int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int 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..ea54a573e025 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, unsigned int rlen)
 {
-	uint	len = 0;
+	unsigned int len;
+	int ret;
 
 	len =  rtw_remainder_len(pfile);
-	len = (rlen > len) ? len : rlen;
+	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 v5 1/3] staging: rtl8723bs: change return type of _rtw_pktfile_read to int

[Dan Carpenter]

On Thu, Jan 22, 2026 at 01:14:48PM +0900, Minu Jin wrote:
> The current return type of _rtw_pktfile_read() is uint,
> which makes it impossible to propagate negative error codes
> from internal calls (skb_copy_bits()).
> 
> In preparation for returning proper error codes
> (eg, skb_copy_bits return -EFAULT when error occurs)
> when data is insufficient or copying fails, change the function's
> return type to int.
> 
> Additionally, update the type of the 'rlen' parameter, 'len' variable
> from 'uint' to 'unsigned int' to comply with the kernel coding style
> as suggested by Andy Shevchenko.
> 
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
>  drivers/staging/rtl8723bs/include/xmit_osdep.h |  2 +-
>  drivers/staging/rtl8723bs/os_dep/xmit_linux.c  | 14 +++++++++-----
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/include/xmit_osdep.h b/drivers/staging/rtl8723bs/include/xmit_osdep.h
> index 8704dced593a..880344bffe2f 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);
> +int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int 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..ea54a573e025 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, unsigned int rlen)
>  {
> -	uint	len = 0;
> +	unsigned int len;
> +	int ret;
>  
>  	len =  rtw_remainder_len(pfile);
> -	len = (rlen > len) ? len : rlen;
> +	len = (rlen > len) ? len : rlen;

How did you generate this diff?  The before and after lines are
exactly the same.

>  
> -	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;

No, you can't do this or it will cause a bug in rtw_xmitframe_coalesce().
So that also means don't change the return type from uint to int in this
patch either.

Normally, we would do the bugfix first and then the cleanup in the
follow on patches.  That way we could backport the fix and leave the
cleanup.  It doesn't matter in a practical sense in this code but
that's the better way.

regards,
dan carpenter

> +	}
>  
>  	pfile->cur_addr += len;
>  	pfile->pkt_len -= len;
> -- 
> 2.43.0

[PATCH v5 2/3] staging: rtl8723bs: update callers to handle negative error codes

[Minu Jin]

Currently, several callers of _rtw_pktfile_read() ignore its return
value. As _rtw_pktfile_read() is updated to return negative error codes
on failure, these return values must be checked to prevent silent failures.

To address this,
this patch implements a proper error propagation mechanism:

1. Add missing error checks: Insert 'if (ret < 0)' checks immediately
   after _rtw_pktfile_read() calls in set_qos(), update_attrib(), and
   rtw_xmitframe_coalesce(). This ensures that any negative error
   code is caught and returned.

2. Enable error propagation: Update set_qos() return type from void to
   int to pass these error codes up the stack instead of swallowing them.

3. Update upper-layer handling: Modify callers like rtw_xmit() and
   xmit_xmitframes() to check for '!= _SUCCESS' instead of '== _FAIL'.
   This allows them to catch both generic failures (0) and the newly
   propagated negative error codes from their respective callees
   (update_attrib and rtw_xmitframe_coalesce).

Specific changes include:
- set_qos():
	Change return type to int and add check for negative returns.

- update_attrib(), rtw_xmitframe_coalesce():
	Add check for negative returns.

- rtw_xmit(), xmit_xmitframes(): Update
	error check logic to '!= _SUCCESS'.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 44 ++++++++++++++-----
 .../staging/rtl8723bs/hal/rtl8723bs_xmit.c    |  6 +--
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 21690857fd62..13eb0e3eae62 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -596,23 +596,31 @@ u8 qos_acm(u8 acm_mask, u8 priority)
 	return priority;
 }
 
-static void set_qos(struct pkt_file *ppktfile, struct pkt_attrib *pattrib)
+static int set_qos(struct pkt_file *ppktfile, struct pkt_attrib *pattrib)
 {
 	struct ethhdr etherhdr;
 	struct iphdr ip_hdr;
 	s32 UserPriority = 0;
+	int ret;
 
 	_rtw_open_pktfile(ppktfile->pkt, ppktfile);
-	_rtw_pktfile_read(ppktfile, (unsigned char *)&etherhdr, ETH_HLEN);
+	ret = _rtw_pktfile_read(ppktfile, (unsigned char *)&etherhdr, ETH_HLEN);
+	if (ret < 0)
+		return ret;
 
 	/*  get UserPriority from IP hdr */
 	if (pattrib->ether_type == 0x0800) {
-		_rtw_pktfile_read(ppktfile, (u8 *)&ip_hdr, sizeof(ip_hdr));
+		ret = _rtw_pktfile_read(ppktfile, (u8 *)&ip_hdr, sizeof(ip_hdr));
+		if (ret < 0)
+			return ret;
+
 		UserPriority = ip_hdr.tos >> 5;
 	}
 	pattrib->priority = UserPriority;
 	pattrib->hdrlen = WLAN_HDR_A3_QOS_LEN;
 	pattrib->subtype = WIFI_QOS_DATA_TYPE;
+
+	return 0;
 }
 
 static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct pkt_attrib *pattrib)
@@ -626,9 +634,12 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 	struct qos_priv *pqospriv = &pmlmepriv->qospriv;
 	signed int res = _SUCCESS;
+	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 +666,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) { */
@@ -737,11 +750,16 @@ static s32 update_attrib(struct adapter *padapter, struct sk_buff *pkt, struct p
 	pattrib->priority = 0;
 
 	if (check_fwstate(pmlmepriv, WIFI_AP_STATE|WIFI_ADHOC_STATE|WIFI_ADHOC_MASTER_STATE)) {
-		if (pattrib->qos_en)
-			set_qos(&pktfile, pattrib);
+		if (pattrib->qos_en) {
+			ret = set_qos(&pktfile, pattrib);
+			if (ret < 0)
+				return ret;
+		}
 	} else {
 		if (pqospriv->qos_option) {
-			set_qos(&pktfile, pattrib);
+			ret = set_qos(&pktfile, pattrib);
+			if (ret < 0)
+				return ret;
 
 			if (pmlmepriv->acm_mask != 0)
 				pattrib->priority = qos_acm(pmlmepriv->acm_mask, pattrib->priority);
@@ -1039,6 +1057,7 @@ s32 rtw_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, struct
 
 	s32 bmcst = is_multicast_ether_addr(pattrib->ra);
 	s32 res = _SUCCESS;
+	int ret;
 
 	if (!pxmitframe->buf_addr)
 		return _FAIL;
@@ -1054,7 +1073,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 +1117,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)) {
@@ -1958,7 +1982,7 @@ s32 rtw_xmit(struct adapter *padapter, struct sk_buff **ppkt)
 
 	res = update_attrib(padapter, *ppkt, &pxmitframe->attrib);
 
-	if (res == _FAIL) {
+	if (res != _SUCCESS) {
 		rtw_free_xmitframe(pxmitpriv, pxmitframe);
 		return -1;
 	}
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c b/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
index abb6fdfe7e1f..55df66ec5f4c 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
@@ -289,10 +289,10 @@ static s32 xmit_xmitframes(struct adapter *padapter, struct xmit_priv *pxmitpriv
 				pxmitframe->buf_addr = pxmitbuf->ptail;
 
 				ret = rtw_xmitframe_coalesce(padapter, pxmitframe->pkt, pxmitframe);
-				if (ret == _FAIL) {
+				if (ret != _SUCCESS) {
 					netdev_err(padapter->pnetdev,
-						   "%s: coalesce FAIL!",
-						   __func__);
+						   "%s: coalesce failed with error %d\n",
+						   __func__, ret);
 					/*  Todo: error handler */
 				} else {
 					k++;
-- 
2.43.0

[PATCH v5 3/3] staging: rtl8723bs: prevent partial reads in _rtw_pktfile_read

[Minu Jin]

The current implementation of _rtw_pktfile_read() allows reading less
data than requested if there isn't enough data remaining.

This is problematic because callers usually request a fixed size (like
a header size) and expect that full amount. Reading only part of the
data means the caller gets incomplete information, which can lead to
errors in packet processing.

To fix this, update the function to:
1. Return -EINVAL if the remaining data is smaller than the requested
   length.
2. Check the return value of skb_copy_bits() and propagate errors.
3. Only update the internal pointers (cur_addr, pkt_len) if the read
   is fully successful.

Callers have already been updated in previous patches to handle these
negative error codes.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
 drivers/staging/rtl8723bs/os_dep/xmit_linux.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
index ea54a573e025..72cf8cd5f7c6 100644
--- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
@@ -23,21 +23,20 @@ void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
 
 int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen)
 {
-	unsigned int len;
 	int ret;
 
-	len =  rtw_remainder_len(pfile);
- 	len = (rlen > len) ? len : rlen;
+	if (rtw_remainder_len(pfile) < rlen)
+		return -EINVAL;
 
 	if (rmem) {
-		ret = skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, len);
+		ret = skb_copy_bits(pfile->pkt, pfile->buf_len - pfile->pkt_len, rmem, rlen);
 		if (ret < 0)
 			return ret;
 	}
 
-	pfile->cur_addr += len;
-	pfile->pkt_len -= len;
-	return len;
+	pfile->cur_addr += rlen;
+	pfile->pkt_len -= rlen;
+	return rlen;
 }
 
 signed int rtw_endofpktfile(struct pkt_file *pfile)
-- 
2.43.0

Re: [PATCH v5 3/3] staging: rtl8723bs: prevent partial reads in _rtw_pktfile_read

[Dan Carpenter]

On Thu, Jan 22, 2026 at 01:14:50PM +0900, Minu Jin wrote:
> The current implementation of _rtw_pktfile_read() allows reading less
> data than requested if there isn't enough data remaining.
> 
> This is problematic because callers usually request a fixed size (like
> a header size) and expect that full amount. Reading only part of the
> data means the caller gets incomplete information, which can lead to
> errors in packet processing.
> 
> To fix this, update the function to:
> 1. Return -EINVAL if the remaining data is smaller than the requested
>    length.
> 2. Check the return value of skb_copy_bits() and propagate errors.
> 3. Only update the internal pointers (cur_addr, pkt_len) if the read
>    is fully successful.
> 
> Callers have already been updated in previous patches to handle these
> negative error codes.
> 
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
>  drivers/staging/rtl8723bs/os_dep/xmit_linux.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> index ea54a573e025..72cf8cd5f7c6 100644
> --- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> @@ -23,21 +23,20 @@ void _rtw_open_pktfile(struct sk_buff *pktptr, struct pkt_file *pfile)
>  
>  int _rtw_pktfile_read(struct pkt_file *pfile, u8 *rmem, unsigned int rlen)
>  {
> -	unsigned int len;
>  	int ret;
>  
> -	len =  rtw_remainder_len(pfile);
> - 	len = (rlen > len) ? len : rlen;

The rest of this looks okay.  But just drop patch 1 since you
delete the len variable in the end anyway.  Leave the type alone and
leave the whitespace alone.  Your subjects are too vague.

patch 1: update _rtw_pktfile_read() to return error codes
patch 2: clean up _rtw_pktfile_read()

regards,
dan carpenter