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@xxxxxxxxxxxxxxxxxx>
> ---
>   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:

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/