[PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[Kees Cook]

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(-)

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;
-- 
2.34.1

Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[Simon Horman]

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>

Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[Johannes Berg]

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

Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[Kees Cook]

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?

-- 
Kees Cook

Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[Johannes Berg]

> > 
> > >  	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

Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[Kees Cook]

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?

-- 
Kees Cook

Re: [PATCH] wifi: iwlwifi: dvm: Add struct_group for struct iwl_keyinfo keys

[Johannes Berg]

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