diff mbox series

staging: 7811: Fix memory leak in _r8712_init_xmit_priv

Message ID 20230712205733.29794-1-Larry.Finger@lwfinger.net
State New
Headers show
Series staging: 7811: Fix memory leak in _r8712_init_xmit_priv | expand

Commit Message

Larry Finger July 12, 2023, 8:57 p.m. UTC
In the above mentioned routine, memory is allocated in several places.
If the first succeeds and a later one fails, the routine will leak memory.
Fixes commit 2865d42c78a9 ("staging: r8712u: Add the new driver to the
mainline kernel").

Fixes: 2865d42c78a9 ("staging: r8712u: Add the new driver to the mainline kernel")
Reported-by: syzbot+cf71097ffb6755df8251@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/staging/rtl8712/rtl871x_xmit.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Nam Cao July 13, 2023, 5:35 p.m. UTC | #1
On Wed, Jul 12, 2023 at 03:57:32PM -0500, Larry Finger wrote:
> In the above mentioned routine, memory is allocated in several places.
> If the first succeeds and a later one fails, the routine will leak memory.
> Fixes commit 2865d42c78a9 ("staging: r8712u: Add the new driver to the
> mainline kernel").
> 
> Fixes: 2865d42c78a9 ("staging: r8712u: Add the new driver to the mainline kernel")
> Reported-by: syzbot+cf71097ffb6755df8251@syzkaller.appspotmail.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>  drivers/staging/rtl8712/rtl871x_xmit.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
> index 090345bad223..16b815588b97 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 clean_up_frame_buf;
>  	pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
>  			      ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
>  	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> @@ -130,12 +127,12 @@ 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 clean_up_xmit_buf;
>  		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;
> +			goto clean_up_xmit_buf;
>  		list_add_tail(&pxmitbuf->list,
>  				 &(pxmitpriv->free_xmitbuf_queue.queue));
>  		pxmitbuf++;
> @@ -146,6 +143,14 @@ 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;
> +
> +clean_up_xmit_buf:
> +	kfree(pxmitbuf->pallocated_xmitbuf);
> +	pxmitbuf->pallocated_buf = NULL;

The allocation was done in a loop. Shouldn't memory from previous loop iterations
also be freed? And allocation by r8712_xmit_resource_alloc() should be freed too.

Best regards,
Nam
Larry Finger July 13, 2023, 6:51 p.m. UTC | #2
On 7/13/23 12:35, Your Name wrote:
> The allocation was done in a loop. Shouldn't memory from previous loop iterations
> also be freed? And allocation by r8712_xmit_resource_alloc() should be freed too.

Nam,

You are right on both counts. I will prepare version 2.

Larry
Nam Cao July 13, 2023, 8:10 p.m. UTC | #3
On Thu, Jul 13, 2023 at 01:51:00PM -0500, Larry Finger wrote:
> On 7/13/23 12:35, Your Name wrote:
> > The allocation was done in a loop. Shouldn't memory from previous loop iterations
> > also be freed? And allocation by r8712_xmit_resource_alloc() should be freed too.
> 
> Nam,
> 
> You are right on both counts. I will prepare version 2.
> 
> Larry

Perhaps you can make use of my WIP that I didn't have time to finish:
https://github.com/covanam/linux/commit/ee8a0719ec8535f932b30aa67325fc21ba998c50

(Untested and the label names are kind of dumb)

Best regards,
Nam
diff mbox series

Patch

diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c b/drivers/staging/rtl8712/rtl871x_xmit.c
index 090345bad223..16b815588b97 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 clean_up_frame_buf;
 	pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
 			      ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
@@ -130,12 +127,12 @@  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 clean_up_xmit_buf;
 		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;
+			goto clean_up_xmit_buf;
 		list_add_tail(&pxmitbuf->list,
 				 &(pxmitpriv->free_xmitbuf_queue.queue));
 		pxmitbuf++;
@@ -146,6 +143,14 @@  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;
+
+clean_up_xmit_buf:
+	kfree(pxmitbuf->pallocated_xmitbuf);
+	pxmitbuf->pallocated_buf = NULL;
+clean_up_frame_buf:
+	kfree(pxmitpriv->pallocated_frame_buf);
+	pxmitpriv->pallocated_frame_buf = NULL;
+	return -ENOMEM;
 }
 
 void _free_xmit_priv(struct xmit_priv *pxmitpriv)