diff mbox series

ath11k: clear the keys properly when DISABLE_KEY

Message ID 20211026155446.457935-1-sven@narfation.org (mailing list archive)
State Rejected, archived
Delegated to: Kalle Valo
Headers show
Series ath11k: clear the keys properly when DISABLE_KEY | expand

Commit Message

Sven Eckelmann Oct. 26, 2021, 3:54 p.m. UTC 
From: Karthikeyan Kathirvel <kathirve@codeaurora.org>

DISABLE_KEY sets the key_len to 0, firmware will not delete the keys if
key_len is 0. Changing from security mode to open mode will cause mcast
to be still encrypted without vdev restart.

Set the proper key_len for DISABLE_KEY cmd to clear the keys in
firmware.

Tested-on: IPQ6018 hw1.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Karthikeyan Kathirvel <kathirve@codeaurora.org>
[sven@narfation.org: Drop chunks which are for NSS and other parts QCA
 didn't upstream]
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/net/wireless/ath/ath11k/mac.c | 4 +---
 drivers/net/wireless/ath/ath11k/wmi.c | 3 ++-
 2 files changed, 3 insertions(+), 4 deletions(-)

Comments

Sven Eckelmann Oct. 26, 2021, 6:22 p.m. UTC | #1 
On Tuesday, 26 October 2021 17:54:46 CEST Sven Eckelmann wrote:
> Tested-on: IPQ6018 hw1.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1

I would like to retract this Tested-on. My test caused another problem which 
resulted in a complete shutdown of the vdev. After fixing this problem, it 
turned out that this change didn't fix anything (as far as I can see) on this 
firmware version.

Kind regards,
	Sven
Sven Eckelmann Oct. 27, 2021, 6:12 a.m. UTC | #2 
On Tuesday, 26 October 2021 17:54:46 CEST Sven Eckelmann wrote:
> Tested-on: IPQ6018 hw1.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1

I would like to retract this Tested-on. My test caused another problem which 
resulted in a complete shutdown of the vdev. After fixing this problem, it 
turned out that this change didn't fix anything (as far as I can see) on this 
firmware version.

Kind regards,
	Sven
Kalle Valo Oct. 28, 2021, 12:38 p.m. UTC | #3 
Sven Eckelmann <sven@narfation.org> writes:

> On Tuesday, 26 October 2021 17:54:46 CEST Sven Eckelmann wrote:
>> Tested-on: IPQ6018 hw1.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1
>
> I would like to retract this Tested-on. My test caused another problem which 
> resulted in a complete shutdown of the vdev. After fixing this problem, it 
> turned out that this change didn't fix anything (as far as I can see) on this 
> firmware version.

But it doesn't either break anything either, right? So in that respect I
would like to keep the Tested-on tag in the commit log to document how
it was tested.

Though I'm not sure what I do now, do you think I should the patch still
or should I drop it?
Sven Eckelmann Oct. 28, 2021, 12:43 p.m. UTC | #4 
On Thursday, 28 October 2021 14:38:27 CEST Kalle Valo wrote:
[...]
> But it doesn't either break anything either, right? So in that respect I
> would like to keep the Tested-on tag in the commit log to document how
> it was tested.
> 
> Though I'm not sure what I do now, do you think I should the patch still
> or should I drop it?

It seems like QCA wanted to have a look again at the problem. So I would 
suggest to drop it or mark it as "Changes Requested".

I cannot make any statements about whether it actually clears anything 
in the firmware because I neither have documentation nor source code of 
it. So only persons which have more knowledge about it can work on this 
problem.

Kind regards,
	Sven
Kalle Valo Nov. 1, 2021, 12:53 p.m. UTC | #5 
Sven Eckelmann <sven@narfation.org> writes:

> On Thursday, 28 October 2021 14:38:27 CEST Kalle Valo wrote:
> [...]
>> But it doesn't either break anything either, right? So in that respect I
>> would like to keep the Tested-on tag in the commit log to document how
>> it was tested.
>> 
>> Though I'm not sure what I do now, do you think I should the patch still
>> or should I drop it?
>
> It seems like QCA wanted to have a look again at the problem. So I would 
> suggest to drop it or mark it as "Changes Requested".

Ok, thanks. I see that someone already marked it as Rejected in
patchwork, and I don't think it was me :)

> I cannot make any statements about whether it actually clears anything 
> in the firmware because I neither have documentation nor source code of 
> it. So only persons which have more knowledge about it can work on this 
> problem.

Can someone from QCA comment?
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 1cc55602787b..cdee7545e876 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -3316,9 +3316,7 @@  static int ath11k_install_key(struct ath11k_vif *arvif,
 		return 0;
 
 	if (cmd == DISABLE_KEY) {
-		/* TODO: Check if FW expects  value other than NONE for del */
-		/* arg.key_cipher = WMI_CIPHER_NONE; */
-		arg.key_len = 0;
+		arg.key_cipher = WMI_CIPHER_NONE;
 		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 5ae2ef4680d6..d97be60689be 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -1689,7 +1689,8 @@  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);
-	memcpy(tlv->value, (u8 *)arg->key_data, key_len_aligned);
+	if (arg->key_data)
+		memcpy(tlv->value, (u8 *)arg->key_data, key_len_aligned);
 
 	ret = ath11k_wmi_cmd_send(wmi, skb, WMI_VDEV_INSTALL_KEY_CMDID);
 	if (ret) {