2010-04-23 07:26:35

by Vivek Natarajan

[permalink] [raw]
Subject: [PATCH] ath9k: Avoid corrupt frames being forwarded to mac80211.

If bit 29 is set, MAC H/W can attempt to decrypt the received aggregate
with WEP or TKIP, eventhough the received frame may be a CRC failed
corrupted frame.

Cc: [email protected]
Reported-by: Johan Hovold <[email protected]>
Signed-off-by: Vivek Natarajan <[email protected]>
Signed-off-by: Ranga Rao Ravuri <[email protected]>
---
drivers/net/wireless/ath/ath9k/ar5008_initvals.h | 2 +-
drivers/net/wireless/ath/ath9k/ar9002_initvals.h | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
index cd953f6..025c31a 100644
--- a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
+++ b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
@@ -249,7 +249,7 @@ static const u32 ar5416Common[][2] = {
{ 0x00008258, 0x00000000 },
{ 0x0000825c, 0x400000ff },
{ 0x00008260, 0x00080922 },
- { 0x00008264, 0xa8000010 },
+ { 0x00008264, 0x88000010 },
{ 0x00008270, 0x00000000 },
{ 0x00008274, 0x40000000 },
{ 0x00008278, 0x003e4180 },
diff --git a/drivers/net/wireless/ath/ath9k/ar9002_initvals.h b/drivers/net/wireless/ath/ath9k/ar9002_initvals.h
index f06313d..dae7f33 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_initvals.h
+++ b/drivers/net/wireless/ath/ath9k/ar9002_initvals.h
@@ -793,7 +793,7 @@ static const u32 ar9280Common_9280_2[][2] = {
{ 0x00008258, 0x00000000 },
{ 0x0000825c, 0x400000ff },
{ 0x00008260, 0x00080922 },
- { 0x00008264, 0xa8a00010 },
+ { 0x00008264, 0x88a00010 },
{ 0x00008270, 0x00000000 },
{ 0x00008274, 0x40000000 },
{ 0x00008278, 0x003e4180 },
@@ -1963,7 +1963,7 @@ static const u32 ar9285Common_9285[][2] = {
{ 0x00008258, 0x00000000 },
{ 0x0000825c, 0x400000ff },
{ 0x00008260, 0x00080922 },
- { 0x00008264, 0xa8a00010 },
+ { 0x00008264, 0x88a00010 },
{ 0x00008270, 0x00000000 },
{ 0x00008274, 0x40000000 },
{ 0x00008278, 0x003e4180 },
@@ -3185,7 +3185,7 @@ static const u32 ar9287Common_9287_1_0[][2] = {
{ 0x00008258, 0x00000000 },
{ 0x0000825c, 0x400000ff },
{ 0x00008260, 0x00080922 },
- { 0x00008264, 0xa8a00010 },
+ { 0x00008264, 0x88a00010 },
{ 0x00008270, 0x00000000 },
{ 0x00008274, 0x40000000 },
{ 0x00008278, 0x003e4180 },
@@ -4973,7 +4973,7 @@ static const u32 ar9271Common_9271[][2] = {
{ 0x00008258, 0x00000000 },
{ 0x0000825c, 0x400000ff },
{ 0x00008260, 0x00080922 },
- { 0x00008264, 0xa8a00010 },
+ { 0x00008264, 0x88a00010 },
{ 0x00008270, 0x00000000 },
{ 0x00008274, 0x40000000 },
{ 0x00008278, 0x003e4180 },
--
1.7.0.5



2010-04-27 05:40:46

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] ath9k: Avoid corrupt frames being forwarded to mac80211.

On Mon, Apr 26, 2010 at 10:32 PM, Vivek Natarajan
<[email protected]> wrote:
> On Sat, Apr 24, 2010 at 9:15 AM, Felix Fietkau <[email protected]> wrote:
>> On 2010-04-23 9:26 AM, Vivek Natarajan wrote:
>>> If bit 29 is set, MAC H/W can attempt to decrypt the received aggregate
>>> with WEP or TKIP, eventhough the received frame may be a CRC failed
>>> corrupted frame.
>>> diff --git a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
>>> index cd953f6..025c31a 100644
>>> --- a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
>>> +++ b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
>>> @@ -249,7 +249,7 @@ static const u32 ar5416Common[][2] = {
>>>      { 0x00008258, 0x00000000 },
>>>      { 0x0000825c, 0x400000ff },
>>>      { 0x00008260, 0x00080922 },
>>> -    { 0x00008264, 0xa8000010 },
>>> +    { 0x00008264, 0x88000010 },
>> I don't think this is enough. This register is called
>> AR_MAC_PCU_LOGIC_ANALYZER and the field you're modifying
>> (AR_MAC_PCU_LOGIC_ANALYZER_DISBUG20768) is being touched by the function
>> ar9002_hw_enable_async_fifo() in ar9002_hw.c.
>> So unless this AR9287 v1.2 or later is unaffected by this issue, the
>> REG_SET_BIT call should be removed as well.
>> By the way, is this change in the other Atheros codebases as well?
>
> Yes. This change is present in other Atheros codebases as well and the
> REG_SET_BIT call is also present for AR9287. This is the feedback that
> I got from the Systems team regarding this register.
> "How H/W works is, if bit 29 is set, H/W obeys key type in keycache.
> If bit 29 is not set and if key type in keycache is neither open nor
> AES, H/W forces key type to be open. Since AsyncFIFO feature wants to
> encrypt/decrypt aggregate with WEP or TKIP, bit 29 should have been
> set to 1 after populating keycache with WEP or TKIP key type."

This would make that commit log entry for this patch actually a good
one. Would you mind massaging that a little and resending with some of
that added?

> Hence I suppose the REG_SET_BIT call needs to be there while enabling
> AsyncFIFO for AR9287.
>
> Vivek.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

2010-04-27 05:32:35

by Vivek Natarajan

[permalink] [raw]
Subject: Re: [PATCH] ath9k: Avoid corrupt frames being forwarded to mac80211.

On Sat, Apr 24, 2010 at 9:15 AM, Felix Fietkau <[email protected]> wrote:
> On 2010-04-23 9:26 AM, Vivek Natarajan wrote:
>> If bit 29 is set, MAC H/W can attempt to decrypt the received aggregate
>> with WEP or TKIP, eventhough the received frame may be a CRC failed
>> corrupted frame.
>> diff --git a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
>> index cd953f6..025c31a 100644
>> --- a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
>> +++ b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
>> @@ -249,7 +249,7 @@ static const u32 ar5416Common[][2] = {
>> ? ? ?{ 0x00008258, 0x00000000 },
>> ? ? ?{ 0x0000825c, 0x400000ff },
>> ? ? ?{ 0x00008260, 0x00080922 },
>> - ? ?{ 0x00008264, 0xa8000010 },
>> + ? ?{ 0x00008264, 0x88000010 },
> I don't think this is enough. This register is called
> AR_MAC_PCU_LOGIC_ANALYZER and the field you're modifying
> (AR_MAC_PCU_LOGIC_ANALYZER_DISBUG20768) is being touched by the function
> ar9002_hw_enable_async_fifo() in ar9002_hw.c.
> So unless this AR9287 v1.2 or later is unaffected by this issue, the
> REG_SET_BIT call should be removed as well.
> By the way, is this change in the other Atheros codebases as well?

Yes. This change is present in other Atheros codebases as well and the
REG_SET_BIT call is also present for AR9287. This is the feedback that
I got from the Systems team regarding this register.
"How H/W works is, if bit 29 is set, H/W obeys key type in keycache.
If bit 29 is not set and if key type in keycache is neither open nor
AES, H/W forces key type to be open. Since AsyncFIFO feature wants to
encrypt/decrypt aggregate with WEP or TKIP, bit 29 should have been
set to 1 after populating keycache with WEP or TKIP key type."

Hence I suppose the REG_SET_BIT call needs to be there while enabling
AsyncFIFO for AR9287.

Vivek.

2010-04-24 03:45:55

by Felix Fietkau

[permalink] [raw]
Subject: Re: [PATCH] ath9k: Avoid corrupt frames being forwarded to mac80211.

On 2010-04-23 9:26 AM, Vivek Natarajan wrote:
> If bit 29 is set, MAC H/W can attempt to decrypt the received aggregate
> with WEP or TKIP, eventhough the received frame may be a CRC failed
> corrupted frame.
>
> Cc: [email protected]
> Reported-by: Johan Hovold <[email protected]>
> Signed-off-by: Vivek Natarajan <[email protected]>
> Signed-off-by: Ranga Rao Ravuri <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/ar5008_initvals.h | 2 +-
> drivers/net/wireless/ath/ath9k/ar9002_initvals.h | 8 ++++----
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
> index cd953f6..025c31a 100644
> --- a/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
> +++ b/drivers/net/wireless/ath/ath9k/ar5008_initvals.h
> @@ -249,7 +249,7 @@ static const u32 ar5416Common[][2] = {
> { 0x00008258, 0x00000000 },
> { 0x0000825c, 0x400000ff },
> { 0x00008260, 0x00080922 },
> - { 0x00008264, 0xa8000010 },
> + { 0x00008264, 0x88000010 },
I don't think this is enough. This register is called
AR_MAC_PCU_LOGIC_ANALYZER and the field you're modifying
(AR_MAC_PCU_LOGIC_ANALYZER_DISBUG20768) is being touched by the function
ar9002_hw_enable_async_fifo() in ar9002_hw.c.
So unless this AR9287 v1.2 or later is unaffected by this issue, the
REG_SET_BIT call should be removed as well.
By the way, is this change in the other Atheros codebases as well? If
so, maybe this change should go into the ini override functions instead,
to make it easier to keep initvals in sync.

- Felix