staging: rtl8188eu: fix Coverity defects in os_dep/ioctl_linux.c

PATCH In review Christian Engelmayer 2 participants 7 messages May 7, 2014 Staging

lore

Share & tools

lore

Christian Engelmayer

author

2014-05-07 19:58 UTC #0 lore raw

v2 cover staging: rtl8188eu: fix Coverity defects in os_dep/ioctl_linux.c

This is a cleanup of staging/rtl8188eu/os_dep/ioctl_linux.c regarding Coverity resource leak findings.

The changes leave the current implementation intact and just attack the problems in the error paths, however, it seems that we could get easily rid of some of the mallocs altogether.

   char *input = kmalloc(wrqu->data.length, GFP_KERNEL);
   copy_from_user(input, wrqu->data.pointer, wrqu->data.length);
   qAutoLoad = strncmp(input, "autoload", 8);

v2: Resend after v1 failed to apply

* rebased against staging-next - commit 09c3fbba (staging: rtl8188eu:
Remove 'u8 *pbuf' from struct recv_buf)
* fixed mua: no multipart, 7bit text/plain us-ascii

The series is compile tested and applies against branch staging-next of tree git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

Christian Engelmayer (5):
staging: rtl8188eu: fix potential leak in rtw_wx_read32()
staging: rtl8188eu: fix potential leak in rtw_wx_set_enc_ext()
staging: rtl8188eu: fix potential leak in rtw_mp_QueryDrv()
staging: rtl8188eu: fix potential leak in rtw_mp_SetRFPath()
staging: rtl8188eu: fix potential leak in rtw_mp_pwrtrk()

 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 69 +++++++++++++++++---------
 1 file changed, 45 insertions(+), 24 deletions(-)

Signature · 1 line

1.9.1

Christian Engelmayer

author

2014-05-07 19:53 UTC #1 lore raw

PATCH v2 1/5 staging: rtl8188eu: fix potential leak in rtw_wx_read32()

Function rtw_wx_read32() dynamically allocates a temporary buffer that is not freed in all error paths. Use a centralized exit path and make sure that all memory is freed correctly. Detected by Coverity - CID 1077711.

Signed-off-by Christian Engelmayer <[email protected]>

---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +9 -6

index cf30a08..45b47e2 100644
@@ -2154,6 +2154,7 @@ static int rtw_wx_read32(struct net_device *dev,
 	u32 bytes;
 	u8 *ptmp;
 	int rv;
+	int ret = 0;
 
 	padapter = (struct adapter *)rtw_netdev_priv(dev);
 	p = &wrqu->data;

@@ -2163,16 +2164,16 @@ static int rtw_wx_read32(struct net_device *dev,
 		return -ENOMEM;
 
 	if (copy_from_user(ptmp, p->pointer, len)) {
-		kfree(ptmp);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto exit;
 	}
 
 	bytes = 0;
 	addr = 0;
 	rv = sscanf(ptmp, "%d,%x", &bytes, &addr);
 	if (rv != 2) {
-		kfree(ptmp);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	switch (bytes) {

@@ -2190,12 +2191,14 @@ static int rtw_wx_read32(struct net_device *dev,
 		break;
 	default:
 		DBG_88E(KERN_INFO "%s: usage> read [bytes],[address(hex)]\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit;
 	}
 	DBG_88E(KERN_INFO "%s: addr = 0x%08X data =%s\n", __func__, addr, extra);
 
+exit:
 	kfree(ptmp);
-	return 0;
+	return ret;
 }
 
 static int rtw_wx_write32(struct net_device *dev,

Signature · 1 line

1.9.1

Christian Engelmayer

author

2014-05-07 19:58 UTC #2 lore raw

PATCH v2 2/5 staging: rtl8188eu: fix potential leak in rtw_wx_set_enc_ext()

In reply to Christian Engelmayer

Function rtw_wx_set_enc_ext() dynamically allocates a temporary buffer that is not freed in all error paths. Use a centralized exit path and make sure that all memory is freed correctly. Detected by Coverity - CID 1077712.

Signed-off-by Christian Engelmayer <[email protected]>

---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +3 -1

index 45b47e2..1bd476d 100644
@@ -2097,7 +2097,8 @@ static int rtw_wx_set_enc_ext(struct net_device *dev,
 		alg_name = "CCMP";
 		break;
 	default:
-		return -1;
+		ret = -1;
+		goto exit;
 	}
 
 	strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);

@@ -2124,6 +2125,7 @@ static int rtw_wx_set_enc_ext(struct net_device *dev,
 
 	ret =  wpa_set_encryption(dev, param, param_len);
 
+exit:
 	kfree(param);
 	return ret;
 }

Signature · 1 line

1.9.1

Christian Engelmayer

author

2014-05-07 19:53 UTC #3 lore raw

PATCH v2 3/5 staging: rtl8188eu: fix potential leak in rtw_mp_QueryDrv()

In reply to Christian Engelmayer

Function rtw_mp_QueryDrv() dynamically allocates a temporary buffer that is not freed in all error paths. Use a centralized exit path and make sure that all memory is freed correctly. Detected by Coverity - CID 1077713.

Signed-off-by Christian Engelmayer <[email protected]>

---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +8 -3

index 1bd476d..8b1579b 100644
@@ -7350,12 +7350,15 @@ static int rtw_mp_QueryDrv(struct net_device *dev,
 	char	*input = kmalloc(wrqu->data.length, GFP_KERNEL);
 	u8 qAutoLoad = 1;
 	struct eeprom_priv *pEEPROM = GET_EEPROM_EFUSE_PRIV(padapter);
+	int ret = 0;
 
 	if (!input)
 		return -ENOMEM;
 
-	if (copy_from_user(input, wrqu->data.pointer, wrqu->data.length))
-			return -EFAULT;
+	if (copy_from_user(input, wrqu->data.pointer, wrqu->data.length)) {
+		ret = -EFAULT;
+		goto exit;
+	}
 	DBG_88E("%s:iwpriv in =%s\n", __func__, input);
 
 	qAutoLoad = strncmp(input, "autoload", 8); /*  strncmp true is 0 */

@@ -7369,8 +7372,10 @@ static int rtw_mp_QueryDrv(struct net_device *dev,
 		sprintf(extra, "ok");
 	}
 	wrqu->data.length = strlen(extra) + 1;
+
+exit:
 	kfree(input);
-	return 0;
+	return ret;
 }
 
 static int rtw_mp_set(struct net_device *dev,

Signature · 1 line

1.9.1

Christian Engelmayer

author

2014-05-07 19:53 UTC #4 lore raw

PATCH v2 4/5 staging: rtl8188eu: fix potential leak in rtw_mp_SetRFPath()

In reply to Christian Engelmayer

Function rtw_mp_SetRFPath() dynamically allocates a temporary buffer that is not freed in all error paths. Use a centralized exit path and make sure that all memory is freed correctly. Detected by Coverity - CID 1077714.

Signed-off-by Christian Engelmayer <[email protected]>

---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +8 -3

index 8b1579b..ea5e1f8 100644
@@ -7321,11 +7321,14 @@ static int rtw_mp_SetRFPath(struct net_device *dev,
 	struct adapter *padapter = rtw_netdev_priv(dev);
 	char	*input = kmalloc(wrqu->data.length, GFP_KERNEL);
 	u8 bMain = 1, bTurnoff = 1;
+	int ret = 0;
 
 	if (!input)
 		return -ENOMEM;
-	if (copy_from_user(input, wrqu->data.pointer, wrqu->data.length))
-			return -EFAULT;
+	if (copy_from_user(input, wrqu->data.pointer, wrqu->data.length)) {
+		ret = -EFAULT;
+		goto exit;
+	}
 	DBG_88E("%s:iwpriv in =%s\n", __func__, input);
 
 	bMain = strncmp(input, "1", 2); /*  strncmp true is 0 */

@@ -7338,8 +7341,10 @@ static int rtw_mp_SetRFPath(struct net_device *dev,
 		MP_PHY_SetRFPathSwitch(padapter, false);
 		DBG_88E("%s:PHY_SetRFPathSwitch = false\n", __func__);
 	}
+
+exit:
 	kfree(input);
-	return 0;
+	return ret;
 }
 
 static int rtw_mp_QueryDrv(struct net_device *dev,

Signature · 1 line

1.9.1

Dan Carpenter

2014-05-07 20:08 UTC #5 lore raw

Re: [PATCH v2 4/5] staging: rtl8188eu: fix potential leak in rtw_mp_SetRFPath()

Quoted 14 lines · Christian Engelmayer

Function rtw_mp_SetRFPath() dynamically allocates a temporary buffer that is not freed in all error paths. Use a centralized exit path and make sure that all memory is freed correctly. Detected by Coverity - CID 1077714.
Signed-off-by Christian Engelmayer <[email protected]>
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +0 -0
index 8b1579b..ea5e1f8 100644

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c static int rtw_mp_SetRFPath(struct net_device *dev, · line 7322

Quoted 2 lines

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +0 -0

@@ -7321,11 +7321,14 @@ static int rtw_mp_SetRFPath(struct net_device *dev,
 	struct adapter *padapter = rtw_netdev_priv(dev);
 	char	*input = kmalloc(wrqu->data.length, GFP_KERNEL);

You didn't introduce this so there is no need to resend but doing the allocation in the declaration block is nasty style. No one reads the declaration block and it is a common cause of bugs. For example, the bug you are fixing here. ;)

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c static int rtw_mp_SetRFPath(struct net_device *dev, · line 7324

Quoted 2 lines

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +1 -0
 	u8 bMain = 1, bTurnoff = 1;
+	int ret = 0;

regards,
dan carpenter

Christian Engelmayer

author

2014-05-07 19:54 UTC #6 lore raw

PATCH v2 5/5 staging: rtl8188eu: fix potential leak in rtw_mp_pwrtrk()

In reply to Christian Engelmayer

Function rtw_mp_pwrtrk() dynamically allocates a temporary buffer that is not freed in all error paths. Use a centralized exit path and make sure that all memory is freed correctly. Detected by Coverity - 1077715.

Signed-off-by Christian Engelmayer <[email protected]>

---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 28 ++++++++++++++++----------
 1 file changed, 17 insertions(+), 11 deletions(-)

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +17 ~~-11~~

index ea5e1f8..f04aaa3 100644
@@ -7119,15 +7119,15 @@ static int rtw_mp_pwrtrk(struct net_device *dev,
 {
 	u8 enable;
 	u32 thermal;
-	s32 ret;
 	struct adapter *padapter = rtw_netdev_priv(dev);
 	char	*input = kmalloc(wrqu->length, GFP_KERNEL);
+	int ret = 0;
 
 	if (!input)
 		return -ENOMEM;
 	if (copy_from_user(input, wrqu->pointer, wrqu->length)) {
-		kfree(input);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto exit;
 	}
 	_rtw_memset(extra, 0, wrqu->length);

@@ -7138,22 +7138,28 @@ static int rtw_mp_pwrtrk(struct net_device *dev,
 			sprintf(extra, "mp tx power tracking stop");
 		} else if (sscanf(input, "ther =%d", &thermal)) {
 				ret = Hal_SetThermalMeter(padapter, (u8)thermal);
-				if (ret == _FAIL)
-					return -EPERM;
+				if (ret == _FAIL) {
+					ret = -EPERM;
+					goto exit;
+				}
 				sprintf(extra, "mp tx power tracking start, target value =%d ok ", thermal);
 		} else {
-			kfree(input);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto exit;
 		}
 	}
 
-	kfree(input);
 	ret = Hal_SetPowerTracking(padapter, enable);
-	if (ret == _FAIL)
-		return -EPERM;
+	if (ret == _FAIL) {
+		ret = -EPERM;
+		goto exit;
+	}
 
 	wrqu->length = strlen(extra);
-	return 0;
+
+exit:
+	kfree(input);
+	return ret;
 }
 
 static int rtw_mp_psd(struct net_device *dev,

Signature · 1 line

1.9.1