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

PATCH In review Christian Engelmayer 2 participants 7 messages Apr 28, 2014 – Apr 29, 2014 Staging

lore

Share & tools

lore

Christian Engelmayer

author

2014-04-28 21:04 UTC #0 lore raw

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

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-04-28 21:04 UTC #1 lore raw

PATCH 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

Larry Finger

Acked-by on v1 1/5

2014-04-29 03:11 UTC #2 lore raw

Re: [PATCH 1/5] staging: rtl8188eu: fix potential leak in rtw_wx_read32()

Quoted 5 lines · Christian Engelmayer

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

Acked-by Larry Finger <[email protected]>

This Ack is valid for all 5 patches.

Larry

Quoted 55 lines

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

Christian Engelmayer

author

2014-04-28 21:05 UTC #3 lore raw

PATCH 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-04-28 21:04 UTC #4 lore raw

PATCH 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-04-28 21:05 UTC #5 lore raw

PATCH 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

Christian Engelmayer

author

2014-04-28 21:04 UTC #6 lore raw

PATCH 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