[ath9k-devel] [PATCH 0/7] ath10k: htt rx fixes

[ath9k-devel] [PATCH 0/7] ath10k: htt rx fixes

[Michal Kazior]

This patchset addresses issues with fragmented
(802.11b) frames.

Michal Kazior (7):
  ath10k: zero htt_rx_info struct in frag rx handler
  ath10k: do not pop more than one frame in frag rx handler
  ath10k: add safety check after popping a frame in frag rx
  ath10k: move ath10k_htt_rx_free_msdu_chain()
  ath10k: drop all msdus in case of htt rx failure
  ath10k: fix tkip fragment rx
  ath10k: strip protected flag

 drivers/net/wireless/ath/ath10k/htt_rx.c |   54 ++++++++++++++++++------------
 drivers/net/wireless/ath/ath10k/txrx.c   |    7 +++-
 2 files changed, 38 insertions(+), 23 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/7] ath10k: zero htt_rx_info struct in frag rx handler

[Michal Kazior]

The struct was filled with garbage and ended up
having fcs_err set to true. This caused mac to
drop fragmented frames.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 055b88d..55c8083 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -877,7 +877,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	struct sk_buff *msdu_head, *msdu_tail;
 	struct htt_rx_desc *rxd;
 	enum rx_msdu_decap_format fmt;
-	struct htt_rx_info info;
+	struct htt_rx_info info = {};
 	int msdu_chaining;
 	bool tkip_mic_err;
 	bool decrypt_err;
-- 
1.7.9.5

[ath9k-devel] [PATCH 2/7] ath10k: do not pop more than one frame in frag rx handler

[Michal Kazior]

This led us to popping frames too early, before FW
actually filled in frames. This was not an issue
earlier. Perhaps the MSDU FW descriptor is now
different.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 55c8083..6d619e1 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -888,7 +888,6 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	fw_desc_len = __le16_to_cpu(frag->fw_rx_desc_bytes);
 	fw_desc = (u8 *)frag->fw_msdu_rx_desc;
 
-more:
 	msdu_head = NULL;
 	msdu_tail = NULL;
 	msdu_chaining = ath10k_htt_rx_amsdu_pop(htt, &fw_desc, &fw_desc_len,
@@ -966,8 +965,7 @@ more:
 end:
 	if (fw_desc_len > 0) {
 		ath10k_dbg(ATH10K_DBG_HTT,
-			   "expecting more fragmented rx in one indication\n");
-		goto more;
+			   "expecting more fragmented rx in one indication %d\n", fw_desc_len);
 	}
 }
 
-- 
1.7.9.5

[ath9k-devel] [PATCH 3/7] ath10k: add safety check after popping a frame in frag rx

[Michal Kazior]

If HTT RX went haywire we dereferenced buggy
pointers that led to system crash.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6d619e1..fe2d27b 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -895,6 +895,11 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 
 	ath10k_dbg(ATH10K_DBG_HTT_DUMP, "htt rx frag ahead\n");
 
+	if (!msdu_head) {
+		ath10k_warn("htt rx frag no data\n");
+		return;
+	}
+
 	if (msdu_chaining || msdu_head != msdu_tail) {
 		ath10k_warn("aggregation with fragmentation?!\n");
 		ath10k_htt_rx_free_msdu_chain(msdu_head);
-- 
1.7.9.5

[ath9k-devel] [PATCH 4/7] ath10k: move ath10k_htt_rx_free_msdu_chain()

[Michal Kazior]

This makes it possible to call the function more
different places now.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index fe2d27b..5afe42a 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -235,6 +235,17 @@ static inline struct sk_buff *ath10k_htt_rx_netbuf_pop(struct ath10k_htt *htt)
 	return msdu;
 }
 
+static void ath10k_htt_rx_free_msdu_chain(struct sk_buff *skb)
+{
+	struct sk_buff *next;
+
+	while (skb) {
+		next = skb->next;
+		dev_kfree_skb_any(skb);
+		skb = next;
+	}
+}
+
 static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 				   u8 **fw_desc, int *fw_desc_len,
 				   struct sk_buff **head_msdu,
@@ -545,17 +556,6 @@ static bool ath10k_htt_rx_hdr_is_amsdu(struct ieee80211_hdr *hdr)
 	return false;
 }
 
-static void ath10k_htt_rx_free_msdu_chain(struct sk_buff *skb)
-{
-	struct sk_buff *next;
-
-	while (skb) {
-		next = skb->next;
-		dev_kfree_skb_any(skb);
-		skb = next;
-	}
-}
-
 static int ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 			struct htt_rx_info *info)
 {
-- 
1.7.9.5

[ath9k-devel] [PATCH 5/7] ath10k: drop all msdus in case of htt rx failure

[Michal Kazior]

It's a lot safer to simply drop everything. It's
broken anyway so why bother.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 5afe42a..37784f6 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -292,9 +292,9 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 		 */
 		if (!(__le32_to_cpu(rx_desc->attention.flags)
 				& RX_ATTENTION_FLAGS_MSDU_DONE)) {
-			if (*head_msdu == msdu)
-				*head_msdu = NULL;
-			dev_kfree_skb_any(msdu);
+			ath10k_htt_rx_free_msdu_chain(*head_msdu);
+			*head_msdu = NULL;
+			msdu = NULL;
 			ath10k_err("htt rx stopped. cannot recover\n");
 			htt->rx_confused = true;
 			break;
-- 
1.7.9.5

[ath9k-devel] [PATCH 6/7] ath10k: fix tkip fragment rx

[Michal Kazior]

TKIP fragmented frames contains different data at
the tail depending on the more flags.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 37784f6..0b20879 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -518,9 +518,8 @@ static int ath10k_htt_rx_crypto_tail_len(enum htt_rx_mpdu_encrypt_type type)
 	case HTT_RX_MPDU_ENCRYPT_WAPI:
 		return 0;
 	case HTT_RX_MPDU_ENCRYPT_TKIP_WITHOUT_MIC:
-		return 4;
 	case HTT_RX_MPDU_ENCRYPT_TKIP_WPA:
-		return 4 + 8;
+		return 4;
 	case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
 		return 8;
 	}
@@ -878,6 +877,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	struct htt_rx_desc *rxd;
 	enum rx_msdu_decap_format fmt;
 	struct htt_rx_info info = {};
+	struct ieee80211_hdr *hdr;
 	int msdu_chaining;
 	bool tkip_mic_err;
 	bool decrypt_err;
@@ -908,6 +908,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 
 	/* FIXME: implement signal strength */
 
+	hdr = (struct ieee80211_hdr *)msdu_head->data;
 	rxd = (void *)msdu_head->data - sizeof(*rxd);
 	tkip_mic_err = !!(__le32_to_cpu(rxd->attention.flags) &
 				RX_ATTENTION_FLAGS_TKIP_MIC_ERR);
@@ -939,7 +940,6 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	}
 
 	if (info.encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE) {
-		struct ieee80211_hdr *hdr = (void *)info.skb->data;
 		int hdrlen = ieee80211_hdrlen(hdr->frame_control);
 		int paramlen = ath10k_htt_rx_crypto_param_len(info.encrypt_type);
 
@@ -948,13 +948,20 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 			(void *)info.skb->data,
 			hdrlen);
 		skb_pull(info.skb, paramlen);
+		hdr = (struct ieee80211_hdr *)info.skb->data;
 	}
 
 	/* remove trailing FCS */
 	trim  = 4;
-	/* remove crypto trailer; we use rx desc for mic failure */
+
+	/* remove crypto trailer */
 	trim += ath10k_htt_rx_crypto_tail_len(info.encrypt_type);
 
+	/* last fragment of TKIP frags has MIC */
+	if (!ieee80211_has_morefrags(hdr->frame_control) &&
+	    info.encrypt_type == HTT_RX_MPDU_ENCRYPT_TKIP_WPA)
+		trim += 8;
+
 	if (trim > info.skb->len) {
 		ath10k_warn("htt rx fragment: trailer longer than the frame itself? drop\n");
 		dev_kfree_skb_any(info.skb);
-- 
1.7.9.5

[ath9k-devel] [PATCH 7/7] ath10k: strip protected flag

[Michal Kazior]

We already do decryption and all crypto
verification in HW. mac80211 tries to verify CCMP
for fragmented rx and fails without this patch.

This fixes fragmented rx on CCMP networks.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/txrx.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index ab5eb3b..a54ad8f 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -238,13 +238,18 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
 {
 	struct ieee80211_rx_status *status;
 	struct ieee80211_channel *ch;
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)info->skb->data;
 
 	status = IEEE80211_SKB_RXCB(info->skb);
 	memset(status, 0, sizeof(*status));
 
-	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE)
+	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE) {
 		status->flag |= RX_FLAG_DECRYPTED | RX_FLAG_IV_STRIPPED |
 				RX_FLAG_MMIC_STRIPPED;
+		hdr->frame_control = __cpu_to_le16(
+				__le16_to_cpu(hdr->frame_control) &
+				~IEEE80211_FCTL_PROTECTED);
+	}
 
 	if (info->status == HTT_RX_IND_MPDU_STATUS_TKIP_MIC_ERR)
 		status->flag |= RX_FLAG_MMIC_ERROR;
-- 
1.7.9.5

[ath9k-devel] [PATCH 7/7] ath10k: strip protected flag

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> We already do decryption and all crypto
> verification in HW. mac80211 tries to verify CCMP
> for fragmented rx and fails without this patch.
>
> This fixes fragmented rx on CCMP networks.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> -	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE)
> +	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE) {
>  		status->flag |= RX_FLAG_DECRYPTED | RX_FLAG_IV_STRIPPED |
>  				RX_FLAG_MMIC_STRIPPED;
> +		hdr->frame_control = __cpu_to_le16(
> +				__le16_to_cpu(hdr->frame_control) &
> +				~IEEE80211_FCTL_PROTECTED);
> +	}

This looks ugly. Is this a bug in mac80211 which we just workaround in
ath10k? Isn't there any other way to fix this? At least this deserves a
big comment in the code to explain why we are doing this.

I'll apply this anyway, but we need to discuss more about this.

-- 
Kalle Valo

[ath9k-devel] [PATCH 0/7] ath10k: htt rx fixes

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This patchset addresses issues with fragmented
> (802.11b) frames.
>
> Michal Kazior (7):
>   ath10k: zero htt_rx_info struct in frag rx handler
>   ath10k: do not pop more than one frame in frag rx handler
>   ath10k: add safety check after popping a frame in frag rx
>   ath10k: move ath10k_htt_rx_free_msdu_chain()
>   ath10k: drop all msdus in case of htt rx failure
>   ath10k: fix tkip fragment rx
>   ath10k: strip protected flag

Thanks, all seven applied.

-- 
Kalle Valo

[ath9k-devel] [PATCH 7/7] ath10k: strip protected flag

[Michal Kazior]

On 30/04/13 09:58, Kalle Valo wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> We already do decryption and all crypto
>> verification in HW. mac80211 tries to verify CCMP
>> for fragmented rx and fails without this patch.
>>
>> This fixes fragmented rx on CCMP networks.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> -	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE)
>> +	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE) {
>>   		status->flag |= RX_FLAG_DECRYPTED | RX_FLAG_IV_STRIPPED |
>>   				RX_FLAG_MMIC_STRIPPED;
>> +		hdr->frame_control = __cpu_to_le16(
>> +				__le16_to_cpu(hdr->frame_control) &
>> +				~IEEE80211_FCTL_PROTECTED);
>> +	}
>
> This looks ugly. Is this a bug in mac80211 which we just workaround in
> ath10k? Isn't there any other way to fix this? At least this deserves a
> big comment in the code to explain why we are doing this.
>
> I'll apply this anyway, but we need to discuss more about this.

I'm not quite sure whether this is an issue in mac80211 or we should be 
actually stripping the protected flag. b43 driver seems to strip the 
flag as well.

mac80211 seems to want to do some extra checks for CCMP in fragmented rx 
data path. Perhaps it shouldn't?


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [PATCH 7/7] ath10k: strip protected flag

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 30/04/13 09:58, Kalle Valo wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> We already do decryption and all crypto
>>> verification in HW. mac80211 tries to verify CCMP
>>> for fragmented rx and fails without this patch.
>>>
>>> This fixes fragmented rx on CCMP networks.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> [...]
>>
>>> -	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE)
>>> +	if (info->encrypt_type != HTT_RX_MPDU_ENCRYPT_NONE) {
>>>   		status->flag |= RX_FLAG_DECRYPTED | RX_FLAG_IV_STRIPPED |
>>>   				RX_FLAG_MMIC_STRIPPED;
>>> +		hdr->frame_control = __cpu_to_le16(
>>> +				__le16_to_cpu(hdr->frame_control) &
>>> +				~IEEE80211_FCTL_PROTECTED);
>>> +	}
>>
>> This looks ugly. Is this a bug in mac80211 which we just workaround in
>> ath10k? Isn't there any other way to fix this? At least this deserves a
>> big comment in the code to explain why we are doing this.
>>
>> I'll apply this anyway, but we need to discuss more about this.
>
> I'm not quite sure whether this is an issue in mac80211 or we should
> be actually stripping the protected flag. b43 driver seems to strip
> the flag as well.

If b43 does the same when I'm happy with this. Just send a new patch
adding a comment to the code as well.

> mac80211 seems to want to do some extra checks for CCMP in fragmented
> rx data path. Perhaps it shouldn't?

IMHO it shouldn't, but I'm no security expert. Maybe you should ask in
the linux-wireless list?

-- 
Kalle Valo