[PATCH 0/5] staging: rtl8188eu: fix Coverity defects in os_dep/ioctl_linux.c

[PATCH 0/5] staging: rtl8188eu: fix Coverity defects in os_dep/ioctl_linux.c

[Christian Engelmayer]

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

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

-- 
1.9.1

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

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

[Christian Engelmayer]

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

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 <cengelma@gmx.at>
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index cf30a08..45b47e2 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -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,
-- 
1.9.1

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

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

[Christian Engelmayer]

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

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 <cengelma@gmx.at>
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index 45b47e2..1bd476d 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -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;
 }
-- 
1.9.1


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

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

[Christian Engelmayer]

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

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 <cengelma@gmx.at>
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index 1bd476d..8b1579b 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -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,
-- 
1.9.1


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

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

[Christian Engelmayer]

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

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 <cengelma@gmx.at>
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index 8b1579b..ea5e1f8 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -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,
-- 
1.9.1

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

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

[Christian Engelmayer]

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

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 <cengelma@gmx.at>
---
 drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 28 ++++++++++++++++----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
index ea5e1f8..f04aaa3 100644
--- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
@@ -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,
-- 
1.9.1

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

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

[Larry Finger]

On 04/28/2014 03:56 PM, Christian Engelmayer wrote:
> 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 <cengelma@gmx.at>

Acked-by: Larry Finger <Larry.Finger@lwfinger.net>

This Ack is valid for all 5 patches.

Larry

> ---
>   drivers/staging/rtl8188eu/os_dep/ioctl_linux.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> index cf30a08..45b47e2 100644
> --- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
> @@ -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,
>