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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Mar 2022 08:04:02 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     xkernel.wang@...mail.com
Cc:     Larry.Finger@...inger.net, phil@...lpotter.co.uk,
        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:
> From: Xiaoke Wang <xkernel.wang@...mail.com>
> 
> In _rtw_init_xmit_priv(), there are several error paths for allocation
> failures without releasing the resources.
> To properly release them, there are several error lables and some
> modifications for rtw_os_xmit_resource_free().
> The `for(; i >= 0; i--)` is to only release the explored items.
> While the modifications for rtw_os_xmit_resource_free() is 
> corresponding to the logic of rtw_os_xmit_resource_alloc() to break 
> unintentional wrong operations. 
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@...mail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_xmit.c     | 41 ++++++++++++++++++---
>  drivers/staging/r8188eu/os_dep/xmit_linux.c |  8 +++-
>  2 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index 5888979..813bddf 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -112,7 +112,7 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  
>  	if (!pxmitpriv->pallocated_xmitbuf) {
>  		res = _FAIL;
> -		goto exit;
> +		goto free_frame_buf;
>  	}
>  
>  	pxmitpriv->pxmitbuf = (u8 *)N_BYTE_ALIGMENT((size_t)(pxmitpriv->pallocated_xmitbuf), 4);
> @@ -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--) {
> +					rtw_os_xmit_resource_free(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> +					pxmitbuf++;
> +				}

This logic should be at the end of the function, when you are exiting
due to an error.  Don't duplicate it in multiple places, that way is
ensured to get out of sync eventually.

thanks,

greg k-h