[PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Nicolas Escande]

This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.

This as been reported by multiple people [0] that with this commit,
broadcast packets were not being delivered after GTK exchange.
Qualcomm seems to have a similar patch [1] confirming the issue.

[0] https://lore.kernel.org/linux-wireless/Z2Q9POuV-6MIdzRf@pilgrim/
[1] https://git.codelinaro.org/clo/qsdk/oss/system/feeds/wlan-open/-/blob/win.wlan_host_opensource.3.0.r24/patches/ath11k/350-ath11k-Revert-clear-the-keys-properly-when-DISABLE_K.patch

Fixes: 436a4e886598 ("ath11k: clear the keys properly via DISABLE_KEY")
Reported-by: Steffen Moser <lists@steffen-moser.de>
Closes: https://lore.kernel.org/linux-wireless/c6366409-9928-4dd7-bf7b-ba7fcf20eabf@steffen-moser.de
Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
---
 drivers/net/wireless/ath/ath11k/mac.c | 4 +++-
 drivers/net/wireless/ath/ath11k/wmi.c | 3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 1556392f7ad4..70793f8b1081 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -4220,7 +4220,9 @@ static int ath11k_install_key(struct ath11k_vif *arvif,
 		return 0;
 
 	if (cmd == DISABLE_KEY) {
-		arg.key_cipher = WMI_CIPHER_NONE;
+		/* TODO: Check if FW expects  value other than NONE for del */
+		/* arg.key_cipher = WMI_CIPHER_NONE; */
+		arg.key_len = 0;
 		arg.key_data = NULL;
 		goto install;
 	}
diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
index 87abfa547529..02ff0a58952d 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -1854,8 +1854,7 @@ int ath11k_wmi_vdev_install_key(struct ath11k *ar,
 	tlv = (struct wmi_tlv *)(skb->data + sizeof(*cmd));
 	tlv->header = FIELD_PREP(WMI_TLV_TAG, WMI_TAG_ARRAY_BYTE) |
 		      FIELD_PREP(WMI_TLV_LEN, key_len_aligned);
-	if (arg->key_data)
-		memcpy(tlv->value, (u8 *)arg->key_data, key_len_aligned);
+	memcpy(tlv->value, (u8 *)arg->key_data, key_len_aligned);
 
 	ret = ath11k_wmi_cmd_send(wmi, skb, WMI_VDEV_INSTALL_KEY_CMDID);
 	if (ret) {
-- 
2.48.1

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Vasanthakumar Thiagarajan]

Hi Nicolas,

On 1/18/2025 12:44 AM, Nicolas Escande wrote:
> This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
> 
> This as been reported by multiple people [0] that with this commit,
> broadcast packets were not being delivered after GTK exchange.
> Qualcomm seems to have a similar patch [1] confirming the issue.
> 

This will re-open https://www.spinics.net/lists/hostap/msg08921.html
reported by Sven. The recommended ath firmware ABI during GTK re-keying
is SET_KEY instead of current DEL_KEY followed by SET_KEY. We are looking
at other options like some marking by mac80211 for the driver to be able
to identify if the received DEL_KEY is for re-keying. Also I'm curious
if roaming between secure and non-secure mode is a critical use case.
If not, we can probably go ahead with this revert as temporary WAR,
@Sven?

Vasanth

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Nicolas Escande]

On Sat Jan 18, 2025 at 11:29 AM CET, Vasanthakumar Thiagarajan wrote:
> Hi Nicolas,
>
> On 1/18/2025 12:44 AM, Nicolas Escande wrote:
> > This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
> > 
> > This as been reported by multiple people [0] that with this commit,
> > broadcast packets were not being delivered after GTK exchange.
> > Qualcomm seems to have a similar patch [1] confirming the issue.
> > 
>
> This will re-open https://www.spinics.net/lists/hostap/msg08921.html
> reported by Sven. The recommended ath firmware ABI during GTK re-keying
> is SET_KEY instead of current DEL_KEY followed by SET_KEY. We are looking
> at other options like some marking by mac80211 for the driver to be able
> to identify if the received DEL_KEY is for re-keying. Also I'm curious
> if roaming between secure and non-secure mode is a critical use case.
> If not, we can probably go ahead with this revert as temporary WAR,
> @Sven?
>
> Vasanth

Yes, indeed it will make the original problem appear once again.

But from my standpoint, switching from encrypted to unencrypted traffic with a
config reload (without an interface restart) is not so frequent of a usecase.

On the otherhand, GTK rekeying is much more frequent. Like once per day with
hostapd's default parameters. And from our tests it fails around 1/4 of the time
with ath11k. So for every 4 days of operations of an AP running the unreverted
code, you won't have broadcast working for one of them...

I would really like a proper fix from QCA, but in the meantime it seems best to
revert it IMHO.

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Nicolas Escande]

On Sat Jan 18, 2025 at 11:29 AM CET, Vasanthakumar Thiagarajan wrote:
> Hi Nicolas,
>
> On 1/18/2025 12:44 AM, Nicolas Escande wrote:
>> This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
>> 
>> This as been reported by multiple people [0] that with this commit,
>> broadcast packets were not being delivered after GTK exchange.
>> Qualcomm seems to have a similar patch [1] confirming the issue.
>> 
>
> This will re-open https://www.spinics.net/lists/hostap/msg08921.html
> reported by Sven. The recommended ath firmware ABI during GTK re-keying
> is SET_KEY instead of current DEL_KEY followed by SET_KEY. We are looking
> at other options like some marking by mac80211 for the driver to be able
> to identify if the received DEL_KEY is for re-keying. Also I'm curious
> if roaming between secure and non-secure mode is a critical use case.
> If not, we can probably go ahead with this revert as temporary WAR,
> @Sven?
>
> Vasanth

Hello,

Any news on this ?
I would hate for this to sink into oblivion once again given how hard this
affects end users when it does hit.

Thanks,

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Vasanthakumar Thiagarajan]

Hi Nicolas

On 5/4/2025 6:06 PM, Nicolas Escande wrote:
> On Sat Jan 18, 2025 at 11:29 AM CET, Vasanthakumar Thiagarajan wrote:
>> Hi Nicolas,
>>
>> On 1/18/2025 12:44 AM, Nicolas Escande wrote:
>>> This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
>>>
>>> This as been reported by multiple people [0] that with this commit,
>>> broadcast packets were not being delivered after GTK exchange.
>>> Qualcomm seems to have a similar patch [1] confirming the issue.
>>>
>>
>> This will re-open https://www.spinics.net/lists/hostap/msg08921.html
>> reported by Sven. The recommended ath firmware ABI during GTK re-keying
>> is SET_KEY instead of current DEL_KEY followed by SET_KEY. We are looking
>> at other options like some marking by mac80211 for the driver to be able
>> to identify if the received DEL_KEY is for re-keying. Also I'm curious
>> if roaming between secure and non-secure mode is a critical use case.
>> If not, we can probably go ahead with this revert as temporary WAR,
>> @Sven?
>>
>> Vasanth
> 
> Hello,
> 
> Any news on this ?
> I would hate for this to sink into oblivion once again given how hard this
> affects end users when it does hit.

We are working on a driver change w/o reverting the commit, we'll share
the patch once complete.

Vasanth

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Nicolas Escande]

On Tue May 6, 2025 at 11:19 AM CEST, Vasanthakumar Thiagarajan wrote:
> Hi Nicolas
>
> On 5/4/2025 6:06 PM, Nicolas Escande wrote:
>> On Sat Jan 18, 2025 at 11:29 AM CET, Vasanthakumar Thiagarajan wrote:
>>> Hi Nicolas,
>>>
>>> On 1/18/2025 12:44 AM, Nicolas Escande wrote:
>>>> This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
>>>>
>>>> This as been reported by multiple people [0] that with this commit,
>>>> broadcast packets were not being delivered after GTK exchange.
>>>> Qualcomm seems to have a similar patch [1] confirming the issue.
>>>>
>>>
>>> This will re-open https://www.spinics.net/lists/hostap/msg08921.html
>>> reported by Sven. The recommended ath firmware ABI during GTK re-keying
>>> is SET_KEY instead of current DEL_KEY followed by SET_KEY. We are looking
>>> at other options like some marking by mac80211 for the driver to be able
>>> to identify if the received DEL_KEY is for re-keying. Also I'm curious
>>> if roaming between secure and non-secure mode is a critical use case.
>>> If not, we can probably go ahead with this revert as temporary WAR,
>>> @Sven?
>>>
>>> Vasanth
>> 
>> Hello,
>> 
>> Any news on this ?
>> I would hate for this to sink into oblivion once again given how hard this
>> affects end users when it does hit.
>
> We are working on a driver change w/o reverting the commit, we'll share
> the patch once complete.
>
> Vasanth

Hello,

I might be mistaken but I did not see anything posted for that yet, right ?

Thanks

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Vasanthakumar Thiagarajan]

On 6/27/2025 1:01 PM, Nicolas Escande wrote:
> On Tue May 6, 2025 at 11:19 AM CEST, Vasanthakumar Thiagarajan wrote:
>> Hi Nicolas
>>
>> On 5/4/2025 6:06 PM, Nicolas Escande wrote:
>>> On Sat Jan 18, 2025 at 11:29 AM CET, Vasanthakumar Thiagarajan wrote:
>>>> Hi Nicolas,
>>>>
>>>> On 1/18/2025 12:44 AM, Nicolas Escande wrote:
>>>>> This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
>>>>>
>>>>> This as been reported by multiple people [0] that with this commit,
>>>>> broadcast packets were not being delivered after GTK exchange.
>>>>> Qualcomm seems to have a similar patch [1] confirming the issue.
>>>>>
>>>>
>>>> This will re-open https://www.spinics.net/lists/hostap/msg08921.html
>>>> reported by Sven. The recommended ath firmware ABI during GTK re-keying
>>>> is SET_KEY instead of current DEL_KEY followed by SET_KEY. We are looking
>>>> at other options like some marking by mac80211 for the driver to be able
>>>> to identify if the received DEL_KEY is for re-keying. Also I'm curious
>>>> if roaming between secure and non-secure mode is a critical use case.
>>>> If not, we can probably go ahead with this revert as temporary WAR,
>>>> @Sven?
>>>>
>>>> Vasanth
>>>
>>> Hello,
>>>
>>> Any news on this ?
>>> I would hate for this to sink into oblivion once again given how hard this
>>> affects end users when it does hit.
>>
>> We are working on a driver change w/o reverting the commit, we'll share
>> the patch once complete.
>>
>> Vasanth
> 
> Hello,
> 
> I might be mistaken but I did not see anything posted for that yet, right ?

Patch has not been posted yet. Since the code change does not
look straight forward and the issue is security related it takes
time. This issue is being actively looked into.

Vasanth

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Jeff Johnson]

On 1/17/2025 11:14 AM, Nicolas Escande wrote:
> This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
> 
> This as been reported by multiple people [0] that with this commit,
> broadcast packets were not being delivered after GTK exchange.
> Qualcomm seems to have a similar patch [1] confirming the issue.
> 
> [0] https://lore.kernel.org/linux-wireless/Z2Q9POuV-6MIdzRf@pilgrim/
> [1] https://git.codelinaro.org/clo/qsdk/oss/system/feeds/wlan-open/-/blob/win.wlan_host_opensource.3.0.r24/patches/ath11k/350-ath11k-Revert-clear-the-keys-properly-when-DISABLE_K.patch
> 
> Fixes: 436a4e886598 ("ath11k: clear the keys properly via DISABLE_KEY")
> Reported-by: Steffen Moser <lists@steffen-moser.de>
> Closes: https://lore.kernel.org/linux-wireless/c6366409-9928-4dd7-bf7b-ba7fcf20eabf@steffen-moser.de
> Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
> ---

Can you please check if this alternative addresses the issue:
[PATCH ath-current v4] wifi: ath11k: fix group data packet drops during rekey
https://msgid.link/20250810170018.1124014-1-rameshkumar.sundaram@oss.qualcomm.com

Any Tested-on/Tested-by tags would be appreciated!

/jeff

Re: [PATCH] Revert "ath11k: clear the keys properly via DISABLE_KEY"

[Nicolas Escande]

On Tue Aug 12, 2025 at 1:34 AM CEST, Jeff Johnson wrote:
> On 1/17/2025 11:14 AM, Nicolas Escande wrote:
>> This reverts commit 436a4e88659842a7cf634d7cc088c8f2cc94ebf5.
>> 
>> This as been reported by multiple people [0] that with this commit,
>> broadcast packets were not being delivered after GTK exchange.
>> Qualcomm seems to have a similar patch [1] confirming the issue.
>> 
>> [0] https://lore.kernel.org/linux-wireless/Z2Q9POuV-6MIdzRf@pilgrim/
>> [1] https://git.codelinaro.org/clo/qsdk/oss/system/feeds/wlan-open/-/blob/win.wlan_host_opensource.3.0.r24/patches/ath11k/350-ath11k-Revert-clear-the-keys-properly-when-DISABLE_K.patch
>> 
>> Fixes: 436a4e886598 ("ath11k: clear the keys properly via DISABLE_KEY")
>> Reported-by: Steffen Moser <lists@steffen-moser.de>
>> Closes: https://lore.kernel.org/linux-wireless/c6366409-9928-4dd7-bf7b-ba7fcf20eabf@steffen-moser.de
>> Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
>> ---
>
> Can you please check if this alternative addresses the issue:
> [PATCH ath-current v4] wifi: ath11k: fix group data packet drops during rekey
> https://msgid.link/20250810170018.1124014-1-rameshkumar.sundaram@oss.qualcomm.com
>
> Any Tested-on/Tested-by tags would be appreciated!
>
> /jeff
Yes that was on my todo.
It seems to work fine with this patch.
Thanks for taking the time to fix this one