[PATCH] staging: rtl8188eu: fix potential leak in rtw_set_key()

[PATCH] staging: rtl8188eu: fix potential leak in rtw_set_key()

[Christian Engelmayer]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

Fix a potential leak in the error path of rtw_set_key(). In case the requested
algorithm is not supported by the driver, the function returns without
enqueuing or freeing the already allocated command and parameter structs. Use
a centralized exit path and make sure that all memory is freed correctly.
Detected by Coverity - CID 1077716, 1077717.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
Compile tested and applies against branch staging-next of tree
git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
---
 drivers/staging/rtl8188eu/core/rtw_mlme.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 769d4dd..275ae7b 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
@@ -1729,13 +1729,12 @@ int rtw_set_key(struct adapter *adapter, struct security_priv *psecuritypriv, in
 	pcmd = (struct	cmd_obj *)rtw_zmalloc(sizeof(struct	cmd_obj));
 	if (pcmd == NULL) {
 		res = _FAIL;  /* try again */
-		goto exit;
+		goto fail_noobj;
 	}
 	psetkeyparm = (struct setkey_parm *)rtw_zmalloc(sizeof(struct setkey_parm));
 	if (psetkeyparm == NULL) {
-		kfree(pcmd);
 		res = _FAIL;
-		goto exit;
+		goto fail_noparm;
 	}
 
 	_rtw_memset(psetkeyparm, 0, sizeof(struct setkey_parm));
@@ -1784,7 +1783,7 @@ int rtw_set_key(struct adapter *adapter, struct security_priv *psecuritypriv, in
 			 ("\n rtw_set_key:psecuritypriv->dot11PrivacyAlgrthm=%x (must be 1 or 2 or 4 or 5)\n",
 			 psecuritypriv->dot11PrivacyAlgrthm));
 		res = _FAIL;
-		goto exit;
+		goto fail;
 	}
 	pcmd->cmdcode = _SetKey_CMD_;
 	pcmd->parmbuf = (u8 *)psetkeyparm;
@@ -1793,7 +1792,13 @@ int rtw_set_key(struct adapter *adapter, struct security_priv *psecuritypriv, in
 	pcmd->rspsz = 0;
 	_rtw_init_listhead(&pcmd->list);
 	res = rtw_enqueue_cmd(pcmdpriv, pcmd);
-exit:
+	return res;
+
+fail:
+	kfree(psetkeyparm);
+fail_noparm:
+	kfree(pcmd);
+fail_noobj:
 	return res;
 }
 
-- 
1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] staging: rtl8188eu: fix potential leak in rtw_set_key()

[Dan Carpenter]

On Thu, May 01, 2014 at 10:38:11AM +0200, Christian Engelmayer wrote:
> Fix a potential leak in the error path of rtw_set_key(). In case the requested
> algorithm is not supported by the driver, the function returns without
> enqueuing or freeing the already allocated command and parameter structs. Use
> a centralized exit path and make sure that all memory is freed correctly.
> Detected by Coverity - CID 1077716, 1077717.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> ---
> Compile tested and applies against branch staging-next of tree
> git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> ---
>  drivers/staging/rtl8188eu/core/rtw_mlme.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c b/drivers/staging/rtl8188eu/core/rtw_mlme.c
> index 769d4dd..275ae7b 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
> @@ -1729,13 +1729,12 @@ int rtw_set_key(struct adapter *adapter, struct security_priv *psecuritypriv, in
>  	pcmd = (struct	cmd_obj *)rtw_zmalloc(sizeof(struct	cmd_obj));
>  	if (pcmd == NULL) {
>  		res = _FAIL;  /* try again */
> -		goto exit;
> +		goto fail_noobj;

Just return here.  The do nothing goto is misleading because you assume
it is a there for a purpose but then when you read all the way to the
end, it doesn't do anything useful.  It doesn't do anything at all.

The label statements should be based on the thing which is labeled and
not the goto locations.  In this case it should be something like
"err_free_cmd" and "err_free_keyparm".  The "fail" label is too generic
so it's not a good name.

regards,
dan carpenter