potential null deref in iwlagn_request_scan()?

potential null deref in iwlagn_request_scan()?

[Dan Carpenter]

Smatch complains about iwlagn_request_scan().

drivers/net/wireless/iwlwifi/iwl-agn-lib.c +1354 iwlagn_request_scan(204)
	error: we previously assumed 'vif' could be null.

  1351          if (!priv->is_internal_short_scan) {
  1352                  cmd_len = iwl_fill_probe_req(priv,
  1353                                          (struct ieee80211_mgmt *)scan->data,
  1354                                          vif->addr,
						^^^^^^^^^

  1355                                          priv->scan_request->ie, 
  1356						priv->scan_request->ie_len,
  1357                                          IWL_MAX_SCAN_SIZE - sizeof(*scan));
  1358          } else {
  1359                  /* use bcast addr, will not be transmitted but must be valid */

This was added in 3a0b9aad0a8166e9f "iwlwifi: use virtual interface
address for scan".  Prior to that commit the function assumed that vif
could be NULL throughout.

I don't know the code well enough to know what to do about this.

Also the same thing for:
drivers/net/wireless/iwlwifi/iwl3945-base.c +2963 iwl3945_request_scan(158)
	error: we previously assumed 'vif' could be null.

  2962          if (!priv->is_internal_short_scan) {
  2963                  scan->tx_cmd.len = cpu_to_le16(
  2964                          iwl_fill_probe_req(priv,
  2965                                  (struct ieee80211_mgmt *)scan->data,
  2966                                  vif->addr,
					^^^^^^^^^
  2967                                  priv->scan_request->ie,
  2968                                  priv->scan_request->ie_len,
  2969                                  IWL_MAX_SCAN_SIZE - sizeof(*scan)));
  2970          } else {

This is from the above commit as well.

regards,
dan carpenter

Re: potential null deref in iwlagn_request_scan()?

[John W. Linville]

On Thu, Jul 22, 2010 at 12:16:16AM +0200, Dan Carpenter wrote:
> Smatch complains about iwlagn_request_scan().
> 
> drivers/net/wireless/iwlwifi/iwl-agn-lib.c +1354 iwlagn_request_scan(204)
> 	error: we previously assumed 'vif' could be null.
> 
>   1351          if (!priv->is_internal_short_scan) {
>   1352                  cmd_len = iwl_fill_probe_req(priv,
>   1353                                          (struct ieee80211_mgmt *)scan->data,
>   1354                                          vif->addr,
> 						^^^^^^^^^
> 
>   1355                                          priv->scan_request->ie, 
>   1356						priv->scan_request->ie_len,
>   1357                                          IWL_MAX_SCAN_SIZE - sizeof(*scan));
>   1358          } else {
>   1359                  /* use bcast addr, will not be transmitted but must be valid */
> 
> This was added in 3a0b9aad0a8166e9f "iwlwifi: use virtual interface
> address for scan".  Prior to that commit the function assumed that vif
> could be NULL throughout.
> 
> I don't know the code well enough to know what to do about this.
> 
> Also the same thing for:
> drivers/net/wireless/iwlwifi/iwl3945-base.c +2963 iwl3945_request_scan(158)
> 	error: we previously assumed 'vif' could be null.

I think both of these are more-or-less OK.  It looks like the only
time vif would be NULL is in the "priv->is_internal_short_scan ==
true" case.  I'll send a patch, but I don't know if it really matters.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH] iwlwifi: assume vif is NULL for internal scans and non-NULL otherwise

[John W. Linville]

The current practice of checking vif for NULL in one place but not
another seems to confuse some static checkers, smatch in particular.
Since vif will only be NULL in the case of internal scans, adjust the
checks accordingly.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/iwlwifi/iwl-agn-lib.c  |    5 ++++-
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
index 74623e0..0ca0df4 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
@@ -1234,7 +1234,10 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
 
 		IWL_DEBUG_INFO(priv, "Scanning while associated...\n");
 		spin_lock_irqsave(&priv->lock, flags);
-		interval = vif ? vif->bss_conf.beacon_int : 0;
+		if (priv->is_internal_short_scan)
+			interval = 0;
+		else
+			interval = vif->bss_conf.beacon_int;
 		spin_unlock_irqrestore(&priv->lock, flags);
 
 		scan->suspend_time = 0;
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 8eb3471..b102bab 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -2883,7 +2883,10 @@ void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
 		IWL_DEBUG_INFO(priv, "Scanning while associated...\n");
 
 		spin_lock_irqsave(&priv->lock, flags);
-		interval = vif ? vif->bss_conf.beacon_int : 0;
+		if (priv->is_internal_short_scan)
+			interval = 0;
+		else
+			interval = vif->bss_conf.beacon_int;
 		spin_unlock_irqrestore(&priv->lock, flags);
 
 		scan->suspend_time = 0;
-- 
1.7.1.1

Re: [PATCH] iwlwifi: assume vif is NULL for internal scans and non-NULL otherwise

[Guy, Wey-Yi]

Hi John,

On Thu, 2010-07-22 at 12:28 -0700, John W. Linville wrote:
> The current practice of checking vif for NULL in one place but not
> another seems to confuse some static checkers, smatch in particular.
> Since vif will only be NULL in the case of internal scans, adjust the
> checks accordingly.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
>  drivers/net/wireless/iwlwifi/iwl-agn-lib.c  |    5 ++++-
>  drivers/net/wireless/iwlwifi/iwl3945-base.c |    5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> index 74623e0..0ca0df4 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
> @@ -1234,7 +1234,10 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
>  
>  		IWL_DEBUG_INFO(priv, "Scanning while associated...\n");
>  		spin_lock_irqsave(&priv->lock, flags);
> -		interval = vif ? vif->bss_conf.beacon_int : 0;
> +		if (priv->is_internal_short_scan)
> +			interval = 0;
> +		else
> +			interval = vif->bss_conf.beacon_int;
>  		spin_unlock_irqrestore(&priv->lock, flags);
>  
>  		scan->suspend_time = 0;
> diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> index 8eb3471..b102bab 100644
> --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
> +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> @@ -2883,7 +2883,10 @@ void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
>  		IWL_DEBUG_INFO(priv, "Scanning while associated...\n");
>  
>  		spin_lock_irqsave(&priv->lock, flags);
> -		interval = vif ? vif->bss_conf.beacon_int : 0;
> +		if (priv->is_internal_short_scan)
> +			interval = 0;
> +		else
> +			interval = vif->bss_conf.beacon_int;
>  		spin_unlock_irqrestore(&priv->lock, flags);
>  
>  		scan->suspend_time = 0;

Make sense, Thanks.

Wey