staging: rtl8188eu: fix potential leak in rtw_set_key()

PATCH Superseded Christian Engelmayer 2 participants 2 messages May 1, 2014 Staging

Share & tools

Christian Engelmayer

author

2014-05-01 08:38 UTC #0 lore raw

PATCH 1/1 staging: rtl8188eu: fix potential leak in rtw_set_key()

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 <[email protected]>

---
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(-)

drivers/staging/rtl8188eu/core/rtw_mlme.c +10 -5

index 769d4dd..275ae7b 100644
@@ -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;
 }

Signature · 1 line

1.9.1

Dan Carpenter

2014-05-01 09:09 UTC #1 lore raw

Quoted 19 lines · Christian Engelmayer

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 <[email protected]>
---
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(-)
drivers/staging/rtl8188eu/core/rtw_mlme.c +0 -0
index 769d4dd..275ae7b 100644

drivers/staging/rtl8188eu/core/rtw_mlme.c int rtw_set_key(struct adapter *adapter, struct security_priv *psecuritypriv, in · line 1732

Quoted 5 lines

drivers/staging/rtl8188eu/core/rtw_mlme.c +1 -1

@@ -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