wifi: iwlwifi: mvm: Fix __counted_by usage in cfg80211_wowlan_nd_*

Both struct cfg80211_wowlan_nd_match and struct cfg80211_wowlan_nd_info pre-allocate space for channels and matches, but then may end up using fewer that the full allocation. Shrink the associated counter (n_channels and n_matches) after counting the results. This avoids compile-time (and run-time) warnings from __counted_by. (The counter member needs to be updated _before_ accessing the array index.)

Seen with coming GCC 15:

drivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function 'iwl_mvm_query_set_freqs':
drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2877:66: warning: operation on 'match->n_channels' may be undefined [-Wsequence-point]
2877 | match->channels[match->n_channels++] =

       |                                                 ~~~~~~~~~~~~~~~~~^~
drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2885:66: warning: operation on 'match->n_channels' may be undefined [-Wsequence-point]
  2885 |                                 match->channels[match->n_channels++] =
       |                                                 ~~~~~~~~~~~~~~~~~^~
drivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function 'iwl_mvm_query_netdetect_reasons':
drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2982:58: warning: operation on 'net_detect->n_matches' may be undefined [-Wsequence-point]
  2982 |                 net_detect->matches[net_detect->n_matches++] = match;
       |                                     ~~~~~~~~~~~~~~~~~~~~~^~

Fixes aa4ec06c455d ("wifi: cfg80211: use __counted_by where appropriate")

Signed-off-by Kees Cook <[email protected]>

---
Cc: Miri Korenblit <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Gustavo A. R. Silva <[email protected]>
Cc: Luca Coelho <[email protected]>
Cc: Gregory Greenman <[email protected]>
Cc: Yedidya Benshimol <[email protected]>
Cc: Haim Dreyfuss <[email protected]>
Cc: [email protected]

---
  drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 14 +++++++++++---
  1 file changed, 11 insertions(+), 3 deletions(-)

drivers/net/wireless/intel/iwlwifi/mvm/d3.c +11 -3

index 54f4acbbd05b..9cd03ea4680d 100644
@@ -2866,6 +2866,7 @@ static void iwl_mvm_query_set_freqs(struct iwl_mvm *mvm,
  				    int idx)
  {
  	int i;
+	int n_channels = 0;
 
  	if (fw_has_api(&mvm->fw->ucode_capa,
  		       IWL_UCODE_TLV_API_SCAN_OFFLOAD_CHANS)) {

@@ -2874,7 +2875,7 @@ static void iwl_mvm_query_set_freqs(struct iwl_mvm *mvm,
 
  		for (i = 0; i < SCAN_OFFLOAD_MATCHING_CHANNELS_LEN * 8; i++)
  			if (matches[idx].matching_channels[i / 8] & (BIT(i % 8)))
-				match->channels[match->n_channels++] =
+				match->channels[n_channels++] =
  					mvm->nd_channels[i]->center_freq;
  	} else {
  		struct iwl_scan_offload_profile_match_v1 *matches =

@@ -2882,9 +2883,11 @@ static void iwl_mvm_query_set_freqs(struct iwl_mvm *mvm,
 
  		for (i = 0; i < SCAN_OFFLOAD_MATCHING_CHANNELS_LEN_V1 * 8; i++)
  			if (matches[idx].matching_channels[i / 8] & (BIT(i % 8)))
-				match->channels[match->n_channels++] =
+				match->channels[n_channels++] =
  					mvm->nd_channels[i]->center_freq;
  	}
+	/* We may have ended up with fewer channels than we allocated. */
+	match->n_channels = n_channels;
  }
 
  /**

@@ -2965,6 +2968,8 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm,
  			     GFP_KERNEL);
  	if (!net_detect || !n_matches)
  		goto out_report_nd;
+	net_detect->n_matches = n_matches;
+	n_matches = 0;
 
  	for_each_set_bit(i, &matched_profiles, mvm->n_nd_match_sets) {
  		struct cfg80211_wowlan_nd_match *match;

@@ -2978,8 +2983,9 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm,
  				GFP_KERNEL);
  		if (!match)
  			goto out_report_nd;
+		match->n_channels = n_channels;
 
-		net_detect->matches[net_detect->n_matches++] = match;
+		net_detect->matches[n_matches++] = match;
 
  		/* We inverted the order of the SSIDs in the scan
  		 * request, so invert the index here.

@@ -2994,6 +3000,8 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm,
 
  		iwl_mvm_query_set_freqs(mvm, d3_data->nd_results, match, i);
  	}
+	/* We may have fewer matches than we allocated. */
+	net_detect->n_matches = n_matches;
 
  out_report_nd:
  	wakeup.net_detect = net_detect;

Both struct cfg80211_wowlan_nd_match and struct cfg80211_wowlan_nd_info pre-allocate space for channels and matches, but then may end up using fewer that the full allocation. Shrink the associated counter (n_channels and n_matches) after counting the results. This avoids compile-time (and run-time) warnings from __counted_by. (The counter member needs to be updated _before_ accessing the array index.)

Seen with coming GCC 15:

drivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function 'iwl_mvm_query_set_freqs':
drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2877:66: warning: operation on 'match->n_channels' may be undefined [-Wsequence-point]
2877 | match->channels[match->n_channels++] =

       |                                                 ~~~~~~~~~~~~~~~~~^~
drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2885:66: warning: operation on 'match->n_channels' may be undefined [-Wsequence-point]
  2885 |                                 match->channels[match->n_channels++] =
       |                                                 ~~~~~~~~~~~~~~~~~^~
drivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function 'iwl_mvm_query_netdetect_reasons':
drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2982:58: warning: operation on 'net_detect->n_matches' may be undefined [-Wsequence-point]
  2982 |                 net_detect->matches[net_detect->n_matches++] = match;
       |                                     ~~~~~~~~~~~~~~~~~~~~~^~

Fixes aa4ec06c455d ("wifi: cfg80211: use __counted_by where appropriate")

Signed-off-by Kees Cook <[email protected]>

---
Cc: Miri Korenblit <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Gustavo A. R. Silva <[email protected]>
Cc: Luca Coelho <[email protected]>
Cc: Gregory Greenman <[email protected]>
Cc: Yedidya Benshimol <[email protected]>
Cc: Haim Dreyfuss <[email protected]>
Cc: [email protected]

---
  drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 14 +++++++++++---
  1 file changed, 11 insertions(+), 3 deletions(-)

drivers/net/wireless/intel/iwlwifi/mvm/d3.c +2 -1

index 54f4acbbd05b..9cd03ea4680d 100644
@@ -2866,6 +2866,7 @@ static void iwl_mvm_query_set_freqs(struct iwl_mvm *mvm,
  				    int idx)
  {
  	int i;
+	int n_channels = 0;
 
  	if (fw_has_api(&mvm->fw->ucode_capa,
  		       IWL_UCODE_TLV_API_SCAN_OFFLOAD_CHANS)) {

@@ -2874,7 +2875,7 @@ static void iwl_mvm_query_set_freqs(struct iwl_mvm *mvm,
 
  		for (i = 0; i < SCAN_OFFLOAD_MATCHING_CHANNELS_LEN * 8; i++)
  			if (matches[idx].matching_channels[i / 8] & (BIT(i % 8)))
-				match->channels[match->n_channels++] =
+				match->channels[n_channels++] =
  					mvm->nd_channels[i]->center_freq;
  	} else {
  		struct iwl_scan_offload_profile_match_v1 *matches =

drivers/net/wireless/intel/iwlwifi/mvm/d3.c +0 -0

  }

/**

drivers/net/wireless/intel/iwlwifi/mvm/d3.c +6 -1

@@ -2965,6 +2968,8 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm,
  			     GFP_KERNEL);
  	if (!net_detect || !n_matches)
  		goto out_report_nd;
+	net_detect->n_matches = n_matches;
+	n_matches = 0;
 
  	for_each_set_bit(i, &matched_profiles, mvm->n_nd_match_sets) {
  		struct cfg80211_wowlan_nd_match *match;

@@ -2978,8 +2983,9 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm,
  				GFP_KERNEL);
  		if (!match)
  			goto out_report_nd;
+		match->n_channels = n_channels;
 
-		net_detect->matches[net_detect->n_matches++] = match;
+		net_detect->matches[n_matches++] = match;
 
  		/* We inverted the order of the SSIDs in the scan
  		 * request, so invert the index here.

@@ -2994,6 +3000,8 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm,
 
  		iwl_mvm_query_set_freqs(mvm, d3_data->nd_results, match, i);
  	}
+	/* We may have fewer matches than we allocated. */
+	net_detect->n_matches = n_matches;
 
  out_report_nd:
  	wakeup.net_detect = net_detect;

My understanding is that 'match', is allocated by :
    match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL);

So match->n_channels is *0* when iwl_mvm_query_set_freqs() is called.

Quoted 5 lines

My understanding is that 'match', is allocated by :
     match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL);

So match->n_channels is *0* when iwl_mvm_query_set_freqs() is called.

n_channels is updated in the line before calling kzalloc():

n_channels = iwl_mvm_query_num_match_chans(mvm, d3_data->nd_results, i);

match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL);

Quoted 10 lines

Quoted 5 lines

My understanding is that 'match', is allocated by :
     match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL);

So match->n_channels is *0* when iwl_mvm_query_set_freqs() is called.

n_channels is updated in the line before calling kzalloc():

n_channels = iwl_mvm_query_num_match_chans(mvm, d3_data->nd_results, i);

Quoted 1 line

match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL);

then match->n_channels updated here:

         if (!match)
             goto out_report_nd;
+        match->n_channels = n_channels;

Sorry about the split response, finger failure ugghh

Signature · 1 line

Gustavo

Quoted 6 lines

then match->n_channels updated here:

          if (!match)
              goto out_report_nd;
+        match->n_channels = n_channels;

Thanks for the explanation.
This is what I was looking for, and I missed this line.

Sorry for the noise.

Le 20/06/2024 à 20:08, Gustavo A. R. Silva a écrit :

Quoted 18 lines · Gustavo A. R. Silva

Quoted 5 lines

Quoted 5 lines

My understanding is that 'match', is allocated by :
     match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL);

Quoted 7 lines

Quoted 1 line

So match->n_channels is *0* when iwl_mvm_query_set_freqs() is called.

n_channels is updated in the line before calling kzalloc():

n_channels = iwl_mvm_query_num_match_chans(mvm, d3_data->nd_results, i);

match = kzalloc(struct_size(match, channels, n_channels), GFP_KERNEL);

then match->n_channels updated here:

Quoted 3 lines

         if (!match)
             goto out_report_nd;
+        match->n_channels = n_channels;

Thanks for the explanation.
This is what I was looking for, and I missed this line.

Sorry for the noise.