Re: [PATCH v2] staging: rtl8723bs: rtw_mlme: fix line length warnings

[Salman Alghamdi <me@cipherat.com>]

> glad to see v2, just one thing before inline reviews of the code. You 
> don't have to state what did you do to make lines shorter than 100 
> characters, because we can read the diff and this commit message is too 
> long for cleanup patch.
>

Understood. Will keep it shorter in v3.

>>  }
>>  
>> -struct wlan_network *_rtw_find_same_network(struct __queue *scanned_queue, struct wlan_network *network)
>> +struct wlan_network *_rtw_find_same_network(struct __queue *scanned_queue,
>> +					    struct wlan_network *network)
> 					    ^^^^^
> This is still not aligned correctly. Please do it like:
> struct wlan_network *_rtw_find_same_network(struct __queue *scanned_queue,
> 					    struct wlan_network *network)
>

Will investigate and fix the alignment issues in v3.

>> @@ -462,10 +470,12 @@ static void update_current_network(struct adapter *adapter, struct wlan_bssid_ex
>>  		&pmlmepriv->cur_network.network,
>>  		&pmlmepriv->cur_network.network);
>>  
>> -	if (check_fwstate(pmlmepriv, _FW_LINKED) && (is_same_network(&pmlmepriv->cur_network.network, pnetwork, 0))) {
>> +	u8 *ie = pmlmepriv->cur_network.network.ies + sizeof(struct ndis_802_11_fix_ie);
>
> You should declare u8 *ie inside the if statement to reduce its scope as
> much as possible and if left this way it will declare it even if 
> condition is false.
>
>> +
>> +	if (check_fwstate(pmlmepriv, _FW_LINKED) &&
>> +	    is_same_network(&pmlmepriv->cur_network.network, pnetwork, 0)) {
>>  		update_network(&pmlmepriv->cur_network.network, pnetwork, adapter, true);
>
> Put u8 *ie ... here, under update_network.
>

Should variables always be declared at the nearest enclosing scope
where they're used, or at the top of the function?

>> -		rtw_update_protection(adapter, (pmlmepriv->cur_network.network.ies) + sizeof(struct ndis_802_11_fix_ie),
>> -								pmlmepriv->cur_network.network.ie_length);
>> +		rtw_update_protection(adapter, ie, pmlmepriv->cur_network.network.ie_length);
>>  	}
>>  }
>>  
>> @@ -600,12 +610,16 @@ static bool rtw_is_desired_network(struct adapter *adapter, struct wlan_network
>>  	desired_encmode = psecuritypriv->ndisencryptstatus;
>>  	privacy = pnetwork->network.privacy;
>>  
>> +	u8 *ie_ptr = pnetwork->network.ies + _FIXED_IE_LENGTH_;
>> +	u32 ie_len_val = pnetwork->network.ie_length - _FIXED_IE_LENGTH_;
>> +
>>  	if (check_fwstate(pmlmepriv, WIFI_UNDER_WPS)) {
>
> Again you should move two variables declared above here, inside if 
> statement. Additionally, calculating ie_len_val outside the check is 
> risky. You should ensure ie_length is actually greater than 
> _FIXED_IE_LENGTH_ before performing the subtraction to prevent an 
> unsigned underflow/wrap-around.
>

Should the guard be a simple early return if ie_length is smaller
than the offset, or is there a preferred pattern for this?

>> @@ -1072,10 +1113,13 @@ static void rtw_joinbss_update_network(struct adapter *padapter, struct wlan_net
>>  			break;
>>  	}
>>  
>> -	rtw_update_protection(padapter, (cur_network->network.ies) + sizeof(struct ndis_802_11_fix_ie),
>> -									(cur_network->network.ie_length));
>> +	u8 *ie = cur_network->network.ies + sizeof(struct ndis_802_11_fix_ie);
>>  
>> -	rtw_update_ht_cap(padapter, cur_network->network.ies, cur_network->network.ie_length, (u8) cur_network->network.configuration.ds_config);
>> +	rtw_update_protection(padapter, ie, (cur_network->network.ie_length));
>
> The original code had a potential buffer over-read here by passing the 
> full ie_length with an offset pointer. Since you are refactoring this 
> line, please fix this bug, in a separate patch, by subtracting the 
> offset from the length and ensuring the length is actually large enough 
> first. 1/X of the patch series should be bug fix, 2/X should be this 
> cleanup. Don't forget to add Fixes and Reported-by tags. Check if this 
> bug is present in stable tree and if so add Cc: stable@vger.kernel.org 
> as well.
>

Should this bug fix be in the same patch that will fix previously
mentioned subtraction underflow(s)?

>> @@ -1684,7 +1764,9 @@ static int rtw_check_roaming_candidate(struct mlme_priv *mlme
>>  	if (jiffies_to_msecs(jiffies - competitor->last_scanned) >= mlme->roam_scanr_exp_ms)
>>  		goto exit;
>>  
>> -	if (competitor->network.rssi - mlme->cur_network_scanned->network.rssi < mlme->roam_rssi_diff_th)
>> +	long rssi_diff = competitor->network.rssi - mlme->cur_network_scanned->network.rssi;
>
> You are declaring a variable (rssi_diff) after executable code again. 
> All declarations must be at the top of the function. Additionally, why 
> use long here?
>

The type of rssi in struct wlan_bssid_ex is long so I matched it.
Will double check and correct if needed.

Cheers,
Salman Alghamdi.