| Message ID | 20230712205733.29794-1-Larry.Finger@lwfinger.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | staging: 7811: Fix memory leak in _r8712_init_xmit_priv | expand |
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
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
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 --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)
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(-)