[PATCH] iwlwifi: Read outside array bounds

[PATCH] iwlwifi: Read outside array bounds

[Roel Kluin]

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 <roel.kluin@gmail.com>
---
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 !
");

Re: [PATCH] iwlwifi: Read outside array bounds

[Zhu Yi]

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 <roel.kluin@gmail.com>
> ---
> 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 !
> ");

RE: [PATCH] iwlwifi: Read outside array bounds

[Winkler, Tomas]

> -----Original Message-----
> From: Zhu, Yi
> Sent: Monday, July 27, 2009 4:42 AM
> To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
> Cc: linux-wireless@vger.kernel.org; ipw3945-devel@lists.sourceforge.net;
> 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 <roel.kluin@gmail.com>
> > ---
> > 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.

RE: [PATCH] iwlwifi: Read outside array bounds

[reinette chatre]

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: linux-wireless@vger.kernel.org; ipw3945-devel@lists.sourceforge.net;
> > 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

RE: [PATCH] iwlwifi: Read outside array bounds

[Zhu Yi]

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: linux-wireless@vger.kernel.org; ipw3945-devel@lists.sourceforge.net;
> > > 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

RE: [PATCH] iwlwifi: Read outside array bounds

[reinette chatre]

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: linux-wireless@vger.kernel.org; ipw3945-devel@lists.sourceforge.net;
> > > > 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

RE: [PATCH] iwlwifi: Read outside array bounds

[Zhu Yi]

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: linux-wireless@vger.kernel.org; ipw3945-devel@lists.sourceforge.net;
> > > > > 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