[PATCH] wifi: iwlwifi: dvm: Fix memcpy: detected field-spanning write backtrace

[PATCH] wifi: iwlwifi: dvm: Fix memcpy: detected field-spanning write backtrace

[Hans de Goede]

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/20230218191056.never.374-kees@kernel.org/
Link: https://lore.kernel.org/linux-wireless/68760035-7f75-1b23-e355-bfb758a87d83@redhat.com/
Cc: Kees Cook <keescook@chromium.org>
Suggested-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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

Re: [PATCH] wifi: iwlwifi: dvm: Fix memcpy: detected field-spanning write backtrace

[Johannes Berg]

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 <johannes@sipsolutions.net>


Should we Cc stable on this?

johannes

Re: [PATCH] wifi: iwlwifi: dvm: Fix memcpy: detected field-spanning write backtrace

[Hans de Goede]

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 <johannes@sipsolutions.net>

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

Re: [PATCH] wifi: iwlwifi: dvm: Fix memcpy: detected field-spanning write backtrace

[Kees Cook]

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/20230218191056.never.374-kees@kernel.org/
> Link: https://lore.kernel.org/linux-wireless/68760035-7f75-1b23-e355-bfb758a87d83@redhat.com/
> Cc: Kees Cook <keescook@chromium.org>
> Suggested-by: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Thanks for chasing this down -- I hadn't had time to come back around to
it.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook