A received TKIP key may be up to 32 bytes because it may contain
MIC rx/tx keys too. These are not used by iwl and copying these
over overflows the iwl_keyinfo.key field.
Add a check to not copy more data to iwl_keyinfo.key then will fit.
This fixes backtraces like this one:
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)
WARNING: CPU: 1 PID: 946 at drivers/net/wireless/intel/iwlwifi/dvm/sta.c:1103 iwlagn_send_sta_key+0x375/0x390 [iwldvm]
<snip>
Hardware name: Dell Inc. Latitude E6430/0H3MT5, BIOS A21 05/08/2017
RIP: 0010:iwlagn_send_sta_key+0x375/0x390 [iwldvm]
<snip>
Call Trace:
<TASK>
iwl_set_dynamic_key+0x1f0/0x220 [iwldvm]
iwlagn_mac_set_key+0x1e4/0x280 [iwldvm]
drv_set_key+0xa4/0x1b0 [mac80211]
ieee80211_key_enable_hw_accel+0xa8/0x2d0 [mac80211]
ieee80211_key_replace+0x22d/0x8e0 [mac80211]
<snip>
Link: https://www.alionet.org/index.php?topic=1469.0
Link: https://lore.kernel.org/linux-wireless/[email protected]/
Link: https://lore.kernel.org/linux-wireless/[email protected]/
Cc: Kees Cook <[email protected]>
Suggested-by: Johannes Berg <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/dvm/sta.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
index cef43cf80620..8b01ab986cb1 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
@@ -1081,6 +1081,7 @@ static int iwlagn_send_sta_key(struct iwl_priv *priv,
{
__le16 key_flags;
struct iwl_addsta_cmd sta_cmd;
+ size_t to_copy;
int i;
spin_lock_bh(&priv->sta_lock);
@@ -1100,7 +1101,9 @@ static int iwlagn_send_sta_key(struct iwl_priv *priv,
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);
+ /* keyconf may contain MIC rx/tx keys which iwl does not use */
+ to_copy = min_t(size_t, sizeof(sta_cmd.key.key), keyconf->keylen);
+ memcpy(sta_cmd.key.key, keyconf->key, to_copy);
break;
case WLAN_CIPHER_SUITE_WEP104:
key_flags |= STA_KEY_FLG_KEY_SIZE_MSK;
--
2.39.2
On Tue, 2023-04-18 at 15:25 +0200, Hans de Goede wrote:
> A received TKIP key may be up to 32 bytes because it may contain
> MIC rx/tx keys too. These are not used by iwl and copying these
> over overflows the iwl_keyinfo.key field.
Thanks for doing (and more importantly testing) this :)
> - memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
> + /* keyconf may contain MIC rx/tx keys which iwl does not use */
> + to_copy = min_t(size_t, sizeof(sta_cmd.key.key), keyconf->keylen);
> + memcpy(sta_cmd.key.key, keyconf->key, to_copy);
>
I'd kind of argue that just hardcoding 16 is fine here, keylen _looks_
variable so the compiler can't optimize it away, but in reality keylen
always must be 32 and sizeof() must be always 16 :)
But it also really doesn't matter.
Reviewed-by: Johannes Berg <[email protected]>
Should we Cc stable on this?
johannes
Hi,
On 4/18/23 16:43, Johannes Berg wrote:
> On Tue, 2023-04-18 at 15:25 +0200, Hans de Goede wrote:
>> A received TKIP key may be up to 32 bytes because it may contain
>> MIC rx/tx keys too. These are not used by iwl and copying these
>> over overflows the iwl_keyinfo.key field.
>
> Thanks for doing (and more importantly testing) this :)
>
>> - memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
>> + /* keyconf may contain MIC rx/tx keys which iwl does not use */
>> + to_copy = min_t(size_t, sizeof(sta_cmd.key.key), keyconf->keylen);
>> + memcpy(sta_cmd.key.key, keyconf->key, to_copy);
>>
>
> I'd kind of argue that just hardcoding 16 is fine here, keylen _looks_
> variable so the compiler can't optimize it away, but in reality keylen
> always must be 32 and sizeof() must be always 16 :)
It is not just the compiler to whom keylen _looks_ variable,
so to be safe I went with the min_t() check. I think this
also makes the code easier to understand for future readers of
the code.
> But it also really doesn't matter.
>
> Reviewed-by: Johannes Berg <[email protected]>
Thanks.
> Should we Cc stable on this?
Yes I think so. I did not add the tag because some subsystems
want that to be left up to the maintainer.
Regards,
Hans
On Tue, Apr 18, 2023 at 03:25:46PM +0200, Hans de Goede wrote:
> A received TKIP key may be up to 32 bytes because it may contain
> MIC rx/tx keys too. These are not used by iwl and copying these
> over overflows the iwl_keyinfo.key field.
>
> Add a check to not copy more data to iwl_keyinfo.key then will fit.
>
> This fixes backtraces like this one:
>
> 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)
> WARNING: CPU: 1 PID: 946 at drivers/net/wireless/intel/iwlwifi/dvm/sta.c:1103 iwlagn_send_sta_key+0x375/0x390 [iwldvm]
> <snip>
> Hardware name: Dell Inc. Latitude E6430/0H3MT5, BIOS A21 05/08/2017
> RIP: 0010:iwlagn_send_sta_key+0x375/0x390 [iwldvm]
> <snip>
> Call Trace:
> <TASK>
> iwl_set_dynamic_key+0x1f0/0x220 [iwldvm]
> iwlagn_mac_set_key+0x1e4/0x280 [iwldvm]
> drv_set_key+0xa4/0x1b0 [mac80211]
> ieee80211_key_enable_hw_accel+0xa8/0x2d0 [mac80211]
> ieee80211_key_replace+0x22d/0x8e0 [mac80211]
> <snip>
>
> Link: https://www.alionet.org/index.php?topic=1469.0
> Link: https://lore.kernel.org/linux-wireless/[email protected]/
> Link: https://lore.kernel.org/linux-wireless/[email protected]/
> Cc: Kees Cook <[email protected]>
> Suggested-by: Johannes Berg <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
Thanks for chasing this down -- I hadn't had time to come back around to
it.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook