iwlwifi: Read outside array bounds

Commit Message

Roel Kluin July 25, 2009, 9:34 p.m. UTC

tid is bounded (above) by the size of default_tid_to_tx_fifo (17 elements), but
the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements.

Signed-off-by: Roel Kluin <[e-mail]>
---
 		IWL_ERR(priv, "Start AGG when state is not IWL_AGG_OFF !
");
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Zhu Yi July 27, 2009, 1:41 a.m. UTC | #1

On Sun, 2009-07-26 at 05:34 +0800, Roel Kluin wrote:
> tid is bounded (above) by the size of default_tid_to_tx_fifo (17 elements), but
> the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements.

I think MAX_TID_COUNT should be defined as 16 or 17. Tomas?

Thanks,
-yi

> Signed-off-by: Roel Kluin <[e-mail]>
> ---
> diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c
> index 85ae7a6..e9441c6 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-tx.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-tx.c
> @@ -1170,6 +1170,8 @@ int iwl_tx_agg_start(struct iwl_priv *priv, const u8 *ra, u16 tid, u16 *ssn)
>  		IWL_ERR(priv, "Start AGG on invalid station
> ");
>  		return -ENXIO;
>  	}
> +	if (unlikely(tid >= MAX_TID_COUNT))
> +		return -EINVAL;
>  
>  	if (priv->stations[sta_id].tid[tid].agg.state != IWL_AGG_OFF) {
>  		IWL_ERR(priv, "Start AGG when state is not IWL_AGG_OFF !
> ");

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Winkler, Tomas July 27, 2009, 8:28 a.m. UTC | #2

> -----Original Message-----
> From: Zhu, Yi
> Sent: Monday, July 27, 2009 4:42 AM
> To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
> Cc: [e-mail]; [e-mail];
> Andrew Morton
> Subject: Re: [PATCH] iwlwifi: Read outside array bounds
> 
> On Sun, 2009-07-26 at 05:34 +0800, Roel Kluin wrote:
> > tid is bounded (above) by the size of default_tid_to_tx_fifo (17
> elements), but
> > the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements.
> 
> I think MAX_TID_COUNT should be defined as 16 or 17. Tomas?
>

In general it's 16. In practice we use only 8.
Tomas

 
> Thanks,
> -yi
> 
> > Signed-off-by: Roel Kluin <[e-mail]>
> > ---
> > diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c
> b/drivers/net/wireless/iwlwifi/iwl-tx.c
> > index 85ae7a6..e9441c6 100644
> > --- a/drivers/net/wireless/iwlwifi/iwl-tx.c
> > +++ b/drivers/net/wireless/iwlwifi/iwl-tx.c
> > @@ -1170,6 +1170,8 @@ int iwl_tx_agg_start(struct iwl_priv *priv,
> const u8 *ra, u16 tid, u16 *ssn)
> >  		IWL_ERR(priv, "Start AGG on invalid station
> > ");
> >  		return -ENXIO;
> >  	}
> > +	if (unlikely(tid >= MAX_TID_COUNT))
> > +		return -EINVAL;
> >
> >  	if (priv->stations[sta_id].tid[tid].agg.state != IWL_AGG_OFF) {
> >  		IWL_ERR(priv, "Start AGG when state is not IWL_AGG_OFF !
> > ");

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reinette Chatre July 27, 2009, 10:12 p.m. UTC | #3

On Mon, 2009-07-27 at 01:28 -0700, Winkler, Tomas wrote:
> 
> > -----Original Message-----
> > From: Zhu, Yi
> > Sent: Monday, July 27, 2009 4:42 AM
> > To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
> > Cc: [e-mail]; [e-mail];
> > Andrew Morton
> > Subject: Re: [PATCH] iwlwifi: Read outside array bounds
> > 
> > On Sun, 2009-07-26 at 05:34 +0800, Roel Kluin wrote:
> > > tid is bounded (above) by the size of default_tid_to_tx_fifo (17
> > elements), but
> > > the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements.
> > 
> > I think MAX_TID_COUNT should be defined as 16 or 17. Tomas?
> >
> 
> In general it's 16. In practice we use only 8.

I think the above statement means that we are mostly using EDCA quality
of service which only uses 8 tids. We do not currently use HCCA (and
thus of course not the hybrid) which would cause more tids to be used.

A closer look at this flow to this function shows:
rs_tl_turn_on_agg 
->rs_tl_turn_on_agg_for_tid
-->ieee80211_start_tx_ba_session
--->iwl_mac_ampdu_action
---->iwl_tx_agg_start

>From what I can tell the tid is not modified from rs_tl_turn_on_agg to
iwl_tx_agg_start and rs_tl_turn_on_agg will not call further with a
value of tid larger than 7 due to its checking.

I thus do not see that tid may be equal or larger than MAX_TID_COUNT at
this point of checking. Even so, having this check will not do harm and
will increase safety.

This patch is already merged and that is ok, I just wanted to add this
information to it.

Reinette



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhu Yi July 28, 2009, 2:27 a.m. UTC | #4

On Tue, 2009-07-28 at 06:12 +0800, Chatre, Reinette wrote:
> On Mon, 2009-07-27 at 01:28 -0700, Winkler, Tomas wrote:
> > 
> > > -----Original Message-----
> > > From: Zhu, Yi
> > > Sent: Monday, July 27, 2009 4:42 AM
> > > To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
> > > Cc: [e-mail]; [e-mail];
> > > Andrew Morton
> > > Subject: Re: [PATCH] iwlwifi: Read outside array bounds
> > > 
> > > On Sun, 2009-07-26 at 05:34 +0800, Roel Kluin wrote:
> > > > tid is bounded (above) by the size of default_tid_to_tx_fifo (17
> > > elements), but
> > > > the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements.
> > > 
> > > I think MAX_TID_COUNT should be defined as 16 or 17. Tomas?
> > >
> > 
> > In general it's 16. In practice we use only 8.
> 
> I think the above statement means that we are mostly using EDCA quality
> of service which only uses 8 tids. We do not currently use HCCA (and
> thus of course not the hybrid) which would cause more tids to be used.
> 
> A closer look at this flow to this function shows:
> rs_tl_turn_on_agg 

rs_tl_add_packet
{
	...
	u8 *qc = ieee80211_get_qos_ctl(hdr);
	tid = qc[0] & 0xf;
	...

	tl = &lq_data->load[tid];
}

This should be a problem.

Thanks,
-yi

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reinette Chatre July 28, 2009, 4:50 a.m. UTC | #5

On Mon, 2009-07-27 at 19:27 -0700, Zhu, Yi wrote:
> On Tue, 2009-07-28 at 06:12 +0800, Chatre, Reinette wrote:
> > On Mon, 2009-07-27 at 01:28 -0700, Winkler, Tomas wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Zhu, Yi
> > > > Sent: Monday, July 27, 2009 4:42 AM
> > > > To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
> > > > Cc: [e-mail]; [e-mail];
> > > > Andrew Morton
> > > > Subject: Re: [PATCH] iwlwifi: Read outside array bounds
> > > > 
> > > > On Sun, 2009-07-26 at 05:34 +0800, Roel Kluin wrote:
> > > > > tid is bounded (above) by the size of default_tid_to_tx_fifo (17
> > > > elements), but
> > > > > the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements.
> > > > 
> > > > I think MAX_TID_COUNT should be defined as 16 or 17. Tomas?
> > > >
> > > 
> > > In general it's 16. In practice we use only 8.
> > 
> > I think the above statement means that we are mostly using EDCA quality
> > of service which only uses 8 tids. We do not currently use HCCA (and
> > thus of course not the hybrid) which would cause more tids to be used.
> > 
> > A closer look at this flow to this function shows:
> > rs_tl_turn_on_agg 
> 
> rs_tl_add_packet
> {
> 	...
> 	u8 *qc = ieee80211_get_qos_ctl(hdr);
> 	tid = qc[0] & 0xf;
> 	...
> 
> 	tl = &lq_data->load[tid];
> }
> 
> This should be a problem.

Indeed. Are there any other cases like this that you can think of? It
seems like we need Roel's fix for iwl_tx_agg_stop also.

Reinette


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhu Yi July 28, 2009, 5:17 a.m. UTC | #6

On Tue, 2009-07-28 at 12:50 +0800, Chatre, Reinette wrote:
> On Mon, 2009-07-27 at 19:27 -0700, Zhu, Yi wrote:
> > On Tue, 2009-07-28 at 06:12 +0800, Chatre, Reinette wrote:
> > > On Mon, 2009-07-27 at 01:28 -0700, Winkler, Tomas wrote:
> > > > 
> > > > > -----Original Message-----
> > > > > From: Zhu, Yi
> > > > > Sent: Monday, July 27, 2009 4:42 AM
> > > > > To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
> > > > > Cc: [e-mail]; [e-mail];
> > > > > Andrew Morton
> > > > > Subject: Re: [PATCH] iwlwifi: Read outside array bounds
> > > > > 
> > > > > On Sun, 2009-07-26 at 05:34 +0800, Roel Kluin wrote:
> > > > > > tid is bounded (above) by the size of default_tid_to_tx_fifo (17
> > > > > elements), but
> > > > > > the size of priv->stations[].tid[] is MAX_TID_COUNT (9) elements.
> > > > > 
> > > > > I think MAX_TID_COUNT should be defined as 16 or 17. Tomas?
> > > > >
> > > > 
> > > > In general it's 16. In practice we use only 8.
> > > 
> > > I think the above statement means that we are mostly using EDCA quality
> > > of service which only uses 8 tids. We do not currently use HCCA (and
> > > thus of course not the hybrid) which would cause more tids to be used.
> > > 
> > > A closer look at this flow to this function shows:
> > > rs_tl_turn_on_agg 
> > 
> > rs_tl_add_packet
> > {
> > 	...
> > 	u8 *qc = ieee80211_get_qos_ctl(hdr);
> > 	tid = qc[0] & 0xf;
> > 	...
> > 
> > 	tl = &lq_data->load[tid];
> > }
> > 
> > This should be a problem.
> 
> Indeed. Are there any other cases like this that you can think of? It
> seems like we need Roel's fix for iwl_tx_agg_stop also.

Yes. I think we can do an audit for all the ieee80211_ops callbacks with
tid as a parameter. Because mac80211 use u16 for tid, but iwlwifi use u8
internally.

Thanks,
-yi

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c
index 85ae7a6..e9441c6 100644
--- a/drivers/net/wireless/iwlwifi/iwl-tx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-tx.c
@@ -1170,6 +1170,8 @@  int iwl_tx_agg_start(struct iwl_priv *priv, const u8 *ra, u16 tid, u16 *ssn)
 		IWL_ERR(priv, "Start AGG on invalid station
");
 		return -ENXIO;
 	}
+	if (unlikely(tid >= MAX_TID_COUNT))
+		return -EINVAL;
 
 	if (priv->stations[sta_id].tid[tid].agg.state != IWL_AGG_OFF) {