[PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

There is otherwise a risk of a possible null pointer dereference.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/staging/rtl8188eu/os_dep/usb_intf.c |  127 ++++++++++++++-------------
 1 file changed, 66 insertions(+), 61 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 2e49cd5..425b955 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -384,9 +384,16 @@ static void process_spec_devid(const struct usb_device_id *pdid)
 
 int rtw_hw_suspend(struct adapter *padapter)
 {
-	struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
-	struct net_device *pnetdev = padapter->pnetdev;
+	struct pwrctrl_priv *pwrpriv;
+	struct net_device *pnetdev;
 
+	if (!padapter)  {
+		goto error_exit;
+	}
+
+	/* system suspend */
+	pwrpriv = &padapter->pwrctrlpriv;
+	pnetdev = padapter->pnetdev;
 
 	if ((!padapter->bup) || (padapter->bDriverStopped) ||
 	    (padapter->bSurpriseRemoved)) {
@@ -396,49 +403,46 @@ int rtw_hw_suspend(struct adapter *padapter)
 		goto error_exit;
 	}
 
-	if (padapter) { /* system suspend */
-		LeaveAllPowerSaveMode(padapter);
+	LeaveAllPowerSaveMode(padapter);
 
-		DBG_88E("==> rtw_hw_suspend\n");
-		_enter_pwrlock(&pwrpriv->lock);
-		pwrpriv->bips_processing = true;
-		/* s1. */
-		if (pnetdev) {
-			netif_carrier_off(pnetdev);
-			rtw_netif_stop_queue(pnetdev);
-		}
+	DBG_88E("==> rtw_hw_suspend\n");
+	_enter_pwrlock(&pwrpriv->lock);
+	pwrpriv->bips_processing = true;
+	/* s1. */
+	if (pnetdev) {
+		netif_carrier_off(pnetdev);
+		rtw_netif_stop_queue(pnetdev);
+	}
 
-		/* s2. */
-		rtw_disassoc_cmd(padapter, 500, false);
+	/* s2. */
+	rtw_disassoc_cmd(padapter, 500, false);
 
-		/* s2-2.  indicate disconnect to os */
-		{
-			struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;
+	/* s2-2.  indicate disconnect to os */
+	{
+		struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
-			if (check_fwstate(pmlmepriv, _FW_LINKED)) {
-				_clr_fwstate_(pmlmepriv, _FW_LINKED);
+		if (check_fwstate(pmlmepriv, _FW_LINKED)) {
+			_clr_fwstate_(pmlmepriv, _FW_LINKED);
 
-				rtw_led_control(padapter, LED_CTL_NO_LINK);
+			rtw_led_control(padapter, LED_CTL_NO_LINK);
 
-				rtw_os_indicate_disconnect(padapter);
+			rtw_os_indicate_disconnect(padapter);
 
-				/* donnot enqueue cmd */
-				rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_DISCONNECT, 0);
-			}
+			/* donnot enqueue cmd */
+			rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_DISCONNECT, 0);
 		}
-		/* s2-3. */
-		rtw_free_assoc_resources(padapter, 1);
+	}
+	/* s2-3. */
+	rtw_free_assoc_resources(padapter, 1);
 
-		/* s2-4. */
-		rtw_free_network_queue(padapter, true);
-		rtw_ips_dev_unload(padapter);
-		pwrpriv->rf_pwrstate = rf_off;
-		pwrpriv->bips_processing = false;
+	/* s2-4. */
+	rtw_free_network_queue(padapter, true);
+	rtw_ips_dev_unload(padapter);
+	pwrpriv->rf_pwrstate = rf_off;
+	pwrpriv->bips_processing = false;
+
+	_exit_pwrlock(&pwrpriv->lock);
 
-		_exit_pwrlock(&pwrpriv->lock);
-	} else {
-		goto error_exit;
-	}
 	return 0;
 
 error_exit:
@@ -448,40 +452,41 @@ error_exit:
 
 int rtw_hw_resume(struct adapter *padapter)
 {
-	struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
-	struct net_device *pnetdev = padapter->pnetdev;
-
+	struct pwrctrl_priv *pwrpriv;
+	struct net_device *pnetdev;
 
-	if (padapter) { /* system resume */
-		DBG_88E("==> rtw_hw_resume\n");
-		_enter_pwrlock(&pwrpriv->lock);
-		pwrpriv->bips_processing = true;
-		rtw_reset_drv_sw(padapter);
+	if (!padapter) {
+		goto error_exit;
+	}
+	/* system resume */
+	pwrpriv = &padapter->pwrctrlpriv;
+	pnetdev = padapter->pnetdev;
 
-		if (pm_netdev_open(pnetdev, false) != 0) {
-			_exit_pwrlock(&pwrpriv->lock);
-			goto error_exit;
-		}
+	DBG_88E("==> rtw_hw_resume\n");
+	_enter_pwrlock(&pwrpriv->lock);
+	pwrpriv->bips_processing = true;
+	rtw_reset_drv_sw(padapter);
 
-		netif_device_attach(pnetdev);
-		netif_carrier_on(pnetdev);
+	if (pm_netdev_open(pnetdev, false) != 0) {
+		_exit_pwrlock(&pwrpriv->lock);
+		goto error_exit;
+	}
 
-		if (!netif_queue_stopped(pnetdev))
-			netif_start_queue(pnetdev);
-		else
-			netif_wake_queue(pnetdev);
+	netif_device_attach(pnetdev);
+	netif_carrier_on(pnetdev);
 
-		pwrpriv->bkeepfwalive = false;
-		pwrpriv->brfoffbyhw = false;
+	if (!netif_queue_stopped(pnetdev))
+		netif_start_queue(pnetdev);
+	else
+		netif_wake_queue(pnetdev);
 
-		pwrpriv->rf_pwrstate = rf_on;
-		pwrpriv->bips_processing = false;
+	pwrpriv->bkeepfwalive = false;
+	pwrpriv->brfoffbyhw = false;
 
-		_exit_pwrlock(&pwrpriv->lock);
-	} else {
-		goto error_exit;
-	}
+	pwrpriv->rf_pwrstate = rf_on;
+	pwrpriv->bips_processing = false;
 
+	_exit_pwrlock(&pwrpriv->lock);
 
 	return 0;
 error_exit:
-- 
1.7.10.4

Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Dan Carpenter]

On Tue, May 20, 2014 at 11:31:16PM +0200, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.
> 

"padapter" can't be NULL.  Just remove the check.

regards,
dan carpenter

Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Larry Finger]

On 05/20/2014 04:31 PM, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
>
> Was largely found by using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>   drivers/staging/rtl8188eu/os_dep/usb_intf.c |  127 ++++++++++++++-------------
>   1 file changed, 66 insertions(+), 61 deletions(-)

Although cppcheck's analysis is correct, pointer padapter can never be null in 
any of these routines. In routine rtw_drv_init(), rtw_usb_if1_init() is called 
to allocate memory for struct adapter for the driver. If that fails, none of 
these other routines will ever be called as the driver will not be loaded.

If it is deemed better to fill the code with needless tests because some static 
checker points out a place like this, that is OK with me, but I do not see the 
point.

Larry

>
> diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> index 2e49cd5..425b955 100644
> --- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> @@ -384,9 +384,16 @@ static void process_spec_devid(const struct usb_device_id *pdid)
>
>   int rtw_hw_suspend(struct adapter *padapter)
>   {
> -	struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
> -	struct net_device *pnetdev = padapter->pnetdev;
> +	struct pwrctrl_priv *pwrpriv;
> +	struct net_device *pnetdev;
>
> +	if (!padapter)  {
> +		goto error_exit;
> +	}
> +
> +	/* system suspend */
> +	pwrpriv = &padapter->pwrctrlpriv;
> +	pnetdev = padapter->pnetdev;
>
>   	if ((!padapter->bup) || (padapter->bDriverStopped) ||
>   	    (padapter->bSurpriseRemoved)) {
> @@ -396,49 +403,46 @@ int rtw_hw_suspend(struct adapter *padapter)
>   		goto error_exit;
>   	}
>
> -	if (padapter) { /* system suspend */
> -		LeaveAllPowerSaveMode(padapter);
> +	LeaveAllPowerSaveMode(padapter);
>
> -		DBG_88E("==> rtw_hw_suspend\n");
> -		_enter_pwrlock(&pwrpriv->lock);
> -		pwrpriv->bips_processing = true;
> -		/* s1. */
> -		if (pnetdev) {
> -			netif_carrier_off(pnetdev);
> -			rtw_netif_stop_queue(pnetdev);
> -		}
> +	DBG_88E("==> rtw_hw_suspend\n");
> +	_enter_pwrlock(&pwrpriv->lock);
> +	pwrpriv->bips_processing = true;
> +	/* s1. */
> +	if (pnetdev) {
> +		netif_carrier_off(pnetdev);
> +		rtw_netif_stop_queue(pnetdev);
> +	}
>
> -		/* s2. */
> -		rtw_disassoc_cmd(padapter, 500, false);
> +	/* s2. */
> +	rtw_disassoc_cmd(padapter, 500, false);
>
> -		/* s2-2.  indicate disconnect to os */
> -		{
> -			struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;
> +	/* s2-2.  indicate disconnect to os */
> +	{
> +		struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;
>
> -			if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> -				_clr_fwstate_(pmlmepriv, _FW_LINKED);
> +		if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> +			_clr_fwstate_(pmlmepriv, _FW_LINKED);
>
> -				rtw_led_control(padapter, LED_CTL_NO_LINK);
> +			rtw_led_control(padapter, LED_CTL_NO_LINK);
>
> -				rtw_os_indicate_disconnect(padapter);
> +			rtw_os_indicate_disconnect(padapter);
>
> -				/* donnot enqueue cmd */
> -				rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_DISCONNECT, 0);
> -			}
> +			/* donnot enqueue cmd */
> +			rtw_lps_ctrl_wk_cmd(padapter, LPS_CTRL_DISCONNECT, 0);
>   		}
> -		/* s2-3. */
> -		rtw_free_assoc_resources(padapter, 1);
> +	}
> +	/* s2-3. */
> +	rtw_free_assoc_resources(padapter, 1);
>
> -		/* s2-4. */
> -		rtw_free_network_queue(padapter, true);
> -		rtw_ips_dev_unload(padapter);
> -		pwrpriv->rf_pwrstate = rf_off;
> -		pwrpriv->bips_processing = false;
> +	/* s2-4. */
> +	rtw_free_network_queue(padapter, true);
> +	rtw_ips_dev_unload(padapter);
> +	pwrpriv->rf_pwrstate = rf_off;
> +	pwrpriv->bips_processing = false;
> +
> +	_exit_pwrlock(&pwrpriv->lock);
>
> -		_exit_pwrlock(&pwrpriv->lock);
> -	} else {
> -		goto error_exit;
> -	}
>   	return 0;
>
>   error_exit:
> @@ -448,40 +452,41 @@ error_exit:
>
>   int rtw_hw_resume(struct adapter *padapter)
>   {
> -	struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
> -	struct net_device *pnetdev = padapter->pnetdev;
> -
> +	struct pwrctrl_priv *pwrpriv;
> +	struct net_device *pnetdev;
>
> -	if (padapter) { /* system resume */
> -		DBG_88E("==> rtw_hw_resume\n");
> -		_enter_pwrlock(&pwrpriv->lock);
> -		pwrpriv->bips_processing = true;
> -		rtw_reset_drv_sw(padapter);
> +	if (!padapter) {
> +		goto error_exit;
> +	}
> +	/* system resume */
> +	pwrpriv = &padapter->pwrctrlpriv;
> +	pnetdev = padapter->pnetdev;
>
> -		if (pm_netdev_open(pnetdev, false) != 0) {
> -			_exit_pwrlock(&pwrpriv->lock);
> -			goto error_exit;
> -		}
> +	DBG_88E("==> rtw_hw_resume\n");
> +	_enter_pwrlock(&pwrpriv->lock);
> +	pwrpriv->bips_processing = true;
> +	rtw_reset_drv_sw(padapter);
>
> -		netif_device_attach(pnetdev);
> -		netif_carrier_on(pnetdev);
> +	if (pm_netdev_open(pnetdev, false) != 0) {
> +		_exit_pwrlock(&pwrpriv->lock);
> +		goto error_exit;
> +	}
>
> -		if (!netif_queue_stopped(pnetdev))
> -			netif_start_queue(pnetdev);
> -		else
> -			netif_wake_queue(pnetdev);
> +	netif_device_attach(pnetdev);
> +	netif_carrier_on(pnetdev);
>
> -		pwrpriv->bkeepfwalive = false;
> -		pwrpriv->brfoffbyhw = false;
> +	if (!netif_queue_stopped(pnetdev))
> +		netif_start_queue(pnetdev);
> +	else
> +		netif_wake_queue(pnetdev);
>
> -		pwrpriv->rf_pwrstate = rf_on;
> -		pwrpriv->bips_processing = false;
> +	pwrpriv->bkeepfwalive = false;
> +	pwrpriv->brfoffbyhw = false;
>
> -		_exit_pwrlock(&pwrpriv->lock);
> -	} else {
> -		goto error_exit;
> -	}
> +	pwrpriv->rf_pwrstate = rf_on;
> +	pwrpriv->bips_processing = false;
>
> +	_exit_pwrlock(&pwrpriv->lock);
>
>   	return 0;
>   error_exit:
>

Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[josh]

On Tue, May 20, 2014 at 06:26:51PM -0500, Larry Finger wrote:
> On 05/20/2014 04:31 PM, Rickard Strandqvist wrote:
> >There is otherwise a risk of a possible null pointer dereference.
> >
> >Was largely found by using a static code analysis program called cppcheck.
> >
> >Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> >---
> >  drivers/staging/rtl8188eu/os_dep/usb_intf.c |  127 ++++++++++++++-------------
> >  1 file changed, 66 insertions(+), 61 deletions(-)
> 
> Although cppcheck's analysis is correct, pointer padapter can never
> be null in any of these routines. In routine rtw_drv_init(),
> rtw_usb_if1_init() is called to allocate memory for struct adapter
> for the driver. If that fails, none of these other routines will
> ever be called as the driver will not be loaded.
> 
> If it is deemed better to fill the code with needless tests because
> some static checker points out a place like this, that is OK with
> me, but I do not see the point.

If it's an invariant of the code that padapter cannot ever be NULL, then
that seems perfectly fine to rely on, and the tool just needs fixing or
silencing.  At most, it might be helpful to add annotations like GCC's
"nonnull", if that helps the checker and the compiler generate more
useful warnings.

- Josh Triplett

Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Dan Carpenter]

On Tue, May 20, 2014 at 04:57:56PM -0700, josh@joshtriplett.org wrote:
> On Tue, May 20, 2014 at 06:26:51PM -0500, Larry Finger wrote:
> > On 05/20/2014 04:31 PM, Rickard Strandqvist wrote:
> > >There is otherwise a risk of a possible null pointer dereference.
> > >
> > >Was largely found by using a static code analysis program called cppcheck.
> > >
> > >Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> > >---
> > >  drivers/staging/rtl8188eu/os_dep/usb_intf.c |  127 ++++++++++++++-------------
> > >  1 file changed, 66 insertions(+), 61 deletions(-)
> > 
> > Although cppcheck's analysis is correct, pointer padapter can never
> > be null in any of these routines. In routine rtw_drv_init(),
> > rtw_usb_if1_init() is called to allocate memory for struct adapter
> > for the driver. If that fails, none of these other routines will
> > ever be called as the driver will not be loaded.
> > 
> > If it is deemed better to fill the code with needless tests because
> > some static checker points out a place like this, that is OK with
> > me, but I do not see the point.
> 
> If it's an invariant of the code that padapter cannot ever be NULL, then
> that seems perfectly fine to rely on, and the tool just needs fixing or
> silencing.  At most, it might be helpful to add annotations like GCC's
> "nonnull", if that helps the checker and the compiler generate more
> useful warnings.

The tool is complaining that the existing checks for NULL don't make
sense.  Rickard changed them to cover all the dereferences but really we
should just remove the checks.

Btw, speaking of GCC, it now silently removes inconsistent NULL
checking.  It takes the opposite of the normall kernel programmer
approach of adding checks everywhere.  :P  The glibc people recently
annotated qsort() to say that the function pointer can't be NULL even
though this "worked" in the past.  Hillarity!

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61236

regards,
dan carpenter

Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

Hi

Exactly as you say Dan, cppcheck has found that there is a null check
at some point while the others in this case are missed. So I'm not for
unnecessary checks.
But we can instead agree that I make a patch that will remove all null
checks padapter?

Dan: I'll check on the scripts/checkpatch.pl

Best regards
Rickard Strandqvist


2014-05-21 9:24 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Tue, May 20, 2014 at 04:57:56PM -0700, josh@joshtriplett.org wrote:
>> On Tue, May 20, 2014 at 06:26:51PM -0500, Larry Finger wrote:
>> > On 05/20/2014 04:31 PM, Rickard Strandqvist wrote:
>> > >There is otherwise a risk of a possible null pointer dereference.
>> > >
>> > >Was largely found by using a static code analysis program called cppcheck.
>> > >
>> > >Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> > >---
>> > >  drivers/staging/rtl8188eu/os_dep/usb_intf.c |  127 ++++++++++++++-------------
>> > >  1 file changed, 66 insertions(+), 61 deletions(-)
>> >
>> > Although cppcheck's analysis is correct, pointer padapter can never
>> > be null in any of these routines. In routine rtw_drv_init(),
>> > rtw_usb_if1_init() is called to allocate memory for struct adapter
>> > for the driver. If that fails, none of these other routines will
>> > ever be called as the driver will not be loaded.
>> >
>> > If it is deemed better to fill the code with needless tests because
>> > some static checker points out a place like this, that is OK with
>> > me, but I do not see the point.
>>
>> If it's an invariant of the code that padapter cannot ever be NULL, then
>> that seems perfectly fine to rely on, and the tool just needs fixing or
>> silencing.  At most, it might be helpful to add annotations like GCC's
>> "nonnull", if that helps the checker and the compiler generate more
>> useful warnings.
>
> The tool is complaining that the existing checks for NULL don't make
> sense.  Rickard changed them to cover all the dereferences but really we
> should just remove the checks.
>
> Btw, speaking of GCC, it now silently removes inconsistent NULL
> checking.  It takes the opposite of the normall kernel programmer
> approach of adding checks everywhere.  :P  The glibc people recently
> annotated qsort() to say that the function pointer can't be NULL even
> though this "worked" in the past.  Hillarity!
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61236
>
> regards,
> dan carpenter
>

Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Dan Carpenter]

On Wed, May 21, 2014 at 11:51:06PM +0200, Rickard Strandqvist wrote:
> Hi
> 
> Exactly as you say Dan, cppcheck has found that there is a null check
> at some point while the others in this case are missed. So I'm not for
> unnecessary checks.
> But we can instead agree that I make a patch that will remove all null
> checks padapter?

Yes.  On this function remove all the checks for padapter.

regards,
dan carpenter

Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

[Dan Carpenter]

On Tue, May 20, 2014 at 11:31:16PM +0200, Rickard Strandqvist wrote:
> +	if (!padapter)  {
> +		goto error_exit;
> +	}

Btw, I forgot to mention this before but please run your patches through
scripts/checkpatch.pl.

regards,
dan carpenter