linux-kernel - Re: [PATCH] wifi: iwlegacy: Check rate

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: <hrpy3omokg5zvrqnchx4jvp26bvfgdrashkmrjonsyz5b64aaz@6d5kn7z7x73q>
Date: Sat, 17 May 2025 15:21:03 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Stanislaw Gruszka <stf_xl@...pl>
Cc: Johannes Berg <johannes@...solutions.net>, 
	Alexei Safin <a.safin@...a.ru>, lvc-project@...uxtesting.org, netdev@...r.kernel.org, 
	Kalle Valo <kvalo@...nel.org>, linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH] wifi: iwlegacy: Check rate_idx range after addition

On Sat, 17. May 09:40, Stanislaw Gruszka wrote:
> Move rate_idx range check after we add IL_FIRST_OFDM_RATE for it
> for 5GHz band.
> 
> Additionally use ">= RATE_COUNT" check instead of "> RATE_COUNT_LEGACY"
> to avoid possible reviewers and static code analyzers confusion about
> size of il_rate array.
> 
> Reported-by: Fedor Pchelkin <pchelkin@...ras.ru>
> Reported-by: Alexei Safin <a.safin@...a.ru>
> Signed-off-by: Stanislaw Gruszka <stf_xl@...pl>
> ---

Thank you for the patch, Stanislaw!

Please see some comments below.

>  drivers/net/wireless/intel/iwlegacy/4965-mac.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> index dc8c408902e6..2294ea43b4c7 100644
> --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> @@ -1567,16 +1567,19 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
>  	/**
>  	 * If the current TX rate stored in mac80211 has the MCS bit set, it's
>  	 * not really a TX rate.  Thus, we use the lowest supported rate for
> -	 * this band.  Also use the lowest supported rate if the stored rate
> -	 * idx is invalid.
> +	 * this band.
>  	 */
>  	rate_idx = info->control.rates[0].idx;
> -	if ((info->control.rates[0].flags & IEEE80211_TX_RC_MCS) || rate_idx < 0
> -	    || rate_idx > RATE_COUNT_LEGACY)
> +	if (info->control.rates[0].flags & IEEE80211_TX_RC_MCS)
>  		rate_idx = rate_lowest_index(&il->bands[info->band], sta);
> -	/* For 5 GHZ band, remap mac80211 rate indices into driver indices */
> -	if (info->band == NL80211_BAND_5GHZ)
> +	else if (info->band == NL80211_BAND_5GHZ)

5GHZ shouldn't be in 'else if' clause, I think. Is it mutually exclusive
with IEEE80211_TX_RC_MCS ?

> +		/* For 5 GHZ band, remap mac80211 rate indices into driver indices */
>  		rate_idx += IL_FIRST_OFDM_RATE;
> +
> +	/* Use the lowest supported rate if the stored rate idx is invalid. */
> +	if (rate_idx < 0 || rate_idx >= RATE_COUNT)

There is a check inside il4965_rs_get_rate():

	/* Check for invalid rates */
	if (rate_idx < 0 || rate_idx >= RATE_COUNT_LEGACY ||
	    (sband->band == NL80211_BAND_5GHZ &&
	     rate_idx < IL_FIRST_OFDM_RATE))
		rate_idx = rate_lowest_index(sband, sta);

so RATE_COUNT_LEGACY (60M) is considered invalid there but is accepted
here in il4965_tx_cmd_build_rate(). It looks strange, at least for the
fresh reader as me..

> +		rate_idx = rate_lowest_index(&il->bands[info->band], sta);
> +
>  	/* Get PLCP rate for tx_cmd->rate_n_flags */
>  	rate_plcp = il_rates[rate_idx].plcp;
>  	/* Zero out flags for this packet */
> -- 
> 2.25.4