[PATCH 0/4] staging: rtl8723bs: Fix error handling and memory

[PATCH 0/4] staging: rtl8723bs: Fix error handling and memory

[Samasth Norway Ananda]

This series fixes several error handling issues and memory leaks in the
rtl8723bs staging driver, found through code review and static analysis.

The patches address the following issues:

1. Firmware memory leak: In rtl8723b_FirmwareDownload(), after
   successfully calling request_firmware(), if subsequent checks fail, 
   the firmware is not released before jumping to the error path.

2. Buffer memory leak: In rtw_cfg80211_inform_bss(), when
   cfg80211_inform_bss_frame() fails, the allocated buffer is not freed.

3. Missing IS_ERR check: kthread_run() returns an ERR_PTR on failure,
   not NULL. The code in rtl8723b_start_thread() was not checking for
   this, which could cause issues when rtl8723b_stop_thread() later
   checks if the thread pointer is non-NULL.

4. Ignored return value: rtw_wdev_alloc() can fail, but its return
   value was being ignored in rtw_sdio_if1_init(), potentially leaving 
   the adapter in an inconsistent state.


Samasth Norway Ananda (4):
  staging: rtl8723bs: fix firmware memory leak on error path
  staging: rtl8723bs: fix memory leak in rtw_cfg80211_inform_bss()
  staging: rtl8723bs: add IS_ERR() check for kthread_run()
  staging: rtl8723bs: check return value of rtw_wdev_alloc()

 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 12 ++++++++++--
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c |  4 +++-
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c      |  3 ++-
 3 files changed, 15 insertions(+), 4 deletions(-)

-- 
2.50.1

[PATCH 1/4] staging: rtl8723bs: fix firmware memory leak on error path

[Samasth Norway Ananda]

Fix memory leak where firmware is not released on error paths in
rtl8723b_FirmwareDownload().

After successfully calling request_firmware(), if the firmware size
check fails or if kmemdup() fails, the code jumps to the exit label
without calling release_firmware(), causing a memory leak.

Add a release_fw label to properly free the firmware in these er:qror
cases. Also add an error message when firmware size exceeds the limit to
help with debugging.

Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
---
 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 57c83f332e74..0eae624a36f0 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -345,14 +345,16 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool  bUsedWoWLANFw)
 	}
 
 	if (fw->size > FW_8723B_SIZE) {
+		pr_err("Firmware size exceed, max: %d, actual: %zu\n",
+		       FW_8723B_SIZE, fw->size);
 		rtStatus = _FAIL;
-		goto exit;
+		goto release_fw;
 	}
 
 	pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL);
 	if (!pFirmware->fw_buffer_sz) {
 		rtStatus = _FAIL;
-		goto exit;
+		goto release_fw;
 	}
 
 	pFirmware->fw_length = fw->size;
@@ -415,6 +417,10 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool  bUsedWoWLANFw)
 		goto fwdl_stat;
 
 fwdl_stat:
+	goto exit;
+
+release_fw:
+	release_firmware(fw);
 
 exit:
 	kfree(pFirmware->fw_buffer_sz);
-- 
2.50.1

Re: [PATCH 1/4] staging: rtl8723bs: fix firmware memory leak on error path

[Dan Carpenter]

On Wed, Dec 17, 2025 at 05:14:11PM -0800, Samasth Norway Ananda wrote:
> Fix memory leak where firmware is not released on error paths in
> rtl8723b_FirmwareDownload().
> 
> After successfully calling request_firmware(), if the firmware size
> check fails or if kmemdup() fails, the code jumps to the exit label
> without calling release_firmware(), causing a memory leak.
> 
> Add a release_fw label to properly free the firmware in these er:qror
> cases. Also add an error message when firmware size exceeds the limit to
> help with debugging.
> 
> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
> ---
>  drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> index 57c83f332e74..0eae624a36f0 100644
> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> @@ -345,14 +345,16 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool  bUsedWoWLANFw)
>  	}
>  
>  	if (fw->size > FW_8723B_SIZE) {
> +		pr_err("Firmware size exceed, max: %d, actual: %zu\n",
> +		       FW_8723B_SIZE, fw->size);
>  		rtStatus = _FAIL;
> -		goto exit;
> +		goto release_fw;
>  	}
>  
>  	pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL);
>  	if (!pFirmware->fw_buffer_sz) {
>  		rtStatus = _FAIL;
> -		goto exit;
> +		goto release_fw;
>  	}
>  
>  	pFirmware->fw_length = fw->size;
> @@ -415,6 +417,10 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool  bUsedWoWLANFw)
>  		goto fwdl_stat;
>  
>  fwdl_stat:
> +	goto exit;

What's the point of this nonsense label the just does another goto?

regards,
dan carpenter

> +
> +release_fw:
> +	release_firmware(fw);
>  
>  exit:
>  	kfree(pFirmware->fw_buffer_sz);
> -- 
> 2.50.1
> 

Re: [External] : Re: [PATCH 1/4] staging: rtl8723bs: fix firmware memory leak on error path

[samasth.norway.ananda]

On 12/17/25 11:14 PM, Dan Carpenter wrote:
> On Wed, Dec 17, 2025 at 05:14:11PM -0800, Samasth Norway Ananda wrote:
>> Fix memory leak where firmware is not released on error paths in
>> rtl8723b_FirmwareDownload().
>>
>> After successfully calling request_firmware(), if the firmware size
>> check fails or if kmemdup() fails, the code jumps to the exit label
>> without calling release_firmware(), causing a memory leak.
>>
>> Add a release_fw label to properly free the firmware in these er:qror
>> cases. Also add an error message when firmware size exceeds the limit to
>> help with debugging.
>>
>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
>> ---
>>   drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
>> index 57c83f332e74..0eae624a36f0 100644
>> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
>> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
>> @@ -345,14 +345,16 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool  bUsedWoWLANFw)
>>   	}
>>   
>>   	if (fw->size > FW_8723B_SIZE) {
>> +		pr_err("Firmware size exceed, max: %d, actual: %zu\n",
>> +		       FW_8723B_SIZE, fw->size);
>>   		rtStatus = _FAIL;
>> -		goto exit;
>> +		goto release_fw;
>>   	}
>>   
>>   	pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL);
>>   	if (!pFirmware->fw_buffer_sz) {
>>   		rtStatus = _FAIL;
>> -		goto exit;
>> +		goto release_fw;
>>   	}
>>   
>>   	pFirmware->fw_length = fw->size;
>> @@ -415,6 +417,10 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool  bUsedWoWLANFw)
>>   		goto fwdl_stat;
>>   
>>   fwdl_stat:
>> +	goto exit;
> 
> What's the point of this nonsense label the just does another goto?

Thanks for the review Dan.
The intermediate label that just does "goto exit" is unnecessary. I will 
rework patch 1 to call release_firmware() directly in each error path 
instead of using an extra label.

regards,
Samasth.

> 
> regards,
> dan carpenter
> 
>> +
>> +release_fw:
>> +	release_firmware(fw);
>>   
>>   exit:
>>   	kfree(pFirmware->fw_buffer_sz);
>> -- 
>> 2.50.1
>>

[PATCH 2/4] staging: rtl8723bs: fix memory leak in rtw_cfg80211_inform_bss()

[Samasth Norway Ananda]

Fix memory leak in rtw_cfg80211_inform_bss() where the allocated buffer
is not freed when cfg80211_inform_bss_frame() fails.

After successfully allocating buf with kzalloc(), if
cfg80211_inform_bss_frame() returns NULL, the code jumps to the exit
label without freeing buf, causing a memory leak.

Add kfree(buf) before the goto to properly free the buffer in this error
case.

Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
---
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 60edeae1cffe..d80e23cfdf8d 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -314,8 +314,10 @@ struct cfg80211_bss *rtw_cfg80211_inform_bss(struct adapter *padapter, struct wl
 	bss = cfg80211_inform_bss_frame(wiphy, notify_channel, (struct ieee80211_mgmt *)buf,
 					len, notify_signal, GFP_ATOMIC);
 
-	if (unlikely(!bss))
+	if (unlikely(!bss)) {
+		kfree(buf);
 		goto exit;
+	}
 
 	cfg80211_put_bss(wiphy, bss);
 	kfree(buf);
-- 
2.50.1

[PATCH 3/4] staging: rtl8723bs: add IS_ERR() check for kthread_run()

[Samasth Norway Ananda]

Add missing error check for kthread_run() in rtl8723b_start_thread().

kthread_run() returns an ERR_PTR on failure, not NULL. Without this
check, rtl8723b_stop_thread() would later check
"if (xmitpriv->SdioXmitThread)" which evaluates to true for error
pointers, potentially causing issues when trying to complete or wait on
an invalid thread.

This follows the same pattern used elsewhere in the driver, such as in
os_dep/os_intfs.c where IS_ERR() is properly checked after kthread_run()
calls.

Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
---
 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 0eae624a36f0..88ea3518bb67 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -2926,6 +2926,8 @@ void rtl8723b_start_thread(struct adapter *padapter)
 	struct xmit_priv *xmitpriv = &padapter->xmitpriv;
 
 	xmitpriv->SdioXmitThread = kthread_run(rtl8723bs_xmit_thread, padapter, "RTWHALXT");
+	if (IS_ERR(xmitpriv->SdioXmitThread))
+		xmitpriv->SdioXmitThread = NULL;
 }
 
 void rtl8723b_stop_thread(struct adapter *padapter)
-- 
2.50.1

[PATCH 4/4] staging: rtl8723bs: check return value of rtw_wdev_alloc()

[Samasth Norway Ananda]

Add missing error check for rtw_wdev_alloc() in rtw_sdio_if1_init().

rtw_wdev_alloc() can fail with -ENOMEM when wiphy_new() or rtw_zmalloc()
fails, or with other negative error codes when wiphy_register() fails.
Without checking the return value, initialization continues even when
wireless device allocation fails, potentially leaving the adapter in an
inconsistent state.

Jump to the error cleanup path when rtw_wdev_alloc() fails to ensure
proper resource cleanup and prevent use of an incompletely initialized
adapter.

Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
---
 drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index 1d0239eef114..432bc6aa1d90 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -296,7 +296,8 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
 	if (rtw_init_drv_sw(padapter) == _FAIL)
 		goto free_hal_data;
 
-	rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj));
+	if (rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj)))
+		goto free_hal_data;
 
 	/* 3 8. get WLan MAC address */
 	/*  set mac addr */
-- 
2.50.1

Re: [PATCH 4/4] staging: rtl8723bs: check return value of rtw_wdev_alloc()

[Dan Carpenter]

On Wed, Dec 17, 2025 at 05:14:14PM -0800, Samasth Norway Ananda wrote:
> Add missing error check for rtw_wdev_alloc() in rtw_sdio_if1_init().
> 
> rtw_wdev_alloc() can fail with -ENOMEM when wiphy_new() or rtw_zmalloc()
> fails, or with other negative error codes when wiphy_register() fails.
> Without checking the return value, initialization continues even when
> wireless device allocation fails, potentially leaving the adapter in an
> inconsistent state.
> 
> Jump to the error cleanup path when rtw_wdev_alloc() fails to ensure
> proper resource cleanup and prevent use of an incompletely initialized
> adapter.
> 
> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
> ---
>  drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> index 1d0239eef114..432bc6aa1d90 100644
> --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> @@ -296,7 +296,8 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
>  	if (rtw_init_drv_sw(padapter) == _FAIL)

This call to rtw_init_drv_sw() does a number of allocations.

>  		goto free_hal_data;
>  
> -	rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj));
> +	if (rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj)))
> +		goto free_hal_data;

So this goto should free them as well.  I have a blog about how to
write cleanup code in a systematic way.

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter

Re: [External] : Re: [PATCH 4/4] staging: rtl8723bs: check return value of rtw_wdev_alloc()

[samasth.norway.ananda]

On 12/17/25 11:12 PM, Dan Carpenter wrote:
> On Wed, Dec 17, 2025 at 05:14:14PM -0800, Samasth Norway Ananda wrote:
>> Add missing error check for rtw_wdev_alloc() in rtw_sdio_if1_init().
>>
>> rtw_wdev_alloc() can fail with -ENOMEM when wiphy_new() or rtw_zmalloc()
>> fails, or with other negative error codes when wiphy_register() fails.
>> Without checking the return value, initialization continues even when
>> wireless device allocation fails, potentially leaving the adapter in an
>> inconsistent state.
>>
>> Jump to the error cleanup path when rtw_wdev_alloc() fails to ensure
>> proper resource cleanup and prevent use of an incompletely initialized
>> adapter.
>>
>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
>> ---
>>   drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
>> index 1d0239eef114..432bc6aa1d90 100644
>> --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
>> +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
>> @@ -296,7 +296,8 @@ static struct adapter *rtw_sdio_if1_init(struct dvobj_priv *dvobj, const struct
>>   	if (rtw_init_drv_sw(padapter) == _FAIL)
> 
> This call to rtw_init_drv_sw() does a number of allocations.
> 
>>   		goto free_hal_data;
>>   
>> -	rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj));
>> +	if (rtw_wdev_alloc(padapter, dvobj_to_dev(dvobj)))
>> +		goto free_hal_data;
> 
> So this goto should free them as well.  I have a blog about how to
> write cleanup code in a systematic way.
> 
> https://urldefense.com/v3/__https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/__;!!ACWV5N9M2RV99hQ!KLPxUw-pZDgh7c5bRyLHFbR9D8QxefnIaHlNgp3DJsspN6yXrwFMCjE6xJIAZhqLWNIzorGl1tTzvpkYINf88wiRyerJrpTr$

Oh okay. Thanks Dan i'll go through it.

regards,
Samasth.

> 
> regards,
> dan carpenter
>