linux-kernel - 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>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 31 Mar 2022 10:35:55 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     xkernel.wang@...mail.com
Cc:     Larry.Finger@...inger.net, phil@...lpotter.co.uk,
        gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] staging: r8188eu: fix potential memory leak in
 _rtw_init_xmit_priv()

On Wed, Mar 30, 2022 at 11:29:22PM +0800, xkernel.wang@...mail.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) {

> +					rtw_os_xmit_resource_free(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> +					pxmitbuf++;
> +				}
> +				goto free_xmitbuf;
>  			}
>  		}
>  
> @@ -153,7 +158,7 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  
>  	if (!pxmitpriv->pallocated_xmit_extbuf) {
>  		res = _FAIL;
> -		goto exit;
> +		goto free_pxmitbuf;
>  	}
>  
>  	pxmitpriv->pxmit_extbuf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_xmit_extbuf), 4);
> @@ -169,8 +174,12 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  
>  		res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, max_xmit_extbuf_size + XMITBUF_ALIGN_SZ);
>  		if (res == _FAIL) {
> -			res = _FAIL;
> -			goto exit;
> +			pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;
> +			for (; i >= 0; i--) {

Same thing here.

> +				rtw_os_xmit_resource_free(padapter, pxmitbuf, (max_xmit_extbuf_size + XMITBUF_ALIGN_SZ));
> +				pxmitbuf++;
> +			}
> +			goto free_xmit_extbuf;
>  		}
>  
>  		list_add_tail(&pxmitbuf->list, &pxmitpriv->free_xmit_extbuf_queue.queue);

[ snip ]

> diff --git a/drivers/staging/r8188eu/os_dep/xmit_linux.c b/drivers/staging/r8188eu/os_dep/xmit_linux.c
> index 565ac5b..7aa39b5 100644
> --- a/drivers/staging/r8188eu/os_dep/xmit_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/xmit_linux.c
> @@ -95,8 +95,14 @@ void rtw_os_xmit_resource_free(struct adapter *padapter,
>  {
>  	int i;
>  
> -	for (i = 0; i < 8; i++)
> +	if (!pxmitbuf->pallocated_buf)
> +		return;
> +
> +	for (i = 0; i < 8; i++) {
> +		if (!pxmitbuf->pxmit_urb[i])
> +			break;
>  		usb_free_urb(pxmitbuf->pxmit_urb[i]);
> +	}
>  
>  	kfree(pxmitbuf->pallocated_buf);

No need to modify rtw_os_xmit_resource_free().  Passing a NULL to
usb_free_urb() or kfree() is a no op.

regards,
dan carpenter