On Thu, Aug 15, 2019 at 03:47:02PM +1200, richard.n.proc...@gmail.com wrote:
> On Thu, 15 Aug 2019, richard.n.proc...@gmail.com wrote:
> > > On Tue, Aug 13, 2019 at 10:47:24AM -0300, Martin Pieuchot wrote:
> > > > if the crash is because of a missing key, put one :)
> > >
> > > Yes, you've convinced me. With this patch we install the CCMP key
> > > to both firmware and net80211. Until firmware confirms that the
> > > key has been installed, we do software encrypt/decrypt.
> >
> > I agree we should handle a missing key but suggest an alternative approach
> > below.
>
> so:
>
> m0 = NULL;
> m_free(m0);
>
> isn't so useful; fixed patch follows.
Hmm... your patch is surprisingly simple. I like it :)
I am still a bit worried about iwm firmware failing to install the key,
which this patch would not handle. But that's a separate question.
Two comments inline:
> Index: net80211/ieee80211_crypto.c
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211_crypto.c,v
> retrieving revision 1.74
> diff -u -p -u -p -r1.74 ieee80211_crypto.c
> --- net80211/ieee80211_crypto.c 24 Sep 2018 20:14:59 -0000 1.74
> +++ net80211/ieee80211_crypto.c 15 Aug 2019 03:44:12 -0000
> @@ -157,6 +157,10 @@ ieee80211_set_key(struct ieee80211com *i
> /* should not get there */
> error = EINVAL;
> }
> +
> + if (error == 0)
> + k->k_flags |= IEEE80211_KEY_SWCRYPTO;
> +
> return error;
> }
>
> @@ -280,6 +284,11 @@ ieee80211_decrypt(struct ieee80211com *i
> }
> k = &ic->ic_nw_keys[kid];
> }
> +
> + if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
> + goto err;
> + }
Can you also add a corresponding panic() to ieee80211_encrypt()?
This would catch drivers calling ieee80211_encrypt() on the wrong key.
> +
> switch (k->k_cipher) {
> case IEEE80211_CIPHER_WEP40:
> case IEEE80211_CIPHER_WEP104:
> @@ -296,9 +305,15 @@ ieee80211_decrypt(struct ieee80211com *i
> break;
> default:
> /* key not defined */
> - m_freem(m0);
> - m0 = NULL;
> + goto err;
> }
> +
> + return m0;
> +
> +err:
> + m_freem(m0);
> + m0 = NULL;
> +
> return m0;
The above could be simplified to:
+ return m0;
+
+err:
+ m_freem(m0);
+ return NULL;
> Index: net80211/ieee80211_crypto.h
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211_crypto.h,v
> retrieving revision 1.25
> diff -u -p -u -p -r1.25 ieee80211_crypto.h
> --- net80211/ieee80211_crypto.h 18 Aug 2017 17:30:12 -0000 1.25
> +++ net80211/ieee80211_crypto.h 15 Aug 2019 03:44:12 -0000
> @@ -78,6 +78,7 @@ struct ieee80211_key {
> #define IEEE80211_KEY_GROUP 0x00000001 /* group data key */
> #define IEEE80211_KEY_TX 0x00000002 /* Tx+Rx */
> #define IEEE80211_KEY_IGTK 0x00000004 /* integrity group key */
> +#define IEEE80211_KEY_SWCRYPTO 0x00000080 /* loaded for software
> crypto */
>
> u_int k_len;
> u_int64_t k_rsc[IEEE80211_NUM_TID];
