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  linux-hardening  linux-cve-announce  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]
Message-ID: <537BE4BB.4010308@lwfinger.net>
Date:	Tue, 20 May 2014 18:26:51 -0500
From:	Larry Finger <Larry.Finger@...inger.net>
To:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	Manu Gupta <manugupt1@...il.com>,
	Josh Triplett <josh@...htriplett.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c:  Fix for possible
 null pointer dereference

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@...ctrumdigital.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:
>

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ