wifi: iwlegacy: 3945-rs: fix possible null-pointer dereferences in il3945_rs_get_rate()

[PATCH v2] wifi: iwlegacy: 3945-rs: fix possible null-pointer dereferences in il3945_rs_get_rate()

Posted by Tuo Li 1 month ago

In this function, il_sta is assigned to rs_sta, and rs_sta is dereferenced
at several points. If il_sta is NULL, this can lead to null-pointer
dereferences. To fix this issue, add an early check for il_sta and return
if it is NULL, consistent with the handling in il3945_rs_tx_status().

Besides, if the STA il data is uninitialized, return early instead of
setting il_sta to NULL, consistent with the handling in
il3945_rs_tx_status().

Signed-off-by: Tuo Li <islituo@gmail.com>
---
v2:
* Return early for uninitialized STA il data and align D_RATE messages with
  il3945_rs_tx_status(). Add a wifi: prefix to the patch title.
  Thanks to Stanislaw Gruszka for the helpful advice.
---
 drivers/net/wireless/intel/iwlegacy/3945-rs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/3945-rs.c b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
index 1826c37c090c..c509c89bba00 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945-rs.c
+++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
@@ -626,10 +626,15 @@ il3945_rs_get_rate(void *il_r, struct ieee80211_sta *sta, void *il_sta,
 
 	D_RATE("enter\n");
 
+	if (!il_sta) {
+		D_RATE("leave: No STA il data to update!\n");
+		return;
+	}
+
 	/* Treat uninitialized rate scaling data same as non-existing. */
-	if (rs_sta && !rs_sta->il) {
-		D_RATE("Rate scaling information not initialized yet.\n");
-		il_sta = NULL;
+	if (!rs_sta->il) {
+		D_RATE("leave: STA il data uninitialized!\n");
+		return;
 	}
 
 	rate_mask = sta->deflink.supp_rates[sband->band];
-- 
2.43.0

Re: [PATCH v2] wifi: iwlegacy: 3945-rs: fix possible null-pointer dereferences in il3945_rs_get_rate()

Posted by Stanislaw Gruszka 1 month ago

On Wed, Jan 07, 2026 at 04:41:49PM +0800, Tuo Li wrote:
> In this function, il_sta is assigned to rs_sta, and rs_sta is dereferenced
> at several points. If il_sta is NULL, this can lead to null-pointer
> dereferences. To fix this issue, add an early check for il_sta and return
> if it is NULL, consistent with the handling in il3945_rs_tx_status().
> 
> Besides, if the STA il data is uninitialized, return early instead of
> setting il_sta to NULL, consistent with the handling in
> il3945_rs_tx_status().
> 
> Signed-off-by: Tuo Li <islituo@gmail.com>
Acked-by: Stanislaw Gruszka <stf_xl@wp.pl>

Thanks
Stanislaw
> ---
> v2:
> * Return early for uninitialized STA il data and align D_RATE messages with
>   il3945_rs_tx_status(). Add a wifi: prefix to the patch title.
>   Thanks to Stanislaw Gruszka for the helpful advice.
> ---
>  drivers/net/wireless/intel/iwlegacy/3945-rs.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlegacy/3945-rs.c b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
> index 1826c37c090c..c509c89bba00 100644
> --- a/drivers/net/wireless/intel/iwlegacy/3945-rs.c
> +++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
> @@ -626,10 +626,15 @@ il3945_rs_get_rate(void *il_r, struct ieee80211_sta *sta, void *il_sta,
>  
>  	D_RATE("enter\n");
>  
> +	if (!il_sta) {
> +		D_RATE("leave: No STA il data to update!\n");
> +		return;
> +	}
> +
>  	/* Treat uninitialized rate scaling data same as non-existing. */
> -	if (rs_sta && !rs_sta->il) {
> -		D_RATE("Rate scaling information not initialized yet.\n");
> -		il_sta = NULL;
> +	if (!rs_sta->il) {
> +		D_RATE("leave: STA il data uninitialized!\n");
> +		return;
>  	}
>  
>  	rate_mask = sta->deflink.supp_rates[sband->band];
> -- 
> 2.43.0
>

Re: [PATCH v2] wifi: iwlegacy: 3945-rs: fix possible null-pointer dereferences in il3945_rs_get_rate()

Posted by Johannes Berg 1 month ago

On Wed, 2026-01-07 at 09:59 +0100, Stanislaw Gruszka wrote:
> On Wed, Jan 07, 2026 at 04:41:49PM +0800, Tuo Li wrote:
> > In this function, il_sta is assigned to rs_sta, and rs_sta is dereferenced
> > at several points. If il_sta is NULL, this can lead to null-pointer
> > dereferences. To fix this issue, add an early check for il_sta and return
> > if it is NULL, consistent with the handling in il3945_rs_tx_status().
> > 
> > Besides, if the STA il data is uninitialized, return early instead of
> > setting il_sta to NULL, consistent with the handling in
> > il3945_rs_tx_status().
> > 
> > Signed-off-by: Tuo Li <islituo@gmail.com>
> Acked-by: Stanislaw Gruszka <stf_xl@wp.pl>

I can apply this if you want, but for the record,

> > +++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
> > @@ -626,10 +626,15 @@ il3945_rs_get_rate(void *il_r, struct ieee80211_sta *sta, void *il_sta,
> >  
> >  	D_RATE("enter\n");
> >  
> > +	if (!il_sta) {
> > +		D_RATE("leave: No STA il data to update!\n");
> > +		return;
> > +	}
> > +

I don't see how this would be possible. _Maybe_ the other one, but I
can't figure out any scenario in mac80211 where it could happen either.

johannes

Re: [PATCH v2] wifi: iwlegacy: 3945-rs: fix possible null-pointer dereferences in il3945_rs_get_rate()

Posted by Tuo Li 1 month ago

Hi Johannes,

On Thu, Jan 8, 2026 at 8:02 PM Johannes Berg <johannes@sipsolutions.net> wrote:
> I can apply this if you want, but for the record,
>
> > > +++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
> > > @@ -626,10 +626,15 @@ il3945_rs_get_rate(void *il_r, struct ieee80211_sta *sta, void *il_sta,
> > >
> > >     D_RATE("enter\n");
> > >
> > > +   if (!il_sta) {
> > > +           D_RATE("leave: No STA il data to update!\n");
> > > +           return;
> > > +   }
> > > +
>
> I don't see how this would be possible. _Maybe_ the other one, but I
> can't figure out any scenario in mac80211 where it could happen either.
>
> johannes

Thanks for the clarification.

I don't have a concrete mac80211 execution path that would result in
il_sta being NULL here either. This issue was reported by a static
analysis tool, and after reviewing the code I noticed that the handling is
not consistent with il3945_rs_tx_status(), which is why I submitted this
patch to add a defensive check.

If you believe this situation cannot occur in practice and the additional
check is unnecessary, I'm fine with dropping this change.

Thanks for taking the time to review this.

Best regards,
Tuo

Re: [PATCH v2] wifi: iwlegacy: 3945-rs: fix possible null-pointer dereferences in il3945_rs_get_rate()

Posted by Stanislaw Gruszka 1 month ago

On Thu, Jan 08, 2026 at 09:28:30PM +0800, Tuo Li wrote:
> On Thu, Jan 8, 2026 at 8:02 PM Johannes Berg <johannes@sipsolutions.net> wrote:
> > I can apply this if you want, but for the record,
> >
> > > > +++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
> > > > @@ -626,10 +626,15 @@ il3945_rs_get_rate(void *il_r, struct ieee80211_sta *sta, void *il_sta,
> > > >
> > > >     D_RATE("enter\n");
> > > >
> > > > +   if (!il_sta) {
> > > > +           D_RATE("leave: No STA il data to update!\n");
> > > > +           return;
> > > > +   }
> > > > +
> >
> > I don't see how this would be possible. _Maybe_ the other one, but I
> > can't figure out any scenario in mac80211 where it could happen either.

Regarding checking the rs_sta->il, we can get rid of the ->il
backpointer, it's only used for printing debug messages in a few 
functions. I don't think person needing to debug 3945 rate scaling
algorithm exist nowadays :-)

I'll send patch for that.

> I don't have a concrete mac80211 execution path that would result in
> il_sta being NULL here either. This issue was reported by a static
> analysis tool, and after reviewing the code I noticed that the handling is
> not consistent with il3945_rs_tx_status(), which is why I submitted this
> patch to add a defensive check.

IMO is ok to have defensive checks (in reasonable amount :-)

They can be marked with WARN_ON_ONCE like this:

   if (WARN_ON_ONCE(!il_sta))
   	return

that would clearly indicate the check is for 'not possible' scenario.

Regards
Stanislaw

Re: [PATCH v2] wifi: iwlegacy: 3945-rs: fix possible null-pointer dereferences in il3945_rs_get_rate()

Posted by Tuo Li 4 weeks, 1 day ago

On Fri, Jan 9, 2026 at 12:33 AM Stanislaw Gruszka <stf_xl@wp.pl> wrote:
>
> On Thu, Jan 08, 2026 at 09:28:30PM +0800, Tuo Li wrote:
> > On Thu, Jan 8, 2026 at 8:02 PM Johannes Berg <johannes@sipsolutions.net> wrote:
> > > I can apply this if you want, but for the record,
> > >
> > > > > +++ b/drivers/net/wireless/intel/iwlegacy/3945-rs.c
> > > > > @@ -626,10 +626,15 @@ il3945_rs_get_rate(void *il_r, struct ieee80211_sta *sta, void *il_sta,
> > > > >
> > > > >     D_RATE("enter\n");
> > > > >
> > > > > +   if (!il_sta) {
> > > > > +           D_RATE("leave: No STA il data to update!\n");
> > > > > +           return;
> > > > > +   }
> > > > > +
> > >
> > > I don't see how this would be possible. _Maybe_ the other one, but I
> > > can't figure out any scenario in mac80211 where it could happen either.
>
> Regarding checking the rs_sta->il, we can get rid of the ->il
> backpointer, it's only used for printing debug messages in a few
> functions. I don't think person needing to debug 3945 rate scaling
> algorithm exist nowadays :-)
>
> I'll send patch for that.
>
> > I don't have a concrete mac80211 execution path that would result in
> > il_sta being NULL here either. This issue was reported by a static
> > analysis tool, and after reviewing the code I noticed that the handling is
> > not consistent with il3945_rs_tx_status(), which is why I submitted this
> > patch to add a defensive check.
>
> IMO is ok to have defensive checks (in reasonable amount :-)
>
> They can be marked with WARN_ON_ONCE like this:
>
>    if (WARN_ON_ONCE(!il_sta))
>         return
>
> that would clearly indicate the check is for 'not possible' scenario.
>
> Regards
> Stanislaw
>
>

Hi Stanislaw,

Thanks for your reply.

I will add a defensive WARN_ON_ONCE() at the beginning of
il3945_rs_get_rate() to catch this unexpected condition, and will submit a
v3 patch accordingly.

Best regards,
Tuo