lkml.org 
[lkml]   [2022]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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@oracle.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

\
 
 \ /
  Last update: 2022-03-31 10:51    [W:0.062 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site