* [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate()
@ 2025-04-24 18:52 Alexei Safin
2025-04-25 5:59 ` Ping-Ke Shih
2025-04-27 6:39 ` Stanislaw Gruszka
0 siblings, 2 replies; 10+ messages in thread
From: Alexei Safin @ 2025-04-24 18:52 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Alexei Safin, Kalle Valo, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-wireless, netdev, linux-kernel,
lvc-project
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)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate()
2025-04-24 18:52 [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate() Alexei Safin
@ 2025-04-25 5:59 ` Ping-Ke Shih
2025-04-27 6:39 ` Stanislaw Gruszka
1 sibling, 0 replies; 10+ messages in thread
From: Ping-Ke Shih @ 2025-04-25 5:59 UTC (permalink / raw)
To: Alexei Safin, Stanislaw Gruszka
Cc: Kalle Valo, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
lvc-project@linuxtesting.org
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)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate()
2025-04-24 18:52 [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate() Alexei Safin
2025-04-25 5:59 ` Ping-Ke Shih
@ 2025-04-27 6:39 ` Stanislaw Gruszka
2025-04-29 17:15 ` Fedor Pchelkin
1 sibling, 1 reply; 10+ messages in thread
From: Stanislaw Gruszka @ 2025-04-27 6:39 UTC (permalink / raw)
To: Alexei Safin
Cc: Kalle Valo, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-wireless, netdev, linux-kernel, lvc-project
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)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate()
2025-04-27 6:39 ` Stanislaw Gruszka
@ 2025-04-29 17:15 ` Fedor Pchelkin
2025-04-29 21:40 ` Stanislaw Gruszka
2025-05-17 7:40 ` [PATCH] wifi: iwlegacy: Check rate_idx range after addition Stanislaw Gruszka
0 siblings, 2 replies; 10+ messages in thread
From: Fedor Pchelkin @ 2025-04-29 17:15 UTC (permalink / raw)
To: Stanislaw Gruszka, Alexei Safin
Cc: lvc-project, netdev, Kalle Valo, linux-wireless, linux-kernel,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, David S . Miller
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate()
2025-04-29 17:15 ` Fedor Pchelkin
@ 2025-04-29 21:40 ` Stanislaw Gruszka
2025-05-17 7:40 ` [PATCH] wifi: iwlegacy: Check rate_idx range after addition Stanislaw Gruszka
1 sibling, 0 replies; 10+ messages in thread
From: Stanislaw Gruszka @ 2025-04-29 21:40 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Alexei Safin, lvc-project, netdev, Kalle Valo, linux-wireless,
linux-kernel, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David S . Miller
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] wifi: iwlegacy: Check rate_idx range after addition
2025-04-29 17:15 ` Fedor Pchelkin
2025-04-29 21:40 ` Stanislaw Gruszka
@ 2025-05-17 7:40 ` Stanislaw Gruszka
2025-05-17 12:21 ` Fedor Pchelkin
1 sibling, 1 reply; 10+ messages in thread
From: Stanislaw Gruszka @ 2025-05-17 7:40 UTC (permalink / raw)
To: Fedor Pchelkin, Johannes Berg
Cc: Alexei Safin, lvc-project, netdev, Kalle Valo, linux-wireless,
linux-kernel, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David S . Miller
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] wifi: iwlegacy: Check rate_idx range after addition
2025-05-17 7:40 ` [PATCH] wifi: iwlegacy: Check rate_idx range after addition Stanislaw Gruszka
@ 2025-05-17 12:21 ` Fedor Pchelkin
2025-05-17 14:53 ` Stanislaw Gruszka
2025-05-25 14:45 ` [PATCH v2] " Stanislaw Gruszka
0 siblings, 2 replies; 10+ messages in thread
From: Fedor Pchelkin @ 2025-05-17 12:21 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Johannes Berg, Alexei Safin, lvc-project, netdev, Kalle Valo,
linux-wireless, linux-kernel, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, David S . Miller
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] wifi: iwlegacy: Check rate_idx range after addition
2025-05-17 12:21 ` Fedor Pchelkin
@ 2025-05-17 14:53 ` Stanislaw Gruszka
2025-05-25 14:45 ` [PATCH v2] " Stanislaw Gruszka
1 sibling, 0 replies; 10+ messages in thread
From: Stanislaw Gruszka @ 2025-05-17 14:53 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Johannes Berg, Alexei Safin, lvc-project, netdev, Kalle Valo,
linux-wireless, linux-kernel, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, David S . Miller
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] wifi: iwlegacy: Check rate_idx range after addition
2025-05-17 12:21 ` Fedor Pchelkin
2025-05-17 14:53 ` Stanislaw Gruszka
@ 2025-05-25 14:45 ` Stanislaw Gruszka
2025-05-27 19:17 ` Fedor Pchelkin
1 sibling, 1 reply; 10+ messages in thread
From: Stanislaw Gruszka @ 2025-05-25 14:45 UTC (permalink / raw)
To: Fedor Pchelkin, Johannes Berg
Cc: Alexei Safin, lvc-project, netdev, Kalle Valo, linux-wireless,
linux-kernel, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
David S . Miller
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] wifi: iwlegacy: Check rate_idx range after addition
2025-05-25 14:45 ` [PATCH v2] " Stanislaw Gruszka
@ 2025-05-27 19:17 ` Fedor Pchelkin
0 siblings, 0 replies; 10+ messages in thread
From: Fedor Pchelkin @ 2025-05-27 19:17 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Johannes Berg, Alexei Safin, lvc-project, netdev, Kalle Valo,
linux-wireless, linux-kernel, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, David S . Miller
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-05-27 19:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-24 18:52 [PATCH v2] iwlegacy: 4965: fix possible out-of-bounds access in il4965_tx_cmd_build_rate() Alexei Safin
2025-04-25 5:59 ` Ping-Ke Shih
2025-04-27 6:39 ` Stanislaw Gruszka
2025-04-29 17:15 ` Fedor Pchelkin
2025-04-29 21:40 ` Stanislaw Gruszka
2025-05-17 7:40 ` [PATCH] wifi: iwlegacy: Check rate_idx range after addition Stanislaw Gruszka
2025-05-17 12:21 ` Fedor Pchelkin
2025-05-17 14:53 ` Stanislaw Gruszka
2025-05-25 14:45 ` [PATCH v2] " Stanislaw Gruszka
2025-05-27 19:17 ` Fedor Pchelkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).