[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20220331084920.GB12805@kadam>
Date: Thu, 31 Mar 2022 11:49:20 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: Xiaoke Wang <xkernel.wang@...mail.com>
Cc: "Larry.Finger" <Larry.Finger@...inger.net>,
phil <phil@...lpotter.co.uk>,
gregkh <gregkh@...uxfoundation.org>,
linux-staging <linux-staging@...ts.linux.dev>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in
_rtw_init_xmit_priv()
On Thu, Mar 31, 2022 at 04:21:45PM +0800, Xiaoke Wang wrote:
> On Thu 31 Mar 2022 15:36:21 +0800, dan.carpenter@...cle.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
Powered by blists - more mailing lists