2023-07-12 21:12:08

by Larry Finger

Subject: [PATCH] staging: 7811: Fix memory leak in _r8712_init_xmit_priv

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: [email protected]
Cc: [email protected]
Signed-off-by: Larry Finger <[email protected]>
---
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;
+clean_up_frame_buf:
+ kfree(pxmitpriv->pallocated_frame_buf);
+ pxmitpriv->pallocated_frame_buf = NULL;
+ return -ENOMEM;
}

void _free_xmit_priv(struct xmit_priv *pxmitpriv)
--
2.41.0

2023-07-13 18:53:58

by Larry Finger

Subject: Re: [PATCH] staging: 7811: Fix memory leak in _r8712_init_xmit_priv

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

2023-07-13 20:14:19

by Nam Cao

Subject: Re: [PATCH] staging: 7811: Fix memory leak in _r8712_init_xmit_priv

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