lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 25 Nov 2017 07:55:15 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     ishraq.i.ashraf@...il.com
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        insafonov@...il.com, goudapatilk@...il.com,
        linux-kernel@...r.kernel.org, himanshujha199640@...il.com
Subject: Re: [PATCH v2] staging: rtl8188eu: Fix private WEXT IOCTL calls

On Sat, Nov 25, 2017 at 02:29:36AM +0100, ishraq.i.ashraf@...il.com wrote:
> +
> +	ret = 0;
> +

Sorry, I wasn't clear before.  When I said don't initialize "ret" to
zero, I just meant that in that specific case we initialized "ret" and
then immediately reassigned it with:

	ret = some_function();
	if (ret)
		return ret;

In this case it's fine to set "ret = 0" at the start so that we don't
have to do it later.

> +	if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
> +		ret = -EFAULT;
> +
> +	if (pwep)
> +		goto err_free_pwep_param;
> +
> +	err_free_param:
> +		kfree(param);
> +		return ret;
> +
> +	err_free_pwep_param:
> +		kfree(pwep);
> +		kfree(param);
> +		return ret;
> +}

Hm...  I said before that it's better to keep the error paths and
success path separate but in this case it's probabaly simpler to merge
them.

This one could look like this:

	if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
		ret = -EFAULT;

free_pwep:
	kfree(pwep);
free_param:
	kfree(param);
	return ret;

There is no need for the if (pwep) conditions, because kfree() can
take a NULL pointer.  Some people would just use one label but I hate
that.  It looks like this:

free:
	kfree(pwep);
	kfree(param);
	return ret;

The reason, I hate it is because I don't like freeing things which have
not been allocated yet.  If you do it the normal kernel way then you
just have to keep track of the most recently allocated thing.

regards,
dan carpenter

Powered by blists - more mailing lists