Re: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit_priv()

[Dan Carpenter <dan.carpenter@oracle.com>]

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