linux-kernel - Re: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in _rtw_init_xmit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

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