drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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 <a.safin@rosa.ru>
---
v2: change reciepent
drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index 78dee8ccfebf..f60d9b9798c1 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -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)
Hi
On Thu, Apr 24, 2025 at 09:52:44PM +0300, Alexei Safin wrote:
> 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];
> 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 <a.safin@rosa.ru>
> ---
> v2: change reciepent
> drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> index 78dee8ccfebf..f60d9b9798c1 100644
> --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> @@ -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
> /* For 5 GHZ band, remap mac80211 rate indices into driver indices */
> if (info->band == NL80211_BAND_5GHZ)
> --
> 2.39.5 (Apple Git-154)
>
Hello, On Sun, 27. Apr 08:39, Stanislaw Gruszka wrote: > > diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c > > index 78dee8ccfebf..f60d9b9798c1 100644 > > --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c > > +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c > > @@ -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 > > > /* 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; > > -- > > 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
Hi On Tue, Apr 29, 2025 at 08:15:59PM +0300, Fedor Pchelkin wrote: > Hello, > > On Sun, 27. Apr 08:39, Stanislaw Gruszka wrote: > > > diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c > > > index 78dee8ccfebf..f60d9b9798c1 100644 > > > --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c > > > +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c > > > @@ -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 > > > > > /* 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; > > > > -- > > > 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
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@ispras.ru>
Reported-by: Alexei Safin <a.safin@rosa.ru>
Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
---
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)
+ /* 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 */
--
2.25.4
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@ispras.ru> > Reported-by: Alexei Safin <a.safin@rosa.ru> > Signed-off-by: Stanislaw Gruszka <stf_xl@wp.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
Hi Fedor, thanks for review,
On Sat, May 17, 2025 at 03:21:03PM +0300, Fedor Pchelkin wrote:
> 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@ispras.ru>
> > Reported-by: Alexei Safin <a.safin@rosa.ru>
> > Signed-off-by: Stanislaw Gruszka <stf_xl@wp.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 ?
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.
>
> > + /* 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..
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
Limit rate_idx to IL_LAST_OFDM_RATE for 5GHz band for thinkable case
the index is incorrect.
Reported-by: Fedor Pchelkin <pchelkin@ispras.ru>
Reported-by: Alexei Safin <a.safin@rosa.ru>
Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
---
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(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index dc8c408902e6..4d2148147b94 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -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 */
--
2.25.4
On Sun, 25. May 16:45, Stanislaw Gruszka wrote:
> Limit rate_idx to IL_LAST_OFDM_RATE for 5GHz band for thinkable case
> the index is incorrect.
>
> Reported-by: Fedor Pchelkin <pchelkin@ispras.ru>
> Reported-by: Alexei Safin <a.safin@rosa.ru>
> Signed-off-by: Stanislaw Gruszka <stf_xl@wp.pl>
> ---
> 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(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> index dc8c408902e6..4d2148147b94 100644
> --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> @@ -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 */
> --
> 2.25.4
>
Thanks, Stanislaw!
To my mind, it looks reasonable. FWIW,
Reviewed-by: Fedor Pchelkin <pchelkin@ispras.ru>
Alexei Safin <a.safin@rosa.ru> wrote:
>
> 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")
The Fixes is obviously wrong. It just moved the code.
> Signed-off-by: Alexei Safin <a.safin@rosa.ru>
> ---
> v2: change reciepent
> drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> index 78dee8ccfebf..f60d9b9798c1 100644
> --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> @@ -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)
>
© 2016 - 2026 Red Hat, Inc.