[PATCH 1/2] staging: r8188eu: properly handle the kzalloc()

[PATCH 1/2] staging: r8188eu: properly handle the kzalloc()

[xkernel.wang]

From: Xiaoke Wang <xkernel.wang@foxmail.com>

kzalloc() is a memory allocation function which can return NULL when
some internal memory errors happen. So it is better to handle the return
of it to prevent potential wrong memory access.
For the kzalloc() in go_add_group_info_attr(), since there is a lack
of error handlers along the call chain it lies and the lifetime of
`pdata_attr` is only in go_add_group_info_attr(), `pdata_attr` is roughly
changed to a local variable on stack like the other functions in 
rtw_p2p.c, such as `u8 p2pie[MAX_P2P_IE_LEN] = { 0x00 };` in 
issue_p2p_presence_resp().

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 drivers/staging/r8188eu/core/rtw_p2p.c     |  6 ++----
 drivers/staging/r8188eu/core/rtw_xmit.c    | 12 +++++++++---
 drivers/staging/r8188eu/include/rtw_xmit.h |  2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
index e2b6cf2..f1a5df8 100644
--- a/drivers/staging/r8188eu/core/rtw_p2p.c
+++ b/drivers/staging/r8188eu/core/rtw_p2p.c
@@ -27,15 +27,14 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
 	struct list_head *phead, *plist;
 	u32 len = 0;
 	u16 attr_len = 0;
-	u8 tmplen, *pdata_attr, *pstart, *pcur;
+	u8 pdata_attr[MAX_P2P_IE_LEN] = { 0x00 };
+	u8 tmplen, *pstart, *pcur;
 	struct sta_info *psta = NULL;
 	struct adapter *padapter = pwdinfo->padapter;
 	struct sta_priv *pstapriv = &padapter->stapriv;
 
 	DBG_88E("%s\n", __func__);
 
-	pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
-
 	pstart = pdata_attr;
 	pcur = pdata_attr;
 
@@ -106,7 +105,6 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
 	if (attr_len > 0)
 		len = rtw_set_p2p_attr_content(pbuf, P2P_ATTR_GROUP_INFO, attr_len, pdata_attr);
 
-	kfree(pdata_attr);
 	return len;
 }
 
diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index 46fe62c..5888979 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -179,7 +179,9 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 
 	pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
 
-	rtw_alloc_hwxmits(padapter);
+	res = rtw_alloc_hwxmits(padapter);
+	if (res == _FAIL)
+		goto exit;
 	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
 
 	for (i = 0; i < 4; i++)
@@ -202,7 +204,6 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 	rtw_hal_init_xmit_priv(padapter);
 
 exit:
-
 	return res;
 }
 
@@ -1516,7 +1517,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
 	return res;
 }
 
-void rtw_alloc_hwxmits(struct adapter *padapter)
+s32 rtw_alloc_hwxmits(struct adapter *padapter)
 {
 	struct hw_xmit *hwxmits;
 	struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
@@ -1525,6 +1526,9 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
 
 	pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
 
+	if (!pxmitpriv->hwxmits)
+		return _FAIL;
+
 	hwxmits = pxmitpriv->hwxmits;
 
 	if (pxmitpriv->hwxmit_entry == 5) {
@@ -1540,6 +1544,8 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
 		hwxmits[3] .sta_queue = &pxmitpriv->bk_pending;
 	} else {
 	}
+
+	return _SUCCESS;
 }
 
 void rtw_free_hwxmits(struct adapter *padapter)
diff --git a/drivers/staging/r8188eu/include/rtw_xmit.h b/drivers/staging/r8188eu/include/rtw_xmit.h
index 5f6e240..b45cd29 100644
--- a/drivers/staging/r8188eu/include/rtw_xmit.h
+++ b/drivers/staging/r8188eu/include/rtw_xmit.h
@@ -345,7 +345,7 @@ s32 rtw_txframes_sta_ac_pending(struct adapter *padapter,
 void rtw_init_hwxmits(struct hw_xmit *phwxmit, int entry);
 s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter);
 void _rtw_free_xmit_priv(struct xmit_priv *pxmitpriv);
-void rtw_alloc_hwxmits(struct adapter *padapter);
+s32 rtw_alloc_hwxmits(struct adapter *padapter);
 void rtw_free_hwxmits(struct adapter *padapter);
 s32 rtw_xmit(struct adapter *padapter, struct sk_buff **pkt);
 
-- 

[PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit_priv()

[xkernel.wang]

From: Xiaoke Wang <xkernel.wang@foxmail.com>

In _rtw_init_xmit_priv(), there are several error paths for allocation
failures without releasing the resources.
To properly release them, there are several error lables and some
modifications for rtw_os_xmit_resource_free().
The `for(; i >= 0; i--)` is to only release the explored items.
While the modifications for rtw_os_xmit_resource_free() is 
corresponding to the logic of rtw_os_xmit_resource_alloc() to break 
unintentional wrong operations. 

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 drivers/staging/r8188eu/core/rtw_xmit.c     | 41 ++++++++++++++++++---
 drivers/staging/r8188eu/os_dep/xmit_linux.c |  8 +++-
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index 5888979..813bddf 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -112,7 +112,7 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 
 	if (!pxmitpriv->pallocated_xmitbuf) {
 		res = _FAIL;
-		goto exit;
+		goto free_frame_buf;
 	}
 
 	pxmitpriv->pxmitbuf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_xmitbuf), 4);
@@ -134,7 +134,12 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 			msleep(10);
 			res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
 			if (res == _FAIL) {
-				goto exit;
+				pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
+				for (; i >= 0; i--) {
+					rtw_os_xmit_resource_free(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
+					pxmitbuf++;
+				}
+				goto free_xmitbuf;
 			}
 		}
 
@@ -153,7 +158,7 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 
 	if (!pxmitpriv->pallocated_xmit_extbuf) {
 		res = _FAIL;
-		goto exit;
+		goto free_pxmitbuf;
 	}
 
 	pxmitpriv->pxmit_extbuf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_xmit_extbuf), 4);
@@ -169,8 +174,12 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 
 		res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, max_xmit_extbuf_size + XMITBUF_ALIGN_SZ);
 		if (res == _FAIL) {
-			res = _FAIL;
-			goto exit;
+			pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;
+			for (; i >= 0; i--) {
+				rtw_os_xmit_resource_free(padapter, pxmitbuf, (max_xmit_extbuf_size + XMITBUF_ALIGN_SZ));
+				pxmitbuf++;
+			}
+			goto free_xmit_extbuf;
 		}
 
 		list_add_tail(&pxmitbuf->list, &pxmitpriv->free_xmit_extbuf_queue.queue);
@@ -181,7 +190,7 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 
 	res = rtw_alloc_hwxmits(padapter);
 	if (res == _FAIL)
-		goto exit;
+		goto free_pxmit_extbuf;
 	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
 
 	for (i = 0; i < 4; i++)
@@ -203,6 +212,26 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 
 	rtw_hal_init_xmit_priv(padapter);
 
+	return res;
+
+free_pxmit_extbuf:
+	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;
+	for (i = 0; i < num_xmit_extbuf; i++) {
+		rtw_os_xmit_resource_free(padapter, pxmitbuf, (max_xmit_extbuf_size + XMITBUF_ALIGN_SZ));
+		pxmitbuf++;
+	}
+free_xmit_extbuf:
+	vfree(pxmitpriv->pallocated_xmit_extbuf);
+free_pxmitbuf:
+	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
+	for (i = 0; i < NR_XMITBUFF; i++) {
+		rtw_os_xmit_resource_free(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
+		pxmitbuf++;
+	}
+free_xmitbuf:
+	vfree(pxmitpriv->pallocated_xmitbuf);
+free_frame_buf:
+	vfree(pxmitpriv->pallocated_frame_buf);
 exit:
 	return res;
 }
diff --git a/drivers/staging/r8188eu/os_dep/xmit_linux.c b/drivers/staging/r8188eu/os_dep/xmit_linux.c
index 565ac5b..7aa39b5 100644
--- a/drivers/staging/r8188eu/os_dep/xmit_linux.c
+++ b/drivers/staging/r8188eu/os_dep/xmit_linux.c
@@ -95,8 +95,14 @@ void rtw_os_xmit_resource_free(struct adapter *padapter,
 {
 	int i;
 
-	for (i = 0; i < 8; i++)
+	if (!pxmitbuf->pallocated_buf)
+		return;
+
+	for (i = 0; i < 8; i++) {
+		if (!pxmitbuf->pxmit_urb[i])
+			break;
 		usb_free_urb(pxmitbuf->pxmit_urb[i]);
+	}
 
 	kfree(pxmitbuf->pallocated_buf);
 }
-- 

Re: [PATCH 1/2] staging: r8188eu: properly handle the kzalloc()

[Greg KH]

On Wed, Mar 30, 2022 at 11:16:07PM +0800, xkernel.wang@foxmail.com wrote:
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> kzalloc() is a memory allocation function which can return NULL when
> some internal memory errors happen. So it is better to handle the return
> of it to prevent potential wrong memory access.
> For the kzalloc() in go_add_group_info_attr(), since there is a lack
> of error handlers along the call chain it lies and the lifetime of
> `pdata_attr` is only in go_add_group_info_attr(), `pdata_attr` is roughly
> changed to a local variable on stack like the other functions in 
> rtw_p2p.c, such as `u8 p2pie[MAX_P2P_IE_LEN] = { 0x00 };` in 
> issue_p2p_presence_resp().
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_p2p.c     |  6 ++----
>  drivers/staging/r8188eu/core/rtw_xmit.c    | 12 +++++++++---
>  drivers/staging/r8188eu/include/rtw_xmit.h |  2 +-
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index e2b6cf2..f1a5df8 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -27,15 +27,14 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
>  	struct list_head *phead, *plist;
>  	u32 len = 0;
>  	u16 attr_len = 0;
> -	u8 tmplen, *pdata_attr, *pstart, *pcur;
> +	u8 pdata_attr[MAX_P2P_IE_LEN] = { 0x00 };

You just created a huge variable on the stack.  Are you _SURE_ that is
ok?

Have you tested this change to make sure it works?  If not, I can't take
it, sorry.

greg k-h

Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit_priv()

[Greg KH]

On Wed, Mar 30, 2022 at 11:29:22PM +0800, xkernel.wang@foxmail.com wrote:
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> In _rtw_init_xmit_priv(), there are several error paths for allocation
> failures without releasing the resources.
> To properly release them, there are several error lables and some
> modifications for rtw_os_xmit_resource_free().
> The `for(; i >= 0; i--)` is to only release the explored items.
> While the modifications for rtw_os_xmit_resource_free() is 
> corresponding to the logic of rtw_os_xmit_resource_alloc() to break 
> unintentional wrong operations. 
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_xmit.c     | 41 ++++++++++++++++++---
>  drivers/staging/r8188eu/os_dep/xmit_linux.c |  8 +++-
>  2 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index 5888979..813bddf 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -112,7 +112,7 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  
>  	if (!pxmitpriv->pallocated_xmitbuf) {
>  		res = _FAIL;
> -		goto exit;
> +		goto free_frame_buf;
>  	}
>  
>  	pxmitpriv->pxmitbuf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_xmitbuf), 4);
> @@ -134,7 +134,12 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  			msleep(10);
>  			res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
>  			if (res == _FAIL) {
> -				goto exit;
> +				pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> +				for (; i >= 0; i--) {
> +					rtw_os_xmit_resource_free(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> +					pxmitbuf++;
> +				}

This logic should be at the end of the function, when you are exiting
due to an error.  Don't duplicate it in multiple places, that way is
ensured to get out of sync eventually.

thanks,

greg k-h

Re: [PATCH 1/2] staging: r8188eu: properly handle the kzalloc()

[Dan Carpenter]

On Wed, Mar 30, 2022 at 11:16:07PM +0800, xkernel.wang@foxmail.com wrote:
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index e2b6cf2..f1a5df8 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -27,15 +27,14 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
>  	struct list_head *phead, *plist;
>  	u32 len = 0;
>  	u16 attr_len = 0;
> -	u8 tmplen, *pdata_attr, *pstart, *pcur;
> +	u8 pdata_attr[MAX_P2P_IE_LEN] = { 0x00 };
> +	u8 tmplen, *pstart, *pcur;
>  	struct sta_info *psta = NULL;
>  	struct adapter *padapter = pwdinfo->padapter;
>  	struct sta_priv *pstapriv = &padapter->stapriv;
>  
>  	DBG_88E("%s\n", __func__);
>  
> -	pdata_attr = kzalloc(MAX_P2P_IE_LEN, GFP_KERNEL);
> -
>  	pstart = pdata_attr;
>  	pcur = pdata_attr;
>  
> @@ -106,7 +105,6 @@ static u32 go_add_group_info_attr(struct wifidirect_info *pwdinfo, u8 *pbuf)
>  	if (attr_len > 0)
>  		len = rtw_set_p2p_attr_content(pbuf, P2P_ATTR_GROUP_INFO, attr_len, pdata_attr);
>  
> -	kfree(pdata_attr);
>  	return len;
>  }
>  

This part is fine.  This change is can be pulled into a separate patch
and reviewed by itself.

> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index 46fe62c..5888979 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -179,7 +179,9 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  
>  	pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>  
> -	rtw_alloc_hwxmits(padapter);
> +	res = rtw_alloc_hwxmits(padapter);
> +	if (res == _FAIL)
> +		goto exit;

There needs to be some cleanup if rtw_alloc_hwxmits() fails.

>  	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>  
>  	for (i = 0; i < 4; i++)
> @@ -202,7 +204,6 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  	rtw_hal_init_xmit_priv(padapter);
>  
>  exit:
> -
>  	return res;

This whitespace change has nothing to do with allocating memory.  Leave
it out.

>  }
>  
> @@ -1516,7 +1517,7 @@ s32 rtw_xmit_classifier(struct adapter *padapter, struct xmit_frame *pxmitframe)
>  	return res;
>  }
>  
> -void rtw_alloc_hwxmits(struct adapter *padapter)
> +s32 rtw_alloc_hwxmits(struct adapter *padapter)
>  {
>  	struct hw_xmit *hwxmits;
>  	struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
> @@ -1525,6 +1526,9 @@ void rtw_alloc_hwxmits(struct adapter *padapter)
>  
>  	pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
>  
> +	if (!pxmitpriv->hwxmits)

Don't leave a blank line between the allocation and the check.

> +		return _FAIL;
> +
>  	hwxmits = pxmitpriv->hwxmits;
>  
>  	if (pxmitpriv->hwxmit_entry == 5) {

regards,
dan carpenter

Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit_priv()

[Dan Carpenter]

On Wed, Mar 30, 2022 at 11:29:22PM +0800, xkernel.wang@foxmail.com wrote:
> @@ -134,7 +134,12 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  			msleep(10);
>  			res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
>  			if (res == _FAIL) {
> -				goto exit;
> +				pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> +				for (; i >= 0; i--) {

This frees one more element than you intended.  It should be:

	while (--i >= 0) {

> +					rtw_os_xmit_resource_free(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> +					pxmitbuf++;
> +				}
> +				goto free_xmitbuf;
>  			}
>  		}
>  
> @@ -153,7 +158,7 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  
>  	if (!pxmitpriv->pallocated_xmit_extbuf) {
>  		res = _FAIL;
> -		goto exit;
> +		goto free_pxmitbuf;
>  	}
>  
>  	pxmitpriv->pxmit_extbuf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_xmit_extbuf), 4);
> @@ -169,8 +174,12 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  
>  		res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, max_xmit_extbuf_size + XMITBUF_ALIGN_SZ);
>  		if (res == _FAIL) {
> -			res = _FAIL;
> -			goto exit;
> +			pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;
> +			for (; i >= 0; i--) {

Same thing here.

> +				rtw_os_xmit_resource_free(padapter, pxmitbuf, (max_xmit_extbuf_size + XMITBUF_ALIGN_SZ));
> +				pxmitbuf++;
> +			}
> +			goto free_xmit_extbuf;
>  		}
>  
>  		list_add_tail(&pxmitbuf->list, &pxmitpriv->free_xmit_extbuf_queue.queue);

[ snip ]

> diff --git a/drivers/staging/r8188eu/os_dep/xmit_linux.c b/drivers/staging/r8188eu/os_dep/xmit_linux.c
> index 565ac5b..7aa39b5 100644
> --- a/drivers/staging/r8188eu/os_dep/xmit_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/xmit_linux.c
> @@ -95,8 +95,14 @@ void rtw_os_xmit_resource_free(struct adapter *padapter,
>  {
>  	int i;
>  
> -	for (i = 0; i < 8; i++)
> +	if (!pxmitbuf->pallocated_buf)
> +		return;
> +
> +	for (i = 0; i < 8; i++) {
> +		if (!pxmitbuf->pxmit_urb[i])
> +			break;
>  		usb_free_urb(pxmitbuf->pxmit_urb[i]);
> +	}
>  
>  	kfree(pxmitbuf->pallocated_buf);

No need to modify rtw_os_xmit_resource_free().  Passing a NULL to
usb_free_urb() or kfree() is a no op.

regards,
dan carpenter

Re: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit_priv()

[Dan Carpenter]

On Thu, Mar 31, 2022 at 04:21:45PM +0800, Xiaoke Wang wrote:
> On Thu 31 Mar 2022 15:36:21 +0800, dan.carpenter@oracle.com wrote:
> >> @@ -134,7 +134,12 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> >>  			msleep(10);
> >>  			res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> >>  			if (res == _FAIL) {
> >> -				goto exit;
> >> +				pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> >> +				for (; i >= 0; i--) {
> >
> > This frees one more element than you intended.  It should be:
> >
> >	 while (--i >= 0) {
> >
> 
> In fact, this is considering that we do not know where is the failure
> from. In rtw_os_xmit_resource_alloc(), the failure can from 
> 
> > pxmitbuf->pallocated_buf = kzalloc(alloc_sz, GFP_KERNEL);
> 
> , but also can from 
> 
> > 		pxmitbuf->pxmit_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> 
> So if we do not handle the current failed item and just skip it, then some
> memory may be ignored.

The rtw_os_xmit_resource_alloc() function should clean up after itself
and not leave things partially allocated.

First of all MAX_XMITBUF_SZ is 20480 bytes.  It's giant.  We need to
figure out what's up with that.  But then if "pxmitbuf->pxmit_urb[i] =
usb_alloc_urb(0, GFP_KERNEL);" fails, we allocate directly over the
pxmitbuf->pallocated_buf on the second attempt.  So it leaks memory.

Every function should clean up after itself.  No partial allocations.
That always leads to bugs.

regards,
dan carpenter

Re: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit_priv()

[Dan Carpenter]

On Thu, Mar 31, 2022 at 04:33:37PM +0800, Xiaoke Wang wrote:
> 
> On Thu, 31 Mar 2022 16:21:43 +0800, xkernel.wang@foxmail.com wrote:
> >In fact, this is considering that we do not know where is the failure
> >from. In rtw_os_xmit_resource_alloc(), the failure can from 
> >
> >> pxmitbuf->pallocated_buf = kzalloc(alloc_sz, GFP_KERNEL);
> >
> > , but also can from 
> >
> >> 		pxmitbuf->pxmit_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> >
> > So if we do not handle the current failed item and just skip it, then some
> > memory may be ignored.
> 
> 
> 
> Maybe this should be the problem of rtw_os_xmit_resource_alloc() as it
> does properly handle the error from it.
> Anyway, I will attempt to fix them.
> 

It would be a lot easier to fix this code, if it were cleaner.

It goes to a lot of work to ensure that the pointers are aligned at the
4 byte mark, but memory from kmalloc() or vmalloc() is already aligned
at 8 bytes so there is no need.

	pxmitpriv->pallocated_frame_buf = vzalloc(NR_XMITFRAME * sizeof(struct xmit_frame) + 4);

The + 4 is for alignment.

	pxmitpriv->pxmit_frame_buf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_frame_buf), 4);

So then it stores pxmitpriv->pxmit_frame_buf which is the aligned
version of the pointer and the ->pallocated_frame_buf pointer which it
uses to free the memory.  Delete pxmitpriv->pallocated_frame_buf.  Just
allocate it directly.  Use vcalloc().

	pxmitpriv->pxmit_frame_buf = vzalloc(NR_XMITFRAME * sizeof(struct xmit_frame);

That stuff can all be done in one patch because it is:

Patch 1: Remove unnecessary alignment hacks.

This code goes to great lengths to ensure that the buffers are 4 bytes
aligned.  However that is not necessary as the vmalloc() and kmalloc()
functions return memory that is already aligned at 8 bytes.
1) No need for the temporary pxmitpriv->pallocated_frame_buf pointer to
   store unaligned memory.
2) No need to allocate "+ 4" extra bytes for alignment.
3) No need to call N_BYTE_ALIGMENT().

Patch 2: Use vcalloc() instead of vzalloc()
Patch 3: Declare the pxmitpriv->pxmit_frame_buf pointer as an array of
structs instead of an array of u8.  This allows us to remove some
casting.

Once the code is cleaner then the error handling will also be easier.

regards,
dan carpenter