[BUG] ath9k truncated management packets from TKIP connected stations

[BUG] ath9k truncated management packets from TKIP connected stations

[Bill Jordan]

I'm not quite sure what the correct fix is for this.

Ath9k in AP mode with a TKIP security: If a connected station sends a
management packet, the packet is truncated by 8 bytes before being
delivered to hostapd. This prevents the station from reauthenticating
or connecting to a different SSID on the same radio.

In ath9k_rx_accept, for management packets, strip_mic will be true,
and RX_FLAG_MMIC_STRIPPED will be set in rxs->flag. In
ath9k_rx_skb_postprocess, if ah->sw_mgmt_crypto is set,
RX_FLAG_DECRYPTED will be cleared. However, RX_FLAG_MMIC_STRIPPED will
still be set, so, in ath_rx_tasklet, 8 bytes will be trimmed off the
end of the skb.

I'm thinking that in ath9k_rx_accept, is_valid_tkip  should also
consider ieee80211_is_mgmt(fc). But this wouldn't take into
consideration ah->sw_mgmt_crypto.

Alternatively, RX_FLAG_MMIC_STRIPPED could be cleared in
ath9k_rx_skb_postprocess when RX_FLAG_DECRYPTED is cleared.

I'm looking for input from someone who understands this code better.

Thanks,
Bill Jordan

[ath9k-devel] [BUG] ath9k truncated management packets from TKIP connected stations

[Bill Jordan]

I'm not quite sure what the correct fix is for this.

Ath9k in AP mode with a TKIP security: If a connected station sends a
management packet, the packet is truncated by 8 bytes before being
delivered to hostapd. This prevents the station from reauthenticating
or connecting to a different SSID on the same radio.

In ath9k_rx_accept, for management packets, strip_mic will be true,
and RX_FLAG_MMIC_STRIPPED will be set in rxs->flag. In
ath9k_rx_skb_postprocess, if ah->sw_mgmt_crypto is set,
RX_FLAG_DECRYPTED will be cleared. However, RX_FLAG_MMIC_STRIPPED will
still be set, so, in ath_rx_tasklet, 8 bytes will be trimmed off the
end of the skb.

I'm thinking that in ath9k_rx_accept, is_valid_tkip ?should also
consider ieee80211_is_mgmt(fc). But this wouldn't take into
consideration ah->sw_mgmt_crypto.

Alternatively, RX_FLAG_MMIC_STRIPPED could be cleared in
ath9k_rx_skb_postprocess when RX_FLAG_DECRYPTED is cleared.

I'm looking for input from someone who understands this code better.

Thanks,
Bill Jordan

Re: [BUG] ath9k truncated management packets from TKIP connected stations

[Felix Fietkau]

On 2011-08-16 2:31 PM, Bill Jordan wrote:
> I'm not quite sure what the correct fix is for this.
>
> Ath9k in AP mode with a TKIP security: If a connected station sends a
> management packet, the packet is truncated by 8 bytes before being
> delivered to hostapd. This prevents the station from reauthenticating
> or connecting to a different SSID on the same radio.
>
> In ath9k_rx_accept, for management packets, strip_mic will be true,
> and RX_FLAG_MMIC_STRIPPED will be set in rxs->flag. In
> ath9k_rx_skb_postprocess, if ah->sw_mgmt_crypto is set,
> RX_FLAG_DECRYPTED will be cleared. However, RX_FLAG_MMIC_STRIPPED will
> still be set, so, in ath_rx_tasklet, 8 bytes will be trimmed off the
> end of the skb.
>
> I'm thinking that in ath9k_rx_accept, is_valid_tkip  should also
> consider ieee80211_is_mgmt(fc). But this wouldn't take into
> consideration ah->sw_mgmt_crypto.
>
> Alternatively, RX_FLAG_MMIC_STRIPPED could be cleared in
> ath9k_rx_skb_postprocess when RX_FLAG_DECRYPTED is cleared.
>
> I'm looking for input from someone who understands this code better.
We should probably just keep strip_mic set to false for mgmt frames.

- Felix

[ath9k-devel] [BUG] ath9k truncated management packets from TKIP connected stations

[Felix Fietkau]

On 2011-08-16 2:31 PM, Bill Jordan wrote:
> I'm not quite sure what the correct fix is for this.
>
> Ath9k in AP mode with a TKIP security: If a connected station sends a
> management packet, the packet is truncated by 8 bytes before being
> delivered to hostapd. This prevents the station from reauthenticating
> or connecting to a different SSID on the same radio.
>
> In ath9k_rx_accept, for management packets, strip_mic will be true,
> and RX_FLAG_MMIC_STRIPPED will be set in rxs->flag. In
> ath9k_rx_skb_postprocess, if ah->sw_mgmt_crypto is set,
> RX_FLAG_DECRYPTED will be cleared. However, RX_FLAG_MMIC_STRIPPED will
> still be set, so, in ath_rx_tasklet, 8 bytes will be trimmed off the
> end of the skb.
>
> I'm thinking that in ath9k_rx_accept, is_valid_tkip  should also
> consider ieee80211_is_mgmt(fc). But this wouldn't take into
> consideration ah->sw_mgmt_crypto.
>
> Alternatively, RX_FLAG_MMIC_STRIPPED could be cleared in
> ath9k_rx_skb_postprocess when RX_FLAG_DECRYPTED is cleared.
>
> I'm looking for input from someone who understands this code better.
We should probably just keep strip_mic set to false for mgmt frames.

- Felix

[PATCH] ath9k: fix MGMT packets when using TKIP

[Bill Jordan]

Prevent 8 bytes from being truncated from MGMT packets
when using TKIP.

Signed-off-by: Bill Jordan <bjordan@rajant.com>
---
 drivers/net/wireless/ath/ath9k/recv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 7409402..d8737f2 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -824,7 +824,8 @@ static bool ath9k_rx_accept(struct ath_common *common,
 	is_mc = !!is_multicast_ether_addr(hdr->addr1);
 	is_valid_tkip = rx_stats->rs_keyix != ATH9K_RXKEYIX_INVALID &&
 		test_bit(rx_stats->rs_keyix, common->tkip_keymap);
-	strip_mic = is_valid_tkip && !(rx_stats->rs_status &
+	strip_mic = is_valid_tkip && !ieee80211_is_mgmt(fc) &&
+		!(rx_stats->rs_status &
 		(ATH9K_RXERR_DECRYPT | ATH9K_RXERR_CRC | ATH9K_RXERR_MIC));
 
 	if (!rx_stats->rs_datalen)
-- 
1.7.6

[ath9k-devel] [PATCH] ath9k: fix MGMT packets when using TKIP

[Bill Jordan]

Prevent 8 bytes from being truncated from MGMT packets
when using TKIP.

Signed-off-by: Bill Jordan <bjordan@rajant.com>
---
 drivers/net/wireless/ath/ath9k/recv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 7409402..d8737f2 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -824,7 +824,8 @@ static bool ath9k_rx_accept(struct ath_common *common,
 	is_mc = !!is_multicast_ether_addr(hdr->addr1);
 	is_valid_tkip = rx_stats->rs_keyix != ATH9K_RXKEYIX_INVALID &&
 		test_bit(rx_stats->rs_keyix, common->tkip_keymap);
-	strip_mic = is_valid_tkip && !(rx_stats->rs_status &
+	strip_mic = is_valid_tkip && !ieee80211_is_mgmt(fc) &&
+		!(rx_stats->rs_status &
 		(ATH9K_RXERR_DECRYPT | ATH9K_RXERR_CRC | ATH9K_RXERR_MIC));
 
 	if (!rx_stats->rs_datalen)
-- 
1.7.6

Re: [PATCH] ath9k: fix MGMT packets when using TKIP

[Felix Fietkau]

On 2011-08-17 1:06 PM, Bill Jordan wrote:
> Prevent 8 bytes from being truncated from MGMT packets
> when using TKIP.
>
> Signed-off-by: Bill Jordan<bjordan@rajant.com>
> ---
>   drivers/net/wireless/ath/ath9k/recv.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index 7409402..d8737f2 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -824,7 +824,8 @@ static bool ath9k_rx_accept(struct ath_common *common,
>   	is_mc = !!is_multicast_ether_addr(hdr->addr1);
>   	is_valid_tkip = rx_stats->rs_keyix != ATH9K_RXKEYIX_INVALID&&
>   		test_bit(rx_stats->rs_keyix, common->tkip_keymap);
> -	strip_mic = is_valid_tkip&&  !(rx_stats->rs_status&
> +	strip_mic = is_valid_tkip&&  !ieee80211_is_mgmt(fc)&&
> +		!(rx_stats->rs_status&
>   		(ATH9K_RXERR_DECRYPT | ATH9K_RXERR_CRC | ATH9K_RXERR_MIC));
>
>   	if (!rx_stats->rs_datalen)
Maybe it would be a better idea to check for ieee80211_is_data(fc) 
instead of !ieee80211_is_mgmt(fc) - just to avoid more potential corner 
cases.

- Felix

[ath9k-devel] [PATCH] ath9k: fix MGMT packets when using TKIP

[Felix Fietkau]

On 2011-08-17 1:06 PM, Bill Jordan wrote:
> Prevent 8 bytes from being truncated from MGMT packets
> when using TKIP.
>
> Signed-off-by: Bill Jordan<bjordan@rajant.com>
> ---
>   drivers/net/wireless/ath/ath9k/recv.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index 7409402..d8737f2 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -824,7 +824,8 @@ static bool ath9k_rx_accept(struct ath_common *common,
>   	is_mc = !!is_multicast_ether_addr(hdr->addr1);
>   	is_valid_tkip = rx_stats->rs_keyix != ATH9K_RXKEYIX_INVALID&&
>   		test_bit(rx_stats->rs_keyix, common->tkip_keymap);
> -	strip_mic = is_valid_tkip&&  !(rx_stats->rs_status&
> +	strip_mic = is_valid_tkip&&  !ieee80211_is_mgmt(fc)&&
> +		!(rx_stats->rs_status&
>   		(ATH9K_RXERR_DECRYPT | ATH9K_RXERR_CRC | ATH9K_RXERR_MIC));
>
>   	if (!rx_stats->rs_datalen)
Maybe it would be a better idea to check for ieee80211_is_data(fc) 
instead of !ieee80211_is_mgmt(fc) - just to avoid more potential corner 
cases.

- Felix

[PATCH] ath9k: fix MGMT packets when using TKIP

[Bill Jordan]

Prevent 8 bytes from being truncated from MGMT packets
when using TKIP.

Signed-off-by: Bill Jordan <bjordan@rajant.com>
---
 drivers/net/wireless/ath/ath9k/recv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 7409402..9adddd9 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -824,7 +824,8 @@ static bool ath9k_rx_accept(struct ath_common *common,
 	is_mc = !!is_multicast_ether_addr(hdr->addr1);
 	is_valid_tkip = rx_stats->rs_keyix != ATH9K_RXKEYIX_INVALID &&
 		test_bit(rx_stats->rs_keyix, common->tkip_keymap);
-	strip_mic = is_valid_tkip && !(rx_stats->rs_status &
+	strip_mic = is_valid_tkip && ieee80211_is_data(fc) &&
+		!(rx_stats->rs_status &
 		(ATH9K_RXERR_DECRYPT | ATH9K_RXERR_CRC | ATH9K_RXERR_MIC));
 
 	if (!rx_stats->rs_datalen)
-- 
1.7.6

[ath9k-devel] [PATCH] ath9k: fix MGMT packets when using TKIP

[Bill Jordan]

Prevent 8 bytes from being truncated from MGMT packets
when using TKIP.

Signed-off-by: Bill Jordan <bjordan@rajant.com>
---
 drivers/net/wireless/ath/ath9k/recv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 7409402..9adddd9 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -824,7 +824,8 @@ static bool ath9k_rx_accept(struct ath_common *common,
 	is_mc = !!is_multicast_ether_addr(hdr->addr1);
 	is_valid_tkip = rx_stats->rs_keyix != ATH9K_RXKEYIX_INVALID &&
 		test_bit(rx_stats->rs_keyix, common->tkip_keymap);
-	strip_mic = is_valid_tkip && !(rx_stats->rs_status &
+	strip_mic = is_valid_tkip && ieee80211_is_data(fc) &&
+		!(rx_stats->rs_status &
 		(ATH9K_RXERR_DECRYPT | ATH9K_RXERR_CRC | ATH9K_RXERR_MIC));
 
 	if (!rx_stats->rs_datalen)
-- 
1.7.6

Re: [PATCH] ath9k: fix MGMT packets when using TKIP

[Kalle Valo]

Bill Jordan <bjordan@rajant.com> writes:

> Prevent 8 bytes from being truncated from MGMT packets
> when using TKIP.

A bit more information in the commit log would be nice. Is this is a
regression or an old bug? Maybe a stable candidate?

-- 
Kalle Valo

[ath9k-devel] [PATCH] ath9k: fix MGMT packets when using TKIP

[Kalle Valo]

Bill Jordan <bjordan@rajant.com> writes:

> Prevent 8 bytes from being truncated from MGMT packets
> when using TKIP.

A bit more information in the commit log would be nice. Is this is a
regression or an old bug? Maybe a stable candidate?

-- 
Kalle Valo

Re: [PATCH] ath9k: fix MGMT packets when using TKIP

[Bill Jordan]

On Mon, Aug 22, 2011 at 4:11 AM, Kalle Valo <kvalo@adurom.com> wrote:
> Bill Jordan <bjordan@rajant.com> writes:
>
>> Prevent 8 bytes from being truncated from MGMT packets
>> when using TKIP.
>
> A bit more information in the commit log would be nice. Is this is a
> regression or an old bug? Maybe a stable candidate?

I believe it is an old bug.

Bill Jordan

[ath9k-devel] [PATCH] ath9k: fix MGMT packets when using TKIP

[Bill Jordan]

On Mon, Aug 22, 2011 at 4:11 AM, Kalle Valo <kvalo@adurom.com> wrote:
> Bill Jordan <bjordan@rajant.com> writes:
>
>> Prevent 8 bytes from being truncated from MGMT packets
>> when using TKIP.
>
> A bit more information in the commit log would be nice. Is this is a
> regression or an old bug? Maybe a stable candidate?

I believe it is an old bug.

Bill Jordan

Re: [PATCH] ath9k: fix MGMT packets when using TKIP

[Christian Lamparter]

On Tuesday, August 23, 2011 11:59:51 PM Bill Jordan wrote:
> On Mon, Aug 22, 2011 at 4:11 AM, Kalle Valo <kvalo@adurom.com> wrote:
> > Bill Jordan <bjordan@rajant.com> writes:
> >
> >> Prevent 8 bytes from being truncated from MGMT packets
> >> when using TKIP.
> >
> > A bit more information in the commit log would be nice. Is this is a
> > regression or an old bug? Maybe a stable candidate?
> 
> I believe it is an old bug.
No it's not. It was introduced by:

commit 66760eac005d569393bac34136bcbb8af55d8a5a
Author: Felix Fietkau <nbd@openwrt.org>
Date:   Wed Jul 13 23:35:05 2011 +0800

    ath9k: improve reliability of MIC error detection

[ath9k-devel] [PATCH] ath9k: fix MGMT packets when using TKIP

[Christian Lamparter]

On Tuesday, August 23, 2011 11:59:51 PM Bill Jordan wrote:
> On Mon, Aug 22, 2011 at 4:11 AM, Kalle Valo <kvalo@adurom.com> wrote:
> > Bill Jordan <bjordan@rajant.com> writes:
> >
> >> Prevent 8 bytes from being truncated from MGMT packets
> >> when using TKIP.
> >
> > A bit more information in the commit log would be nice. Is this is a
> > regression or an old bug? Maybe a stable candidate?
> 
> I believe it is an old bug.
No it's not. It was introduced by:

commit 66760eac005d569393bac34136bcbb8af55d8a5a
Author: Felix Fietkau <nbd@openwrt.org>
Date:   Wed Jul 13 23:35:05 2011 +0800

    ath9k: improve reliability of MIC error detection

[ath9k-devel] [PATCH] ath9k: fix MGMT packets when using TKIP

[Senthilkumar Balasubramanian]

On Wed, Aug 24, 2011 at 3:44 AM, Christian Lamparter <
chunkeey@googlemail.com> wrote:

> On Tuesday, August 23, 2011 11:59:51 PM Bill Jordan wrote:
> > On Mon, Aug 22, 2011 at 4:11 AM, Kalle Valo <kvalo@adurom.com> wrote:
> > > Bill Jordan <bjordan@rajant.com> writes:
> > >
> > >> Prevent 8 bytes from being truncated from MGMT packets
> > >> when using TKIP.
> > >
> > > A bit more information in the commit log would be nice. Is this is a
> > > regression or an old bug? Maybe a stable candidate?
> >
> > I believe it is an old bug.
> No it's not. It was introduced by:
>
> Yes it is. Stripping mic was recently added.


> commit 66760eac005d569393bac34136bcbb8af55d8a5a
> Author: Felix Fietkau <nbd@openwrt.org>
> Date:   Wed Jul 13 23:35:05 2011 +0800
>
>    ath9k: improve reliability of MIC error detection
>
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20110824/ad8e19cf/attachment.htm