wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

Function iwlagn_send_sta_key() was trying to write across multiple structure members in a single memcpy(). Add a struct group "keys" to let the compiler see the intended bounds of the memcpy, which includes the tkip keys as well. Silences false positive memcpy() run-time warning:

memcpy: detected field-spanning write (size 32) of single field "sta_cmd.key.key" at drivers/net/wireless/intel/iwlwifi/dvm/sta.c:1103 (size 16)

Link https://www.alionet.org/index.php?topic=1469.0

Cc Gregory Greenman <[email protected]>

Cc Kalle Valo <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Johannes Berg <[email protected]>

Cc Benjamin Berg <[email protected]>

Cc Sriram R <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

drivers/net/wireless/intel/iwlwifi/dvm/sta.c static int iwlagn_send_sta_key(struct iwl_priv *priv, · line 1096

Quoted 5 lines · Kees Cook

drivers/net/wireless/intel/iwlwifi/dvm/sta.c +1 -1

 	case WLAN_CIPHER_SUITE_CCMP:
 		key_flags |= STA_KEY_FLG_CCMP;
-		memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
+		memcpy(&sta_cmd.key.keys, keyconf->key, keyconf->keylen);

This should be fine though, only up to 16 bytes for CCMP.

drivers/net/wireless/intel/iwlwifi/dvm/sta.c static int iwlagn_send_sta_key(struct iwl_priv *priv, · line 1103

Quoted 7 lines

drivers/net/wireless/intel/iwlwifi/dvm/sta.c +1 -1

 	case WLAN_CIPHER_SUITE_TKIP:
 		key_flags |= STA_KEY_FLG_TKIP;
 		sta_cmd.key.tkip_rx_tsc_byte2 = tkip_iv32;
 		for (i = 0; i < 5; i++)
 			sta_cmd.key.tkip_rx_ttak[i] = cpu_to_le16(tkip_p1k[i]);
-		memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
+		memcpy(&sta_cmd.key.keys, keyconf->key, keyconf->keylen);

And that's actually a bug, we should've copied only 16 bytes, I guess. DVM didn't support MIC offload anyway (at least the way Linux uses the firmware, though I thought it doesn't at all), so we don't need the MIC RX/TX keys in there, but anyway the sequence counter values are not part of the key material on the host.

I don't think I have a machine now to test this with (nor a TKIP AP, of course, but that could be changed) - but I suspect that since we actually calculate the TTAK above, we might not even need this memcpy() at all?

Quoted 15 lines

drivers/net/wireless/intel/iwlwifi/dvm/sta.c static int iwlagn_send_sta_key(struct iwl_priv *priv, · line 1103

Quoted 8 lines

drivers/net/wireless/intel/iwlwifi/dvm/sta.c +1 -1

 	case WLAN_CIPHER_SUITE_TKIP:
 		key_flags |= STA_KEY_FLG_TKIP;
 		sta_cmd.key.tkip_rx_tsc_byte2 = tkip_iv32;
 		for (i = 0; i < 5; i++)
 			sta_cmd.key.tkip_rx_ttak[i] = cpu_to_le16(tkip_p1k[i]);
-		memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
+		memcpy(&sta_cmd.key.keys, keyconf->key, keyconf->keylen);

And that's actually a bug, we should've copied only 16 bytes, I guess. DVM didn't support MIC offload anyway (at least the way Linux uses the firmware, though I thought it doesn't at all), so we don't need the MIC RX/TX keys in there, but anyway the sequence counter values are not part of the key material on the host.

Quoted 4 lines

I don't think I have a machine now to test this with (nor a TKIP AP, of course, but that could be changed) - but I suspect that since we actually calculate the TTAK above, we might not even need this memcpy() at all?

It's the latter that is triggered in the real world, though. See the referenced URL and also now on bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=217214
i.e.: drivers/net/wireless/intel/iwlwifi/dvm/sta.c:1103

So keyconf->keylen is coming in as 32. If this is a bug, I'm not sure where/how to fix it.

Quoted 20 lines

Quoted 15 lines

drivers/net/wireless/intel/iwlwifi/dvm/sta.c static int iwlagn_send_sta_key(struct iwl_priv *priv, · line 1103

Quoted 8 lines

drivers/net/wireless/intel/iwlwifi/dvm/sta.c +1 -1

 	case WLAN_CIPHER_SUITE_TKIP:
 		key_flags |= STA_KEY_FLG_TKIP;
 		sta_cmd.key.tkip_rx_tsc_byte2 = tkip_iv32;
 		for (i = 0; i < 5; i++)
 			sta_cmd.key.tkip_rx_ttak[i] = cpu_to_le16(tkip_p1k[i]);
-		memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
+		memcpy(&sta_cmd.key.keys, keyconf->key, keyconf->keylen);

And that's actually a bug, we should've copied only 16 bytes, I guess. DVM didn't support MIC offload anyway (at least the way Linux uses the firmware, though I thought it doesn't at all), so we don't need the MIC RX/TX keys in there, but anyway the sequence counter values are not part of the key material on the host.

Quoted 4 lines

I don't think I have a machine now to test this with (nor a TKIP AP, of course, but that could be changed) - but I suspect that since we actually calculate the TTAK above, we might not even need this memcpy() at all?

Quoted 7 lines

It's the latter that is triggered in the real world, though. See the referenced URL and also now on bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=217214
i.e.: drivers/net/wireless/intel/iwlwifi/dvm/sta.c:1103

So keyconf->keylen is coming in as 32. If this is a bug, I'm not sure where/how to fix it.

Yes, I know it's coming in as such - I believe it should be copying 16 bytes instead of the full keylen. TKIP keys are comprised of 16 bytes encryption/decryption key and 8 bytes TX/RX MIC keys for a total of 32, but since the device doesn't do MIC calculations, it only needs the first 16 bytes here (if even that, since we also give it the P1K which is derived from the TK...? maybe not even that)

But I guess we should test it ... not sure I still have a machine that takes these NICs (I do have NICs).

What sort of patch would you like here? How should the other cases in the switch statement behave?

Are these the correct bounds?

	WLAN_CIPHER_SUITE_CCMP:   keylen <= 16
	WLAN_CIPHER_SUITE_TKIP:   keylen <= 16
	WLAN_CIPHER_SUITE_WEP104: keylen <= 13
	WLAN_CIPHER_SUITE_WEP40:  keylen <= 13

and should it silently ignore larger values in each case?