iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate()

PATCH In review Alexei Safin 4 participants 10 messages Apr 24, 2025 – May 27, 2025

Subsystems (3)

Networking Network drivers Intel wireless 3945ABG/bg, 4965AGN (iwlegacy)

lore

Share & tools

lore

Alexei Safin

author

2025-04-24 18:52 UTC #0 lore raw

PATCH v2 1/1 iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate()

Prevent out-of-bounds access in il4965_tx_cmd_build_rate() by rejecting rate_idx values greater than or equal to RATE_COUNT_LEGACY.

Use a correct bounds check to avoid accessing il_rates[] with an invalid index. The previous comparison allowed rate_idx to become equal to RATE_COUNT_LEGACY, which exceeds the array limit.

Replace the check 'rate_idx > RATE_COUNT_LEGACY' with 'rate_idx >= RATE_COUNT_LEGACY' to ensure memory safety.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes 7ac9a364c172 ("iwlegacy: move under intel directory")

Signed-off-by Alexei Safin <[email protected]>

---
v2: change reciepent

 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/intel/iwlegacy/4965-mac.c +1 -1

index 78dee8ccfebf..f60d9b9798c1 100644
@@ -1572,7 +1572,7 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
 	 */
 	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)
+	    || rate_idx >= RATE_COUNT_LEGACY)
 		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)

Signature · 1 line

2.39.5 (Apple Git-154)

Ping-Ke Shih

2025-04-25 06:00 UTC #1 lore raw

Quoted 8 lines · Alexei Safin

Prevent out-of-bounds access in il4965_tx_cmd_build_rate() by rejecting rate_idx values greater than or equal to RATE_COUNT_LEGACY.
Use a correct bounds check to avoid accessing il_rates[] with an invalid index. The previous comparison allowed rate_idx to become equal to RATE_COUNT_LEGACY, which exceeds the array limit.

Quoted 6 lines

Replace the check 'rate_idx > RATE_COUNT_LEGACY' with 'rate_idx >= RATE_COUNT_LEGACY' to ensure memory safety.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes 7ac9a364c172 ("iwlegacy: move under intel directory")

The Fixes is obviously wrong. It just moved the code.

Quoted 23 lines

Signed-off-by Alexei Safin <[email protected]>

---
v2: change reciepent

 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/intel/iwlegacy/4965-mac.c +1 -2

index 78dee8ccfebf..f60d9b9798c1 100644
@@ -1572,7 +1572,7 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
         */
        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)
+           || rate_idx >= RATE_COUNT_LEGACY)
                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)
--

2.39.5 (Apple Git-154)

Stanislaw Gruszka

maintainer

2025-04-27 06:45 UTC #2 lore raw

In reply to Alexei Safin

Quoted 6 lines · Alexei Safin

Prevent out-of-bounds access in il4965_tx_cmd_build_rate() by rejecting rate_idx values greater than or equal to RATE_COUNT_LEGACY.
Use a correct bounds check to avoid accessing il_rates[] with an invalid index. The previous comparison allowed rate_idx to become equal to RATE_COUNT_LEGACY, which exceeds the array limit.

Thanks for the patch, however I think it's not correct.

The definitions are:

enum {
        RATE_1M_IDX = 0,
	...
	RATE_54M_INDEX,
	RATE_60M_INDEX,
	RATE_COUNT
        RATE_COUNT_LEGACY = RATE_COUNT - 1,     /* Excluding 60M */
}

extern const struct il_rate_info il_rates[RATE_COUNT];

Quoted 17 lines

Replace the check 'rate_idx > RATE_COUNT_LEGACY' with 'rate_idx >= RATE_COUNT_LEGACY' to ensure memory safety.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes 7ac9a364c172 ("iwlegacy: move under intel directory")
Signed-off-by Alexei Safin <[email protected]>
---
v2: change reciepent
 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/intel/iwlegacy/4965-mac.c +0 -0
index 78dee8ccfebf..f60d9b9798c1 100644

drivers/net/wireless/intel/iwlegacy/4965-mac.c il4965_tx_cmd_build_rate(struct il_priv *il, · line 1576

Quoted 6 lines

drivers/net/wireless/intel/iwlegacy/4965-mac.c +1 -1

@@ -1572,7 +1572,7 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
 	 */
 	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)
+	    || rate_idx >= RATE_COUNT_LEGACY)
 		rate_idx = rate_lowest_index(&il->bands[info->band], sta);

.. so looks the check is fine already and changing it will induce a bug for RATE_54M_INDEX.

Regards
Stanislaw

Quoted 5 lines

drivers/net/wireless/intel/iwlegacy/4965-mac.c +0 -0
 	/* For 5 GHZ band, remap mac80211 rate indices into driver indices */
 	if (info->band == NL80211_BAND_5GHZ)
Signature · 1 line
2.39.5 (Apple Git-154)

Fedor Pchelkin

2025-04-29 17:16 UTC #3 lore raw

Hello,

Quoted 15 lines · Stanislaw Gruszka

drivers/net/wireless/intel/iwlegacy/4965-mac.c il4965_tx_cmd_build_rate(struct il_priv *il, · line 1576
Quoted 11 lines
drivers/net/wireless/intel/iwlegacy/4965-mac.c +1 -1
index 78dee8ccfebf..f60d9b9798c1 100644
@@ -1572,7 +1572,7 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
 	 */
 	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)
+	    || rate_idx >= RATE_COUNT_LEGACY)
 		rate_idx = rate_lowest_index(&il->bands[info->band], sta);
.. so looks the check is fine already and changing it will induce a bug for RATE_54M_INDEX.

Quoted 5 lines

Regards
Stanislaw
Quoted 2 lines
drivers/net/wireless/intel/iwlegacy/4965-mac.c +0 -0
 	/* For 5 GHZ band, remap mac80211 rate indices into driver indices */
 	if (info->band == NL80211_BAND_5GHZ)

Here goes the fragment:

		rate_idx += IL_FIRST_OFDM_RATE;
	/* Get PLCP rate for tx_cmd->rate_n_flags */
	rate_plcp = il_rates[rate_idx].plcp;

Quoted 3 lines

Quoted 3 lines
Signature · 1 line
2.39.5 (Apple Git-154)

Looks like the proper checks should be added to address the 5GHZ case and validate that the index won't exceed the array boundaries after being shifted by IL_FIRST_OFDM_RATE.

--
Thanks,
Fedor

Stanislaw Gruszka

maintainer

2025-04-29 21:40 UTC #4 lore raw

Quoted 30 lines · Fedor Pchelkin

Hello,
Quoted 15 lines · Stanislaw Gruszka
drivers/net/wireless/intel/iwlegacy/4965-mac.c il4965_tx_cmd_build_rate(struct il_priv *il, · line 1576
Quoted 11 lines
drivers/net/wireless/intel/iwlegacy/4965-mac.c +1 -1
index 78dee8ccfebf..f60d9b9798c1 100644
@@ -1572,7 +1572,7 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
 	 */
 	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)
+	    || rate_idx >= RATE_COUNT_LEGACY)
 		rate_idx = rate_lowest_index(&il->bands[info->band], sta);
.. so looks the check is fine already and changing it will induce a bug for RATE_54M_INDEX.
Quoted 5 lines
Regards
Stanislaw
Quoted 2 lines
drivers/net/wireless/intel/iwlegacy/4965-mac.c +0 -0
 	/* For 5 GHZ band, remap mac80211 rate indices into driver indices */
 	if (info->band == NL80211_BAND_5GHZ)
Here goes the fragment:
		rate_idx += IL_FIRST_OFDM_RATE;
	/* Get PLCP rate for tx_cmd->rate_n_flags */
	rate_plcp = il_rates[rate_idx].plcp;

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
Signature · 1 line
2.39.5 (Apple Git-154)
Looks like the proper checks should be added to address the 5GHZ case and validate that the index won't exceed the array boundaries after being shifted by IL_FIRST_OFDM_RATE.

Good point. It make sense to move rate_idx range check after possible IL_FIRST_OFDM_RATE addition for 5GHz.

Regards
Stanislaw

Stanislaw Gruszka

maintainer

2025-05-17 07:47 UTC #5 lore raw

PATCH 1/1 wifi: iwlegacy: Check rate_idx range after addition

In reply to Fedor Pchelkin

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 <[email protected]>

Reported-by Alexei Safin <[email protected]>

Signed-off-by Stanislaw Gruszka <[email protected]>

---
 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

drivers/net/wireless/intel/iwlegacy/4965-mac.c +9 -6

index dc8c408902e6..2294ea43b4c7 100644
@@ -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)
+		/* 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)
+		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 */

Signature · 1 line

2.25.4

Fedor Pchelkin

2025-05-17 12:21 UTC #6 lore raw

Re: [PATCH] wifi: iwlegacy: Check rate_idx range after addition

Quoted 11 lines · Stanislaw Gruszka

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 <[email protected]>
Reported-by Alexei Safin <[email protected]>
Signed-off-by Stanislaw Gruszka <[email protected]>
---

Thank you for the patch, Stanislaw!

Please see some comments below.

Quoted 13 lines

 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

drivers/net/wireless/intel/iwlegacy/4965-mac.c +0 -2

index dc8c408902e6..2294ea43b4c7 100644
@@ -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.

drivers/net/wireless/intel/iwlegacy/4965-mac.c il4965_tx_cmd_build_rate(struct il_priv *il, · line 1575

Quoted 10 lines

drivers/net/wireless/intel/iwlegacy/4965-mac.c +3 -4

+	 * 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 ?

drivers/net/wireless/intel/iwlegacy/4965-mac.c il4965_tx_cmd_build_rate(struct il_priv *il, · line 1580

Quoted 5 lines

drivers/net/wireless/intel/iwlegacy/4965-mac.c +4 -0

+		/* 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..

Quoted 7 lines

drivers/net/wireless/intel/iwlegacy/4965-mac.c +2 -0

+		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 */

Signature · 1 line

2.25.4

Stanislaw Gruszka

maintainer

2025-05-17 14:53 UTC #7 lore raw

Re: [PATCH] wifi: iwlegacy: Check rate_idx range after addition

Hi Fedor, thanks for review,

Quoted 35 lines · Fedor Pchelkin

Quoted 11 lines · Stanislaw Gruszka
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 <[email protected]>
Reported-by Alexei Safin <[email protected]>
Signed-off-by Stanislaw Gruszka <[email protected]>
---
Thank you for the patch, Stanislaw!
Please see some comments below.
Quoted 18 lines
 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/net/wireless/intel/iwlegacy/4965-mac.c +1 -4
index dc8c408902e6..2294ea43b4c7 100644
@@ -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)

Quoted 8 lines

drivers/net/wireless/intel/iwlegacy/4965-mac.c il4965_tx_cmd_build_rate(struct il_priv *il, · line 1575
Quoted 5 lines
drivers/net/wireless/intel/iwlegacy/4965-mac.c +2 -2
+	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 ?

Right, this is wrong. I thought we can use index returned by rate_lowest_index() but we still should add IL_FIRST_OFDM_RATE. At least this is how is done now.

Quoted 15 lines

drivers/net/wireless/intel/iwlegacy/4965-mac.c il4965_tx_cmd_build_rate(struct il_priv *il, · line 1580
Quoted 6 lines
drivers/net/wireless/intel/iwlegacy/4965-mac.c +4 -0
+		/* 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);

Quoted 3 lines

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..

Indeed this is strange. I'm not sure why those checks differ.

Anyway for the rate_idx in il4965_tx_cmd_build_rate() for 5GHs I'll just add additional check like below:

	if (info->band == NL80211_BAND_5GHZ) {
		rate_idx += IL_FIRST_OFDM_RATE;
		if (rate_idx > IL_LAST_OFDM_RATE);
			rate_idx = IL_LAST_OFDM_RATE;
	}

This patch should be dropped.

Regards
Stanislaw

Stanislaw Gruszka

maintainer

2025-05-25 14:52 UTC #8 lore raw

PATCH v2 1/1 wifi: iwlegacy: Check rate_idx range after addition

In reply to Fedor Pchelkin

Limit rate_idx to IL_LAST_OFDM_RATE for 5GHz band for thinkable case the index is incorrect.

Reported-by Fedor Pchelkin <[email protected]>

Reported-by Alexei Safin <[email protected]>

Signed-off-by Stanislaw Gruszka <[email protected]>

---
v1 -> v2:
- just add check one possible case the index could be incorrect, instead of doing broader changes.

 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/net/wireless/intel/iwlegacy/4965-mac.c +4 -1

index dc8c408902e6..4d2148147b94 100644
@@ -1575,8 +1575,11 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
 	    || rate_idx > RATE_COUNT_LEGACY)
 		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)
+	if (info->band == NL80211_BAND_5GHZ) {
 		rate_idx += IL_FIRST_OFDM_RATE;
+		if (rate_idx > IL_LAST_OFDM_RATE)
+			rate_idx = IL_LAST_OFDM_RATE;
+	}
 	/* Get PLCP rate for tx_cmd->rate_n_flags */
 	rate_plcp = il_rates[rate_idx].plcp;
 	/* Zero out flags for this packet */

Signature · 1 line

2.25.4

Fedor Pchelkin

Reviewed-by on v2 1/1

2025-05-27 19:17 UTC #9 lore raw

Re: [PATCH v2] wifi: iwlegacy: Check rate_idx range after addition

Quoted 34 lines · Stanislaw Gruszka

Limit rate_idx to IL_LAST_OFDM_RATE for 5GHz band for thinkable case the index is incorrect.
Reported-by Fedor Pchelkin <[email protected]>
Reported-by Alexei Safin <[email protected]>
Signed-off-by Stanislaw Gruszka <[email protected]>
---
v1 -> v2:
- just add check one possible case the index could be incorrect, instead of doing broader changes.
 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/wireless/intel/iwlegacy/4965-mac.c +4 -1
index dc8c408902e6..4d2148147b94 100644
@@ -1575,8 +1575,11 @@ il4965_tx_cmd_build_rate(struct il_priv *il,
 	    || rate_idx > RATE_COUNT_LEGACY)
 		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)
+	if (info->band == NL80211_BAND_5GHZ) {
 		rate_idx += IL_FIRST_OFDM_RATE;
+		if (rate_idx > IL_LAST_OFDM_RATE)
+			rate_idx = IL_LAST_OFDM_RATE;
+	}
 	/* Get PLCP rate for tx_cmd->rate_n_flags */
 	rate_plcp = il_rates[rate_idx].plcp;
 	/* Zero out flags for this packet */
Signature · 1 line
2.25.4

Thanks, Stanislaw!

To my mind, it looks reasonable. FWIW,

Reviewed-by Fedor Pchelkin <[email protected]>