[PATCH v3] staging: rtl8712: fix potential memory leak in _r8712_init_xmit_priv()

[PATCH v3] staging: rtl8712: fix potential memory leak in _r8712_init_xmit_priv()

[xkernel.wang]

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

In _r8712_init_xmit_priv(), if `pxmitbuf->pallocated_buf` is allocated in
failure or `r8712_xmit_resource_alloc(padapter, pxmitbuf)` fails, then it
just returns -ENOMEM but not releases the previous allocated resources,
which can lead to various memory leaks.

To fix them, this patch unifies the error handler in the above function.
Note that if `r8712_xmit_resource_alloc(padapter, pxmitbuf)` fails, we
first call `kfree(pxmitbuf->pallocated_buf);` before goto the error
section so that we do not need to concern the failed item.

As there is no proper device to test with, no runtime testing was
performed.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
ChangeLog:
v1->v2 update the description.
v2->v3 update the description.
 drivers/staging/rtl8712/rtl871x_xmit.c | 27 ++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
index 090345b..dcf3f76 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -117,11 +117,8 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
 	_init_queue(&pxmitpriv->pending_xmitbuf_queue);
 	pxmitpriv->pallocated_xmitbuf =
 		kmalloc(NR_XMITBUFF * sizeof(struct xmit_buf) + 4, GFP_ATOMIC);
-	if (!pxmitpriv->pallocated_xmitbuf) {
-		kfree(pxmitpriv->pallocated_frame_buf);
-		pxmitpriv->pallocated_frame_buf = NULL;
-		return -ENOMEM;
-	}
+	if (!pxmitpriv->pallocated_xmitbuf)
+		goto free_frame_buf;
 	pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
 			      ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
@@ -130,12 +127,14 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
 		pxmitbuf->pallocated_buf =
 			kmalloc(MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ, GFP_ATOMIC);
 		if (!pxmitbuf->pallocated_buf)
-			return -ENOMEM;
+			goto free_xmitbuf;
 		pxmitbuf->pbuf = pxmitbuf->pallocated_buf + XMITBUF_ALIGN_SZ -
 				 ((addr_t) (pxmitbuf->pallocated_buf) &
 				 (XMITBUF_ALIGN_SZ - 1));
-		if (r8712_xmit_resource_alloc(padapter, pxmitbuf))
-			return -ENOMEM;
+		if (r8712_xmit_resource_alloc(padapter, pxmitbuf)) {
+			kfree(pxmitbuf->pallocated_buf);
+			goto free_xmitbuf;
+		}
 		list_add_tail(&pxmitbuf->list,
 				 &(pxmitpriv->free_xmitbuf_queue.queue));
 		pxmitbuf++;
@@ -146,6 +145,18 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
 	init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
 	tasklet_setup(&pxmitpriv->xmit_tasklet, r8712_xmit_bh);
 	return 0;
+
+free_xmitbuf:
+	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
+	while (i--) {
+		r8712_xmit_resource_free(padapter, pxmitbuf);
+		kfree(pxmitbuf->pallocated_buf);
+		pxmitbuf++;
+	}
+	kfree(pxmitpriv->pallocated_xmitbuf);
+free_frame_buf:
+	kfree(pxmitpriv->pallocated_frame_buf);
+	return -ENOMEM;
 }
 
 void _free_xmit_priv(struct xmit_priv *pxmitpriv)
-- 

Re: [PATCH v3] staging: rtl8712: fix potential memory leak in _r8712_init_xmit_priv()

[Dan Carpenter]

On Mon, Sep 26, 2022 at 03:06:05PM +0800, xkernel.wang@foxmail.com wrote:
> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
> index 090345b..dcf3f76 100644
> --- a/drivers/staging/rtl8712/rtl871x_xmit.c
> +++ b/drivers/staging/rtl8712/rtl871x_xmit.c
> @@ -117,11 +117,8 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>  	_init_queue(&pxmitpriv->pending_xmitbuf_queue);
>  	pxmitpriv->pallocated_xmitbuf =
>  		kmalloc(NR_XMITBUFF * sizeof(struct xmit_buf) + 4, GFP_ATOMIC);
> -	if (!pxmitpriv->pallocated_xmitbuf) {
> -		kfree(pxmitpriv->pallocated_frame_buf);
> -		pxmitpriv->pallocated_frame_buf = NULL;
> -		return -ENOMEM;
> -	}
> +	if (!pxmitpriv->pallocated_xmitbuf)
> +		goto free_frame_buf;
>  	pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
>  			      ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
>  	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> @@ -130,12 +127,14 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>  		pxmitbuf->pallocated_buf =
>  			kmalloc(MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ, GFP_ATOMIC);
>  		if (!pxmitbuf->pallocated_buf)
> -			return -ENOMEM;
> +			goto free_xmitbuf;
>  		pxmitbuf->pbuf = pxmitbuf->pallocated_buf + XMITBUF_ALIGN_SZ -
>  				 ((addr_t) (pxmitbuf->pallocated_buf) &
>  				 (XMITBUF_ALIGN_SZ - 1));
> -		if (r8712_xmit_resource_alloc(padapter, pxmitbuf))
> -			return -ENOMEM;
> +		if (r8712_xmit_resource_alloc(padapter, pxmitbuf)) {
> +			kfree(pxmitbuf->pallocated_buf);
> +			goto free_xmitbuf;
> +		}
>  		list_add_tail(&pxmitbuf->list,
>  				 &(pxmitpriv->free_xmitbuf_queue.queue));


pxmitbuf points to somewhere in the middle of pxmitpriv->pallocated_xmitbuf.
We add it to the list here.

>  		pxmitbuf++;
> @@ -146,6 +145,18 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>  	init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>  	tasklet_setup(&pxmitpriv->xmit_tasklet, r8712_xmit_bh);
>  	return 0;
> +
> +free_xmitbuf:
> +	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> +	while (i--) {
> +		r8712_xmit_resource_free(padapter, pxmitbuf);
> +		kfree(pxmitbuf->pallocated_buf);
> +		pxmitbuf++;
> +	}
> +	kfree(pxmitpriv->pallocated_xmitbuf);

But then we free pxmitpriv->pallocated_xmitbuf here but it the memory
is still on the list.  So that means there will be a use after free
eventually.

regards,
dan carpenter

Re: [PATCH v3] staging: rtl8712: fix potential memory leak in _r8712_init_xmit_priv()

[Dan Carpenter]

On Tue, Sep 27, 2022 at 08:55:08PM +0800, Xiaoke Wang wrote:
> 
> Yes, the memory address is still on the list, and at the above position of
> `Note`, the address of `pxmitpriv->pallocated_frame_buf` is also left on a
> list named `pxmitpriv->free_xmit_queue`.
> However, these lists are in `pxmitpriv` and this function is for
> initializing `pxmitpriv`. When this function fails, the probe function of
> this driver will finally fail. So I guess the list in `pxmitpriv` will not
> be accessed.

Sorry for the delayed response.  I think you are right.  This patch is
fine then.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH v3] staging: rtl8712: fix potential memory leak in _r8712_init_xmit_priv()

[Xiaoke Wang]

On Tue, Sep 27, 2022 03:57 PM, dan.carpenter@oracle.com wrote:
> On Mon, Sep 26, 2022 at 03:06:05PM +0800, xkernel.wang@foxmail.com wrote:
>> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
>> index 090345b..dcf3f76 100644
>> --- a/drivers/staging/rtl8712/rtl871x_xmit.c
>> +++ b/drivers/staging/rtl8712/rtl871x_xmit.c
>> @@ -117,11 +117,8 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>> _init_queue(&pxmitpriv->pending_xmitbuf_queue);
>> pxmitpriv->pallocated_xmitbuf =
>>  kmalloc(NR_XMITBUFF * sizeof(struct xmit_buf) + 4, GFP_ATOMIC);
>> - if (!pxmitpriv->pallocated_xmitbuf) {
>> -kfree(pxmitpriv->pallocated_frame_buf);
>> -pxmitpriv->pallocated_frame_buf = NULL;
>> -return -ENOMEM;
>> - }
>> + if (!pxmitpriv->pallocated_xmitbuf)
>> +goto free_frame_buf;

Note here: may have the same concern.

>> pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
>>       ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
>> pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
>> @@ -130,12 +127,14 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>>  pxmitbuf->pallocated_buf =
>> kmalloc(MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ, GFP_ATOMIC);
>>  if (!pxmitbuf->pallocated_buf)
>> - return -ENOMEM;
>> + goto free_xmitbuf;
>>  pxmitbuf->pbuf = pxmitbuf->pallocated_buf + XMITBUF_ALIGN_SZ -
>> ((addr_t) (pxmitbuf->pallocated_buf) &
>> (XMITBUF_ALIGN_SZ - 1));
>> -if (r8712_xmit_resource_alloc(padapter, pxmitbuf))
>> - return -ENOMEM;
>> +if (r8712_xmit_resource_alloc(padapter, pxmitbuf)) {
>> + kfree(pxmitbuf->pallocated_buf);
>> + goto free_xmitbuf;
>> +}
>>  list_add_tail(&pxmitbuf->list,
>> &(pxmitpriv->free_xmitbuf_queue.queue));
>
>
> pxmitbuf points to somewhere in the middle of pxmitpriv->pallocated_xmitbuf.
> We add it to the list here.
>
>>  pxmitbuf++;
>> @@ -146,6 +145,18 @@ int _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>> init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>> tasklet_setup(&pxmitpriv->xmit_tasklet, r8712_xmit_bh);
>> return 0;
>> +
>> +free_xmitbuf:
>> + pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
>> + while (i--) {
>> +r8712_xmit_resource_free(padapter, pxmitbuf);
>> +kfree(pxmitbuf->pallocated_buf);
>> +pxmitbuf++;
>> + }
>> + kfree(pxmitpriv->pallocated_xmitbuf);
>
> But then we free pxmitpriv->pallocated_xmitbuf here but it the memory
> is still on the list.  So that means there will be a use after free
> eventually.

Yes, the memory address is still on the list, and at the above position of
`Note`, the address of `pxmitpriv->pallocated_frame_buf` is also left on a
list named `pxmitpriv->free_xmit_queue`.
However, these lists are in `pxmitpriv` and this function is for
initializing `pxmitpriv`. When this function fails, the probe function of
this driver will finally fail. So I guess the list in `pxmitpriv` will not
be accessed.

Please let me know if you still hold such concerns, I am glad to find a
time (in 2 weeks I guess) to add `list_del_init()` on the error paths
to clear all the improper pointing records.

Regards,
Xiaoke Wang