[ath9k-devel] [NOT FOR MERGE] ath9k: work around key cache corruption

[Johannes Berg]

> "The patch itself has (at least) one big problem. It is using some
> mac80211
> internals in ath_key_config_iter to make sure that the uploaded keys
> were
> actually programmed in the hardware. Without this check the keys
> could end up
> in the lower slots and thus break all connections."

The KEY_FLAG_UPLOADED_TO_HARDWARE use? Might not be so nice, but it's
probably not really a problem. If you wanted to avoid it, you could
just use the hw_key_idx in some way, e.g. the highest-order bit
indicates that you've set this value, or you just make sure that 0 is
an invalid value and set it to real index + 1 or something, then you
can check that?

johannes

[Kalle Valo]

Antonio Quartulli <***@unstable.cc> writes:

> From: Antonio Quartulli <***@open-mesh.com>
>
> This patch was crafted long time ago to work around a key cache
> corruption problem on ath9k chipsets.
>
> The workaround consists in periodically triggering a worker that
> uploads all the keys to the HW cache. The worker is triggered also
> when the vif detects 21 undecryptable packets.
>
>
> This patch is based on top compat-wireless-2015-10-26.
>
>
> I was asked to release this code to the public and, since it is
> GPL, I am now doing it.

GPL? AFAICS ath9k is under ISC. I'm not sure what you mean with the
sentence above, but it's possible to interpret it so that this patch is
not under ISC license, which is problematic.

--
Kalle Valo

[Ferry Huberts]

do a grep in the kernel: dual license bsd/gpl

On 26/10/16 16:05, Kalle Valo wrote:
> Antonio Quartulli <***@unstable.cc> writes:
>
>> From: Antonio Quartulli <***@open-mesh.com>
>>
>> This patch was crafted long time ago to work around a key cache
>> corruption problem on ath9k chipsets.
>>
>> The workaround consists in periodically triggering a worker that
>> uploads all the keys to the HW cache. The worker is triggered also
>> when the vif detects 21 undecryptable packets.
>>
>>
>> This patch is based on top compat-wireless-2015-10-26.
>>
>>
>> I was asked to release this code to the public and, since it is
>> GPL, I am now doing it.
>
> GPL? AFAICS ath9k is under ISC. I'm not sure what you mean with the
> sentence above, but it's possible to interpret it so that this patch is
> not under ISC license, which is problematic.
>

--
Ferry Huberts

[Kalle Valo]

Antonio Quartulli <***@unstable.cc> writes:

> On Wed, Oct 26, 2016 at 05:05:14PM +0300, Kalle Valo wrote:
>> Antonio Quartulli <***@unstable.cc> writes:
>>
>> > From: Antonio Quartulli <***@open-mesh.com>
>> >
>> > This patch was crafted long time ago to work around a key cache
>> > corruption problem on ath9k chipsets.
>> >
>> > The workaround consists in periodically triggering a worker that
>> > uploads all the keys to the HW cache. The worker is triggered also
>> > when the vif detects 21 undecryptable packets.
>> >
>> >
>> > This patch is based on top compat-wireless-2015-10-26.
>> >
>> >
>> > I was asked to release this code to the public and, since it is
>> > GPL, I am now doing it.
>>
>> GPL? AFAICS ath9k is under ISC. I'm not sure what you mean with the
>> sentence above, but it's possible to interpret it so that this patch is
>> not under ISC license, which is problematic.
>
> Honestly, my sentence was just a way to say "it makes sense to release this
> patch to the public".

I was suspecting it was like that, I just wasn't sure.

> If it needs to be ISC in order to be merged, then it can be released under ISC.
>
> I don't want to enter a legal case :)

Me neither. But I can't apply patches with unclear license.

--
Kalle Valo

[Johannes Berg]

On Thu, 2016-10-27 at 09:54 +0200, Sebastian Gottschall wrote:
> all patches have a unclear license since most patches are not comming
> with any licence declaration ;-)

You should read the DCO some time. Maybe you shouldn't be sending
patches if you think so.

johannes

[Stam, Michel [FINT]]

Dear List,

After 2 weeks of Ferry Huberts and me trying to resolve this issue, this is the result:
The problem can be mitigated by resetting the chip and immediately replumbing the encryption keys. I wrote a patch for this, which unfortunately this seems to suffer from some locking issues, rtnl_lock and sc->mutex being the chief culprits.

Code snippet:
static void ath9k_plumb_key(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
struct ieee80211_key_conf *keyconf, void *data)
{
struct ath_common *common = (struct ath_common *)data;
int ret;

/* delete and re-install keys which were programmed into the hardware */
if (vif && test_bit(keyconf->hw_key_idx, common->keymap)) {
ath_key_delete(common, keyconf);
keyconf->hw_key_idx = 0;
ret = ath_key_config(common, vif, sta, keyconf);
if (ret >= 0)
keyconf->hw_key_idx = ret;
}
}

To reset and replumb, taken from ath_reset_internal():
ath9k_hw_kill_interrupts(sc->sc_ah);
set_bit(ATH_OP_HW_RESET, &common->op_flags);
ath9k_ps_wakeup(sc);
ath_reset_internal(sc, NULL);
rtnl_lock();
ieee80211_iter_keys(sc->hw, NULL, ath9k_plumb_key, common);
rtnl_unlock();
ath9k_ps_restore(sc);

In any case, the problem is only mitigated. It is not solved, as I see the error appearing still (less frequently though).
Looking at the OpenHAL has not yielded anything working either (there is a reference to KEY_PLUMB_WAR, but it is not a full implementation).

What I understand from previous attempts to tackle this issue is that Qualcomm is aware of the problem, and a workaround exists in at least the proprietary driver. I have tried to contact Qualcomm support about this, since it is not radio card specific, but I have not received any response so far.

I hope a Qualcomm employee reads this and is able to help obtain the information that is necessary to resolve this issue. Until then, I recommend using software encryption (nohwcrypt), or setting a very long rekeying time.

Kind regards,

Michel Stam
-----Original Message-----
From: ath9k-devel-***@lists.ath9k.org [mailto:ath9k-devel-***@lists.ath9k.org] On Behalf Of Johannes Berg
Sent: Thursday, October 27, 2016 17:07 PM
To: Sebastian Gottschall; Kalle Valo; Antonio Quartulli
Cc: ath9k-***@lists.ath9k.org; linux-***@vger.kernel.org; Antonio Quartulli
Subject: Re: [ath9k-devel] [NOT FOR MERGE] ath9k: work around key cache corruption

On Thu, 2016-10-27 at 09:54 +0200, Sebastian Gottschall wrote:
> all patches have a unclear license since most patches are not comming
> with any licence declaration ;-)

You should read the DCO some time. Maybe you shouldn't be sending
patches if you think so.

johannes

[Adrian Chadd]

Hiya,

maybe the right thing to do is to set a flag that says "please
replumb", then do a reset, and have the reset path see if we need to
replumb keys and do so?

To make locking less terrible, maybe we need to cache the keys in the
ath9k driver somewhere so replumbing doesn't require reaching into
mac82011.



-adrian

[Stam, Michel [FINT]]

Hey Adrian,

Sounds like an idea, although up to this point I have not been able to detect whether replumbing is necessary. decrypt_errors does not increment exponentially on the receive path, as far as I saw only one decrypt_error comes through, which could just as well be a corrupted frame. I have not seen any other indicators of this issue, other than a complete lack of connectivity on unicast frames.

So this attempt was to see if I could get the link stable, in any way possible, by blankly replumbing on every rekey. This should, in theory "solve" the issue, although at the cost of huge packet loss. So far, no go.

I agree that some form of key caching may be necessary. It is a pity, because the mac80211 layer does have an interface for grabbing the keys, although it requires the rtnl_lock. As far as I saw, only the Intel wireless driver uses it to write the keys back into the chip upon resuming from a low-power state.

Kind regards,
Fugro Intersite B.V.

Michel Stam
-----Original Message-----
From: ***@gmail.com [mailto:***@gmail.com] On Behalf Of Adrian Chadd
Sent: Tuesday, November 15, 2016 0:25 AM
To: Stam, Michel [FINT]
Cc: Johannes Berg; Sebastian Gottschall; Kalle Valo; Antonio Quartulli; ath9k-***@lists.ath9k.org; linux-***@vger.kernel.org; Antonio Quartulli
Subject: Re: [ath9k-devel] [NOT FOR MERGE] ath9k: work around key cache corruption

Hiya,

maybe the right thing to do is to set a flag that says "please
replumb", then do a reset, and have the reset path see if we need to
replumb keys and do so?

To make locking less terrible, maybe we need to cache the keys in the
ath9k driver somewhere so replumbing doesn't require reaching into
mac82011.



-adrian