| Message ID | 20230218191056.never.374-kees@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Miri Korenblit |
| Headers | show |
| Series | wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys | expand |
On Sat, Feb 18, 2023 at 11:11:00AM -0800, Kees Cook wrote: > 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 <gregory.greenman@intel.com> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Johannes Berg <johannes.berg@intel.com> > Cc: Benjamin Berg <benjamin.berg@intel.com> > Cc: Sriram R <quic_srirrama@quicinc.com> > Cc: linux-wireless@vger.kernel.org > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Sat, 2023-02-18 at 11:11 -0800, Kees Cook wrote: > > 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. > 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? johannes
On Sun, Feb 19, 2023 at 04:12:05PM +0100, Johannes Berg wrote: > On Sat, 2023-02-18 at 11:11 -0800, Kees Cook wrote: > > > > 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. > > > 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? 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. Perhaps this patch can be taken as-is, and a WARN_ON added for the >16 case to be tracked down separately?
> > > > > 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? > > 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). johannes
On Mon, Mar 20, 2023 at 07:34:59PM +0100, Johannes Berg wrote: > > > > > > > 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? > > > > 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?
On Mon, 2023-03-20 at 12:52 -0700, Kees Cook wrote: > > 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 40 bits is only 5 bytes :-) > and should it silently ignore larger values in each case? > For the cases other than TKIP, no changes should be necessary - in those cases, the key will always be == the value from the (corrected) table above. For TKIP, the keylen will be 32, but comprised of the actual encryption key and then MIC keys, so only 16 bytes should be relevant. I don't really care how you handle this. We can be bug-compatible with the old code, then I'd say from your patch only the changes in the TKIP case are needed. Or we can just limit the copy to only 16 bytes, but that would need some validation that it still works. From memory, I'd say it will still work, and I'd even say the whole memcpy() might not be needed in the TKIP case at all since the iwldvm firmware deals with phase 1 keys (P1K) to derive phase 2 keys, not the original encryption key. So simpler is probably to just take the TKIP hunk from your patch, even knowing that the memcpy() there is almost certainly incorrect now. johannes
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/commands.h b/drivers/net/wireless/intel/iwlwifi/dvm/commands.h index 75a4b8e26232..0eceac4b9131 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/commands.h +++ b/drivers/net/wireless/intel/iwlwifi/dvm/commands.h @@ -783,10 +783,12 @@ struct iwl_keyinfo { __le16 tkip_rx_ttak[5]; /* 10-byte unicast TKIP TTAK */ u8 key_offset; u8 reserved2; - u8 key[16]; /* 16-byte unicast decryption key */ - __le64 tx_secur_seq_cnt; - __le64 hw_tkip_mic_rx_key; - __le64 hw_tkip_mic_tx_key; + struct_group(keys, + u8 key[16]; /* 16-byte unicast decryption key */ + __le64 tx_secur_seq_cnt; + __le64 hw_tkip_mic_rx_key; + __le64 hw_tkip_mic_tx_key; + ); } __packed; /** diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c index cef43cf80620..a1c9e201b058 100644 --- a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c +++ b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c @@ -1093,14 +1093,14 @@ static int iwlagn_send_sta_key(struct iwl_priv *priv, switch (keyconf->cipher) { 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); break; 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); break; case WLAN_CIPHER_SUITE_WEP104: key_flags |= STA_KEY_FLG_KEY_SIZE_MSK;
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 <gregory.greenman@intel.com> Cc: Kalle Valo <kvalo@kernel.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Johannes Berg <johannes.berg@intel.com> Cc: Benjamin Berg <benjamin.berg@intel.com> Cc: Sriram R <quic_srirrama@quicinc.com> Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/net/wireless/intel/iwlwifi/dvm/commands.h | 10 ++++++---- drivers/net/wireless/intel/iwlwifi/dvm/sta.c | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-)