linux-kernel - Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250410215527.3001-1-spasswolf@web.de>
Date: Thu, 10 Apr 2025 23:55:26 +0200
From: Bert Karwatzki <spasswolf@....de>
To: Remi Pommarel <repk@...plefau.lt>
Cc: Bert Karwatzki <spasswolf@....de>,
	linux-wireless@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	johannes@...solutions.net
Subject: Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

This commit breaks the mediatek mt7921 wireless driver. In linux-next-20250410
my mt7921e Wi-Fi controller is no longer able to connect to a network.
I bisected this to commit a104042e2bf6 ("wifi: mac80211: Update skb's control
block key in ieee80211_tx_dequeue()").

Hardware:
04:00.0 Network controller: MEDIATEK Corp. MT7921K (RZ608) Wi-Fi 6E 80MHz

This debugging patch reveals that the change causes key to be NULL in
mt7921_tx_prepare_skb().

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
index 881812ba03ff..3b8552a1055c 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
@@ -13,6 +13,7 @@ int mt7921e_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
        struct mt792x_dev *dev = container_of(mdev, struct mt792x_dev, mt76);
        struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
        struct ieee80211_key_conf *key = info->control.hw_key;
+       dev_info(mdev->dev, "%s: key = %px\n", __func__, key);
        struct mt76_connac_hw_txp *txp;
        struct mt76_txwi_cache *t;
        int id, pid;


So why is info->control.hw_key not updated by ieee80211_tx_h_select_key()?

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 34f229a6eab0..2510e3533d13 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -631,8 +631,10 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 		case WLAN_CIPHER_SUITE_WEP40:
 		case WLAN_CIPHER_SUITE_WEP104:
 		case WLAN_CIPHER_SUITE_TKIP:
-			if (!ieee80211_is_data_present(hdr->frame_control))
+			if (!ieee80211_is_data_present(hdr->frame_control)) {
+				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
 				tx->key = NULL;
+			}
 			break;
 		case WLAN_CIPHER_SUITE_CCMP:
 		case WLAN_CIPHER_SUITE_CCMP_256:
@@ -641,19 +643,23 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 			if (!ieee80211_is_data_present(hdr->frame_control) &&
 			    !ieee80211_use_mfp(hdr->frame_control, tx->sta,
 					       tx->skb) &&
-			    !ieee80211_is_group_privacy_action(tx->skb))
+			    !ieee80211_is_group_privacy_action(tx->skb)) {
+				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
 				tx->key = NULL;
-			else
+			} else {
 				skip_hw = (tx->key->conf.flags &
 					   IEEE80211_KEY_FLAG_SW_MGMT_TX) &&
 					ieee80211_is_mgmt(hdr->frame_control);
+			}
 			break;
 		case WLAN_CIPHER_SUITE_AES_CMAC:
 		case WLAN_CIPHER_SUITE_BIP_CMAC_256:
 		case WLAN_CIPHER_SUITE_BIP_GMAC_128:
 		case WLAN_CIPHER_SUITE_BIP_GMAC_256:
-			if (!ieee80211_is_mgmt(hdr->frame_control))
+			if (!ieee80211_is_mgmt(hdr->frame_control)) {
+				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
 				tx->key = NULL;
+			}
 			break;
 		}

@@ -662,9 +668,13 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 			     tx->skb->protocol != tx->sdata->control_port_protocol)
 			return TX_DROP;

+		printk(KERN_INFO "%s: skip_hw=%d tx->key=%px\n",
+				__func__, skip_hw, tx->key);
 		if (!skip_hw && tx->key &&
-		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)
+		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
 			info->control.hw_key = &tx->key->conf;
+			printk(KERN_INFO "%s: info->control.hw_key = %px\n", __func__, info->control.hw_key);
+		}
 	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
 		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
 		return TX_DROP;
@@ -3894,6 +3904,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
 	 * The key can be removed while the packet was queued, so need to call
 	 * this here to get the current key.
 	 */
+	printk(KERN_INFO "%s: info->control.hw_key = %px, setting to NULL\n",
+			__func__, info->control.hw_key);
 	info->control.hw_key = NULL;
 	r = ieee80211_tx_h_select_key(&tx);
 	if (r != TX_CONTINUE) {

This patch reveals that tx->key is set to NULL (in the @@ -641,19 +643,23 @@ chunk)
and so the updating of info->contro.hw_key is skipped:

[   17.411298] [   T1232] ieee80211_tx_h_select_key 647: setting tx->key = NULL
[   17.411300] [   T1232] ieee80211_tx_h_select_key: skip_hw=0 tx->key=0000000000000000
[   17.411307] [   T1232] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key = 0000000000000000

If I revert commit a104042e2bf6 while keeping the debug patches it shows that
the for mt7921e the key is never updated in ieee80211_tx_h_select_key(), mt7921e
relies on the key your patch is setting to NULL.

Is this a problem with your patch or with the mt7921e driver that just got
revealed by your patch?

Bert Karwatzki