iwlwifi: Read outside array bounds

PATCH In review Roel Kluin 4 participants 7 messages Jul 25, 2009 – Jul 28, 2009 Networking drivers (wireless)

Share & tools

Roel Kluin

author

2009-07-25 21:31 UTC #0 lore raw

PATCH 1/1 iwlwifi: Read outside array bounds

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 <[email protected]>

---

drivers/net/wireless/iwlwifi/iwl-tx.c +0 -0

index 85ae7a6..e9441c6 100644
@@ -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 !
");

Zhu Yi

2009-07-27 01:41 UTC #1 lore raw

Quoted 2 lines · 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.

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

Thanks,
-yi

Quoted 17 lines

Signed-off-by Roel Kluin <[email protected]>

---

drivers/net/wireless/iwlwifi/iwl-tx.c +2 -0

index 85ae7a6..e9441c6 100644
@@ -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 !
");

Winkler, Tomas

2009-07-27 08:29 UTC #2 lore raw

Quoted 10 lines

-----Original Message-----
From: Zhu, Yi
Sent: Monday, July 27, 2009 4:42 AM
To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
Cc: [email protected]; [email protected]; Andrew Morton
Subject: Re: [PATCH] iwlwifi: Read outside array bounds

Quoted 7 lines

Quoted 1 line · Roel Kluin
tid is bounded (above) by the size of default_tid_to_tx_fifo (17
elements), but
Quoted 1 line
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

Quoted 19 lines

Thanks,
-yi
Review
Quoted 3 lines
Signed-off-by Roel Kluin <[email protected]>
---
(quoted diff) +0 -0
b/drivers/net/wireless/iwlwifi/iwl-tx.c
drivers/net/wireless/iwlwifi/iwl-tx.c int iwl_tx_agg_start(struct iwl_priv *priv,
Quoted 4 lines
index 85ae7a6..e9441c6 100644
drivers/net/wireless/iwlwifi/iwl-tx.c +0 -0
@@ -1170,6 +1170,8 @@ int iwl_tx_agg_start(struct iwl_priv *priv,
const u8 *ra, u16 tid, u16 *ssn)
Quoted 7 lines
drivers/net/wireless/iwlwifi/iwl-tx.c +2 -0
 		IWL_ERR(priv, "Start AGG on invalid station
 ");
 		return -ENXIO;
 	}
+	if (unlikely(tid >= MAX_TID_COUNT))
+		return -EINVAL;
 

Quoted 3 lines

Quoted 3 lines
drivers/net/wireless/iwlwifi/iwl-tx.c +0 -0
 	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.

reinette chatre

2009-07-27 22:12 UTC #3 lore raw

Quoted 10 lines · Tomas

Quoted 10 lines
-----Original Message-----
From: Zhu, Yi
Sent: Monday, July 27, 2009 4:42 AM
To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
Cc: [email protected]; [email protected]; Andrew Morton
Subject: Re: [PATCH] iwlwifi: Read outside array bounds
On Sun, 2009-07-26 at 05:34 +0800, Roel Kluin wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
tid is bounded (above) by the size of default_tid_to_tx_fifo (17
elements), but
Quoted 1 line
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

Quoted 1 line

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

Zhu Yi

2009-07-28 02:27 UTC #4 lore raw

Quoted 18 lines · Reinette

Quoted 17 lines · Tomas
Quoted 16 lines
-----Original Message-----
From: Zhu, Yi
Sent: Monday, July 27, 2009 4:42 AM
To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
Cc: [email protected]; [email protected]; Andrew Morton
Subject: Re: [PATCH] iwlwifi: Read outside array bounds
Quoted 1 line · Roel Kluin
tid is bounded (above) by the size of default_tid_to_tx_fifo (17
elements), but
Quoted 1 line
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?

Quoted 8 lines

Quoted 1 line
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

reinette chatre

2009-07-28 04:50 UTC #5 lore raw

Quoted 38 lines · Yi

Quoted 18 lines · Reinette
Quoted 17 lines · Tomas
Quoted 16 lines
-----Original Message-----
From: Zhu, Yi
Sent: Monday, July 27, 2009 4:42 AM
To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
Cc: [email protected]; [email protected]; Andrew Morton
Subject: Re: [PATCH] iwlwifi: Read outside array bounds
Quoted 1 line · Roel Kluin
tid is bounded (above) by the size of default_tid_to_tx_fifo (17
elements), but
Quoted 1 line
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?
Quoted 8 lines
Quoted 1 line
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];
}

Quoted 1 line

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

Zhu Yi

2009-07-28 05:17 UTC #6 lore raw

Quoted 36 lines · Reinette

Quoted 35 lines · Yi
Quoted 18 lines · Reinette
Quoted 17 lines · Tomas
Quoted 16 lines
-----Original Message-----
From: Zhu, Yi
Sent: Monday, July 27, 2009 4:42 AM
To: Roel Kluin; Winkler, Tomas; Chatre, Reinette
Cc: [email protected]; [email protected]; Andrew Morton
Subject: Re: [PATCH] iwlwifi: Read outside array bounds
Quoted 1 line · Roel Kluin
tid is bounded (above) by the size of default_tid_to_tx_fifo (17
elements), but
Quoted 1 line
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?
Quoted 8 lines
Quoted 1 line
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;
	...

Quoted 7 lines

Quoted 4 lines
	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