[PATCH] staging: rtl8712: fix potential leak in r871x_wx_set_enc_ext()

[PATCH] staging: rtl8712: fix potential leak in r871x_wx_set_enc_ext()

[Christian Engelmayer]

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

Fix a potential leak in the error path of r871x_wx_set_enc_ext(). In case the
requested algorithm is not supported by the driver, the function returns
without freeing the already allocated 'param' struct. Move the input
verification to the beginning of the function so that the direct return is
safe. Detected by Coverity - CID 144373.

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/rtl8712/rtl871x_ioctl_linux.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 23d539d..1eca992 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -1801,13 +1801,6 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
 	u32 param_len;
 	int ret = 0;
 
-	param_len = sizeof(struct ieee_param) + pext->key_len;
-	param = (struct ieee_param *)_malloc(param_len);
-	if (param == NULL)
-		return -ENOMEM;
-	memset(param, 0, param_len);
-	param->cmd = IEEE_CMD_SET_ENCRYPTION;
-	memset(param->sta_addr, 0xff, ETH_ALEN);
 	switch (pext->alg) {
 	case IW_ENCODE_ALG_NONE:
 		alg_name = "none";
@@ -1824,6 +1817,15 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
 	default:
 		return -EINVAL;
 	}
+
+	param_len = sizeof(struct ieee_param) + pext->key_len;
+	param = (struct ieee_param *)_malloc(param_len);
+	if (param == NULL)
+		return -ENOMEM;
+	memset(param, 0, param_len);
+	param->cmd = IEEE_CMD_SET_ENCRYPTION;
+	memset(param->sta_addr, 0xff, ETH_ALEN);
+
 	strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
 	if (pext->ext_flags & IW_ENCODE_EXT_GROUP_KEY)
 		param->u.crypt.set_tx = 0;
-- 
1.9.1

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

Re: [PATCH] staging: rtl8712: fix potential leak in r871x_wx_set_enc_ext()

[Andy Shevchenko]

On Thu, 2014-05-01 at 11:45 +0200, Christian Engelmayer wrote:
> Fix a potential leak in the error path of r871x_wx_set_enc_ext(). In case the
> requested algorithm is not supported by the driver, the function returns
> without freeing the already allocated 'param' struct. Move the input
> verification to the beginning of the function so that the direct return is
> safe. Detected by Coverity - CID 144373.
> 


One comment below.


> 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/rtl8712/rtl871x_ioctl_linux.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 23d539d..1eca992 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -1801,13 +1801,6 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
>  	u32 param_len;
>  	int ret = 0;
>  
> -	param_len = sizeof(struct ieee_param) + pext->key_len;
> -	param = (struct ieee_param *)_malloc(param_len);
> -	if (param == NULL)
> -		return -ENOMEM;
> -	memset(param, 0, param_len);
> -	param->cmd = IEEE_CMD_SET_ENCRYPTION;
> -	memset(param->sta_addr, 0xff, ETH_ALEN);
>  	switch (pext->alg) {
>  	case IW_ENCODE_ALG_NONE:
>  		alg_name = "none";
> @@ -1824,6 +1817,15 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
>  	default:
>  		return -EINVAL;
>  	}
> +
> +	param_len = sizeof(struct ieee_param) + pext->key_len;
> +	param = (struct ieee_param *)_malloc(param_len);

While you are here could you substitute _malloc by kzalloc and remove
explicit casting and memset?

> +	if (param == NULL)
> +		return -ENOMEM;
> +	memset(param, 0, param_len);
> +	param->cmd = IEEE_CMD_SET_ENCRYPTION;
> +	memset(param->sta_addr, 0xff, ETH_ALEN);
> +
>  	strncpy((char *)param->u.crypt.alg, alg_name, IEEE_CRYPT_ALG_NAME_LEN);
>  	if (pext->ext_flags & IW_ENCODE_EXT_GROUP_KEY)
>  		param->u.crypt.set_tx = 0;


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] staging: rtl8712: fix potential leak in r871x_wx_set_enc_ext()

[Dan Carpenter]

On Wed, May 07, 2014 at 09:55:47AM +0300, Andy Shevchenko wrote:
> > @@ -1824,6 +1817,15 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
> >  	default:
> >  		return -EINVAL;
> >  	}
> > +
> > +	param_len = sizeof(struct ieee_param) + pext->key_len;
> > +	param = (struct ieee_param *)_malloc(param_len);
> 
> While you are here could you substitute _malloc by kzalloc and remove
> explicit casting and memset?
> 

Normally, that's the kind of thing we would do in a separate patch
because or the "one thing per patch" rule.  Eventually someone will do a
driver wide replacement of _malloc().  Or if the bug fixer wanted, he
could do it in this patch because it is on the same line and all so it
counts as a "minor closely related change."  Either way is fine.

Another way to say this is that since the _malloc() was there in the
original code and Christian didn't introduce it, then we shouldn't
reject his patch because of it.  This is staging code, and there are so
many problems that if you start trying to fix everything you'll just get
lost.

In my experience v2 patches are much harder to write than v1 patches.
Twice in the past few days I have messed up the subject line in my v2
patches.

regards,
dan carpenter

Re: [PATCH] staging: rtl8712: fix potential leak in r871x_wx_set_enc_ext()

[Andy Shevchenko]

On Wed, 2014-05-07 at 11:55 +0300, Dan Carpenter wrote:
> On Wed, May 07, 2014 at 09:55:47AM +0300, Andy Shevchenko wrote:
> > > @@ -1824,6 +1817,15 @@ static int r871x_wx_set_enc_ext(struct net_device *dev,
> > >  	default:
> > >  		return -EINVAL;
> > >  	}
> > > +
> > > +	param_len = sizeof(struct ieee_param) + pext->key_len;
> > > +	param = (struct ieee_param *)_malloc(param_len);
> > 
> > While you are here could you substitute _malloc by kzalloc and remove
> > explicit casting and memset?
> > 
> 
> Normally, that's the kind of thing we would do in a separate patch
> because or the "one thing per patch" rule.  Eventually someone will do a
> driver wide replacement of _malloc().  Or if the bug fixer wanted, he
> could do it in this patch because it is on the same line and all so it
> counts as a "minor closely related change."  Either way is fine.
> 
> Another way to say this is that since the _malloc() was there in the
> original code and Christian didn't introduce it, then we shouldn't
> reject his patch because of it.  This is staging code, and there are so
> many problems that if you start trying to fix everything you'll just get
> lost.
> 
> In my experience v2 patches are much harder to write than v1 patches.
> Twice in the past few days I have messed up the subject line in my v2
> patches.

I'm not objecting to let green light for it, indeed maybe someone could
change all _mallocs & memsets in that code.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy