Re: iwm(4): fix ccmp decrypt edge cases

2019-08-16 Thread Stefan Sperling 
On Fri, Aug 16, 2019 at 10:11:55PM +1200, richard.n.proc...@gmail.com wrote:
> See below for updated diff. Nice idea to add a panic on encrypt. 
> Also I've followed the existing idiom of m_freem(m0); return NULL; 
> 
> I've checked that all assignments to k_flags are to fresh keys; the new 
> flag is never clobbered.
> 
> The new check will be accounted against is_rx_wepfail ("input wep/wpa 
> packets failed").
> 
> Lightly tested with a download and ifconfig up/down. 
> 
> ok?

Yes, ok.

I would suggest to change the panic message to say something like
"%s: key not loaded for software crypto", but it's no big deal.

Thanks!

> 
> best, 
> Richard. 
> 
> 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 -  1.74
> +++ net80211/ieee80211_crypto.c   16 Aug 2019 09:50:47 -
> @@ -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;
>  }
>  
> @@ -209,6 +213,9 @@ struct mbuf *
>  ieee80211_encrypt(struct ieee80211com *ic, struct mbuf *m0,
>  struct ieee80211_key *k)
>  {
> + if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0)
> + panic("%s: unset key %d", __func__, k->k_id);
> +
>   switch (k->k_cipher) {
>   case IEEE80211_CIPHER_WEP40:
>   case IEEE80211_CIPHER_WEP104:
> @@ -280,6 +287,12 @@ ieee80211_decrypt(struct ieee80211com *i
>   }
>   k = >ic_nw_keys[kid];
>   }
> +
> + if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
> + m_free(m0);
> + return NULL;
> + }
> +
>   switch (k->k_cipher) {
>   case IEEE80211_CIPHER_WEP40:
>   case IEEE80211_CIPHER_WEP104:
> 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 -  1.25
> +++ net80211/ieee80211_crypto.h   16 Aug 2019 09:50:47 -
> @@ -78,6 +78,7 @@ struct ieee80211_key {
>  #define IEEE80211_KEY_GROUP  0x0001  /* group data key */
>  #define IEEE80211_KEY_TX 0x0002  /* Tx+Rx */
>  #define IEEE80211_KEY_IGTK   0x0004  /* integrity group key */
> +#define IEEE80211_KEY_SWCRYPTO   0x0080  /* loaded for software 
> crypto */
>  
>   u_int   k_len;
>   u_int64_t   k_rsc[IEEE80211_NUM_TID];
>

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-16 Thread richard . n . procter 


On Thu, 15 Aug 2019, Stefan Sperling wrote:
> On Thu, Aug 15, 2019 at 03:47:02PM +1200, richard.n.proc...@gmail.com wrote:
> > > I agree we should handle a missing key but suggest an alternative 
> > > approach 
> > > below.
> 
> 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.

See below for updated diff. Nice idea to add a panic on encrypt. 
Also I've followed the existing idiom of m_freem(m0); return NULL; 

I've checked that all assignments to k_flags are to fresh keys; the new 
flag is never clobbered.

The new check will be accounted against is_rx_wepfail ("input wep/wpa 
packets failed").

Lightly tested with a download and ifconfig up/down. 

ok?

best, 
Richard. 

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 -  1.74
+++ net80211/ieee80211_crypto.c 16 Aug 2019 09:50:47 -
@@ -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;
 }
 
@@ -209,6 +213,9 @@ struct mbuf *
 ieee80211_encrypt(struct ieee80211com *ic, struct mbuf *m0,
 struct ieee80211_key *k)
 {
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0)
+   panic("%s: unset key %d", __func__, k->k_id);
+
switch (k->k_cipher) {
case IEEE80211_CIPHER_WEP40:
case IEEE80211_CIPHER_WEP104:
@@ -280,6 +287,12 @@ ieee80211_decrypt(struct ieee80211com *i
}
k = >ic_nw_keys[kid];
}
+
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
+   m_free(m0);
+   return NULL;
+   }
+
switch (k->k_cipher) {
case IEEE80211_CIPHER_WEP40:
case IEEE80211_CIPHER_WEP104:
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 -  1.25
+++ net80211/ieee80211_crypto.h 16 Aug 2019 09:50:47 -
@@ -78,6 +78,7 @@ struct ieee80211_key {
 #define IEEE80211_KEY_GROUP0x0001  /* group data key */
 #define IEEE80211_KEY_TX   0x0002  /* Tx+Rx */
 #define IEEE80211_KEY_IGTK 0x0004  /* integrity group key */
+#define IEEE80211_KEY_SWCRYPTO 0x0080  /* loaded for software crypto */
 
u_int   k_len;
u_int64_t   k_rsc[IEEE80211_NUM_TID];

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-15 Thread Stefan Sperling 
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 -  1.74
> +++ net80211/ieee80211_crypto.c   15 Aug 2019 03:44:12 -
> @@ -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_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 -  1.25
> +++ net80211/ieee80211_crypto.h   15 Aug 2019 03:44:12 -
> @@ -78,6 +78,7 @@ struct ieee80211_key {
>  #define IEEE80211_KEY_GROUP  0x0001  /* group data key */
>  #define IEEE80211_KEY_TX 0x0002  /* Tx+Rx */
>  #define IEEE80211_KEY_IGTK   0x0004  /* integrity group key */
> +#define IEEE80211_KEY_SWCRYPTO   0x0080  /* loaded for software 
> crypto */
>  
>   u_int   k_len;
>   u_int64_t   k_rsc[IEEE80211_NUM_TID];

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-14 Thread richard . n . procter 



On Thu, 15 Aug 2019, richard.n.proc...@gmail.com wrote:

> Hi Stefan, 
> 
> On Wed, 14 Aug 2019, Stefan Sperling 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.

best, 
Richard. 

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 -  1.74
+++ net80211/ieee80211_crypto.c 15 Aug 2019 03:44:12 -
@@ -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_nw_keys[kid];
}
+
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
+   goto err;
+   }
+
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;
 }
 
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 -  1.25
+++ net80211/ieee80211_crypto.h 15 Aug 2019 03:44:12 -
@@ -78,6 +78,7 @@ struct ieee80211_key {
 #define IEEE80211_KEY_GROUP0x0001  /* group data key */
 #define IEEE80211_KEY_TX   0x0002  /* Tx+Rx */
 #define IEEE80211_KEY_IGTK 0x0004  /* integrity group key */
+#define IEEE80211_KEY_SWCRYPTO 0x0080  /* loaded for software crypto */
 
u_int   k_len;
u_int64_t   k_rsc[IEEE80211_NUM_TID];



> > 
> > Should the firmware fail to install the key for some reason, we will
> > just keep using our software fallback until the interface is reset.
> > 
> > This also fixes the race with incoming frames during WPA handshake,
> > i.e. while (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0.
> > Such frames can be passed to net80211 and will be dropped there.
> > 
> > Lightly tested on a 7260 device.
> > 
> > Ok?
> > 
> > diff refs/heads/master refs/heads/ccmp-crash2
> > blob - 038e6a63dfff113b525bb1e9a30c935996535569
> > blob + 8afcce065eb0d85f4469a3f378cacba1513bc215
> > --- sys/dev/pci/if_iwm.c
> > +++ sys/dev/pci/if_iwm.c
> > @@ -3595,10 +3595,17 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct 
> > iwm_rx_pac
> > rxi.rxi_rssi = rssi;
> > rxi.rxi_tstamp = device_timestamp;
> >  
> > -   /* Handle hardware decryption. */
> > +   /*
> > +* Handle hardware decryption.
> > +*
> > +* If the WPA handshake has completed but firmware is not yet ready
> > +* to decrypt frames, fall back to software decryption in net80211. 
> > +* net80211 will drop encrypted frames until the handshake completes.
> > +*/
> > if (((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) != IEEE80211_FC0_TYPE_CTL)
> > && (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) &&
> > !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
> > +   (sc->sc_flags & IWM_FLAG_CCMP_READY) &&
> > (ni->ni_flags & IEEE80211_NODE_RXPROT) &&
> > ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) {
> > if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) !=
> > @@ -4402,7 +4409,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie
> >  
> > if (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) {
> > k = ieee80211_get_txkey(ic, wh, ni);
> > -   if  (k->k_cipher != IEEE80211_CIPHER_CCMP) {
> > +   if  (k->k_cipher != IEEE80211_CIPHER_CCMP ||
> > +   (sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) {
> > if ((m = ieee80211_encrypt(ic, m, k)) == NULL)
> > return ENOBUFS;
> >  
> > @@ -4418,7 +4426,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie
> > /* Copy 802.11 header in TX command. */
> > memcpy(((uint8_t *)tx) + sizeof(*tx), wh, hdrlen);
> >  
> > -   if  (k != NULL && k->k_cipher

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-14 Thread richard . n . procter 
Hi Stefan, 

On Wed, 14 Aug 2019, Stefan Sperling 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.

I'm unconvinced the root cause is a race on loading the key: I tried to 
induce an infinite race, by skipping the command to upload the key to 
hardware, but couldn't reproduce the crash. I suspect the guard for 
handling hardware decryption in iwm_rx_rx_mpdu() is too weak, and a frame 
governed by the offloaded key is slipping through.

The immediate cause of the crash is that, unless the driver sets 
IEEE80211_RXI_HWDEC on all frames governed by a key, the stack will 
attempt software decryption without the necessary context and crash. i.e. 
software decryption is the default and the stack assumes this is always 
possible. But it isn't: the stack has delegated decryption to the driver.

So, evade this class of errors and check that software decrypt can handle 
a key, i.e that ieee8021_set_key() has been called, and drop the frame if 
not. (see below; compiled but untested.)

Thoughts? 

best, 
Richard. 

iwm0 at pci1 dev 0 function 0 "Intel Dual Band Wireless AC 7265" rev 0x69, msi
iwm0: hw rev 0x210, fw ver 16.242414.0, address xx:xx:xx:xx:xx:xx 

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 -  1.74
+++ net80211/ieee80211_crypto.c 15 Aug 2019 01:12:28 -
@@ -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_nw_keys[kid];
}
+
+   if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) {
+   goto err;
+   }
+
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:
+   m0 = NULL;
+   m_freem(m0);
+
return m0;
 }
 
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 -  1.25
+++ net80211/ieee80211_crypto.h 15 Aug 2019 01:12:28 -
@@ -78,6 +78,7 @@ struct ieee80211_key {
 #define IEEE80211_KEY_GROUP0x0001  /* group data key */
 #define IEEE80211_KEY_TX   0x0002  /* Tx+Rx */
 #define IEEE80211_KEY_IGTK 0x0004  /* integrity group key */
+#define IEEE80211_KEY_SWCRYPTO 0x0080  /* loaded for software crypto */
 
u_int   k_len;
u_int64_t   k_rsc[IEEE80211_NUM_TID];

---
illustration of disabling key upload below here
---


Index: dev/pci/if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.244
diff -u -p -u -p -r1.244 if_iwm.c
--- dev/pci/if_iwm.c8 Aug 2019 13:56:56 -   1.244
+++ dev/pci/if_iwm.c14 Aug 2019 20:32:47 -
@@ -6088,7 +6088,7 @@ int
 iwm_set_key(struct ieee80211com *ic, struct ieee80211_node *ni,
 struct ieee80211_key *k)
 {
-   struct iwm_softc *sc = ic->ic_softc;
+   // struct iwm_softc *sc = ic->ic_softc;
struct iwm_add_sta_key_cmd cmd = { 0 };
 
if ((k->k_flags & IEEE80211_KEY_GROUP) ||
@@ -6108,8 +6108,10 @@ iwm_set_key(struct ieee80211com *ic, str
cmd.key_offset = 0;
cmd.sta_id = IWM_STATION_ID;
 
-   return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
-   sizeof(cmd), );
+   // return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
+   //sizeof(cmd), );
+
+   return 0;
 }
 
 void


> 
> Should the firmware fail to install the key for some reason, we will
> just keep using our software fallback until the interface is reset.
> 
> This also fixes the race with incoming frames during WPA handshake,
> i.e. while (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0.
> Such frames can

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-14 Thread Stefan Sperling 
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.

Should the firmware fail to install the key for some reason, we will
just keep using our software fallback until the interface is reset.

This also fixes the race with incoming frames during WPA handshake,
i.e. while (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0.
Such frames can be passed to net80211 and will be dropped there.

Lightly tested on a 7260 device.

Ok?

diff refs/heads/master refs/heads/ccmp-crash2
blob - 038e6a63dfff113b525bb1e9a30c935996535569
blob + 8afcce065eb0d85f4469a3f378cacba1513bc215
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -3595,10 +3595,17 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
rxi.rxi_rssi = rssi;
rxi.rxi_tstamp = device_timestamp;
 
-   /* Handle hardware decryption. */
+   /*
+* Handle hardware decryption.
+*
+* If the WPA handshake has completed but firmware is not yet ready
+* to decrypt frames, fall back to software decryption in net80211. 
+* net80211 will drop encrypted frames until the handshake completes.
+*/
if (((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) != IEEE80211_FC0_TYPE_CTL)
&& (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) &&
!IEEE80211_IS_MULTICAST(wh->i_addr1) &&
+   (sc->sc_flags & IWM_FLAG_CCMP_READY) &&
(ni->ni_flags & IEEE80211_NODE_RXPROT) &&
ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) {
if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) !=
@@ -4402,7 +4409,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie
 
if (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) {
k = ieee80211_get_txkey(ic, wh, ni);
-   if  (k->k_cipher != IEEE80211_CIPHER_CCMP) {
+   if  (k->k_cipher != IEEE80211_CIPHER_CCMP ||
+   (sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) {
if ((m = ieee80211_encrypt(ic, m, k)) == NULL)
return ENOBUFS;
 
@@ -4418,7 +4426,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie
/* Copy 802.11 header in TX command. */
memcpy(((uint8_t *)tx) + sizeof(*tx), wh, hdrlen);
 
-   if  (k != NULL && k->k_cipher == IEEE80211_CIPHER_CCMP) {
+   if  (k != NULL && k->k_cipher == IEEE80211_CIPHER_CCMP &&
+   (sc->sc_flags & IWM_FLAG_CCMP_READY)) {
/* Trim 802.11 header and prepend CCMP IV. */
m_adj(m, hdrlen - IEEE80211_CCMP_HDRLEN);
ivp = mtod(m, u_int8_t *);
@@ -6090,6 +6099,7 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
 {
struct iwm_softc *sc = ic->ic_softc;
struct iwm_add_sta_key_cmd cmd = { 0 };
+   int err;
 
if ((k->k_flags & IEEE80211_KEY_GROUP) ||
k->k_cipher != IEEE80211_CIPHER_CCMP)  {
@@ -6097,6 +6107,8 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
return (ieee80211_set_key(ic, ni, k));
}
 
+   sc->sc_flags &= ~(IWM_FLAG_CCMP_READY | IWM_FLAG_CCMP_KEY_SENT);
+
cmd.key_flags = htole16(IWM_STA_KEY_FLG_CCM |
IWM_STA_KEY_FLG_WEP_KEY_MAP |
((k->k_id << IWM_STA_KEY_FLG_KEYID_POS) &
@@ -6108,8 +6120,15 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
cmd.key_offset = 0;
cmd.sta_id = IWM_STATION_ID;
 
-   return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
-   sizeof(cmd), );
+   /* Allow for software CCMP encryption/decryption fallback. */
+   err = ieee80211_set_key(ic, ni, k);
+   if (!err) {
+   err = iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
+   sizeof(cmd), );
+   if (!err)
+   sc->sc_flags |= IWM_FLAG_CCMP_KEY_SENT;
+   }
+   return err;
 }
 
 void
@@ -6135,6 +6154,10 @@ iwm_delete_key(struct ieee80211com *ic, struct ieee802
cmd.sta_id = IWM_STATION_ID;
 
iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC, sizeof(cmd), );
+   sc->sc_flags &= ~(IWM_FLAG_CCMP_KEY_SENT | IWM_FLAG_CCMP_READY);
+
+   /* Delete software fallback CCMP key. */
+ieee80211_delete_key(ic, ni, k);
 }
 
 void
@@ -6846,6 +6869,7 @@ iwm_stop(struct ifnet *ifp)
sc->sc_flags &= ~IWM_FLAG_TE_ACTIVE;
sc->sc_flags &= ~IWM_FLAG_HW_ERR;
sc->sc_flags &= ~IWM_FLAG_SHUTDOWN;
+   sc->sc_flags &= ~(IWM_FLAG_CCMP_KEY_SENT | IWM_FLAG_CCMP_READY);
 
sc->sc_newstate(ic, IEEE80211_S_INIT, -1);
 
@@ -7331,6 +7355,12 @@ iwm_notif_intr(struct iwm_softc *sc)
break;
 
case IWM_ADD_STA_KEY:
+   if (sc->sc_flags & IWM_FLAG_CCMP_KEY_SENT) {
+

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-13 Thread Jesper Wallin 
Hi, (cc'ed to bugs@ as well)

On Tue, Aug 13, 2019 at 02:58:05PM +0200, Stefan Sperling wrote:
> On Tue, Aug 13, 2019 at 09:40:22AM -0300, Martin Pieuchot wrote:
> > 
> > How does the stack crashes?
> 
> Jesper only sent me a screen shot and no public bug report :(
>

I've had a really busy week and you asked me to transcribe the
screenshot, which took a extra time since I couldn't use OCR.  I've also
tried to reproduce it and running solely on wireless the last couple of
days, but sadly no luck.  I ran chrome, rtorrent and was starting the
tor-browser when the crash happened, but I doubt that really matters.

I now noticed the trace was in /var/log/messages, *after* I copied it
manually. Oh well... :-)

I also apologize for the noise and the column length of this mail, I
just want to provide as much as possible and copied it as-is.  The stack
trace below is written by hand, so typos might occur. Doublecheck the
screenshot (see below).

--8<--
restore_fbdev_mode_atomic(80862e00,1) at restore_fbdev-mode_atomic+0x1ac
drm_fb_helper_restore_fbdev_mode_unlocked(80862e00) at 
drm_fb_helper_restore_fbdev_mode_unlocked+0x76
intel_fbdev_restore_mode(80147078) at intel_fbdev_restore_mode+0x33
db_ktrap(6,0,80002227b4a0) at db_ktrap+0x2c
kerntrap(80002227b4a0) at kerntrap+0xa2
alltraps_kern_meltdown(6,0,80002226b760,0,0,1) at 
alltraps_kern_meltdown+0x7b
AES_Encrypt_ECB(0,80002227b760,80002227b760,1) at AES_Encrypt_ECB+0xd6
ieee80211_ccmp_phase1(0,fd800239700c,53,5e4,80002227b760,80002227b700)
 at ieee80211_ccmp_phase1+0x1fc
ieee80211_ccmp_decrypt(80183048,fd808ce4ee00,80dba1a0) at 
ieee80211_ccmp_decrypt+0x238
ieee80211_input(80183048,fd808ce4ee00,80dba000,81368038)
 at ieee80211_input+0xa1f
ieee80211_ba_move_window(80183048,80dba000,0,9d) at 
ieee80211_ba_move_window+0xb7
ieee80211_input(80183048,fd8095616800,80dba000,80002227baa0)
 at ieee80211_input+0x71d
iwm_rx_rx_mpdu(80183000,fd800238f000,801e3d00) at 
iwm_rx_rx_mpdu+0x421
iwm_notif_intr(80183000) at iwm_notif_intr+0x67b
iwm_intr(80183000) at iwm_intr+0x389
intr_handler(80002227bc50,80143180) at intr_handler+0x6e
Xintr_ioapic_edge22_untramp(0,0,1388,0,0,81d3c6f8) at 
Xintr_ioapic_edge22_untramp+0x19f
acpicpu_idle() at acpicpu_idle+0x1d2
sched_idle(81d3bff0) at sched_idle+0x225
end trace frame: 0x0, count: 231
End of stack trace.
splassert: pool_do_put: want 0 have 7
Starting stack trace...
pool_do_put(81de50d0,fd81f6054f20) at pool_do_put+0x40
pool_put(81de50d0,fd81f6054f20) at pool_put+0x5a
futex_wait(3e157caf160,c8,3e168a70960,2) at futex_wait+0x1fe
sys_futex(800044028628,800044767590,8000447675f0) at sys_futex+0x94
syscall(800044767660) at syscall+0x389
Xsyscall(0,53,16,53,3,3e1b03d0400) at Xsyscall+0x128
end of kernel
end trace frame: 0x3e168a709d0, count: 251
End of stack trace.
splassert: assertwaitok: want 0 have 7
Starting stack trace...
assertwaitok() at assertwaitok+0x40
mi_switch() at mi_switch+0x40
preempt() at preempt+0x5c
ast(800044767660) at ast+0xb3
Xsyscall(0,3c,16,53,3,3e1b03d0400) at Xsyscall+0x156
end of kernel
end trace frame: 0x3e168a709d0, count: 252
End of stack trace.
kernel: page fault trap, code=0
Stopped at  AES_Encrypt_ECB+0xd6:movl0x2d0(%rax),%edi
ddb{0}> 
--8<--

The above mentioned screenshots can also be found at:
https://ifconfig.se/IMG_5204.jpg
https://ifconfig.se/IMG_5205.jpg
https://ifconfig.se/IMG_5206.jpg
https://ifconfig.se/IMG_5207.jpg
https://ifconfig.se/IMG_5208.jpg
https://ifconfig.se/IMG_5209.jpg
https://ifconfig.se/IMG_5210.jpg
https://ifconfig.se/IMG_5211.jpg
https://ifconfig.se/IMG_5212.jpg
https://ifconfig.se/IMG_5213.jpg
https://ifconfig.se/IMG_5214.jpg
https://ifconfig.se/IMG_5215.jpg
https://ifconfig.se/IMG_5216.jpg


I sadly don't have a dmesg from the day, but tried to extract as much as
I could from /var/log/messages:

--8<--
OpenBSD 6.5-current (GENERIC.MP) #186: Thu Aug  8 21:35:57 MDT 2019
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 8261529600 (7878MB)
avail mem = 8000983040 (7630MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xacbfd000 (65 entries)
bios0: vendor LENOVO version "N14ET50W (1.28 )" date 03/21/2019
bios0: LENOVO 20BS00A5MS
acpi0 at bios0: ACPI 5.0
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SLIC ASF! HPET ECDT APIC MCFG SSDT SSDT SSDT SSDT SSDT 
SSDT SSDT SSDT SSDT PCCT SSDT UEFI MSDM BATB FPDT UEFI DMAR
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP2(S4) XHCI(S3) EHC1(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpihpet0 at acpi0: 14318179 Hz
acpiec0 at acpi0
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-5500U CPU @ 2.40GHz, 2295.13 MHz,

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-13 Thread Martin Pieuchot 
On 13/08/19(Tue) 14:58, Stefan Sperling wrote:
> On Tue, Aug 13, 2019 at 09:40:22AM -0300, Martin Pieuchot wrote:
> > On 13/08/19(Tue) 13:52, Stefan Sperling wrote:
> > > This should hopefully prevent a crash reported to me by Jesper Wallin,
> > > where net80211 crashes when it attempts to decrypt a CCMP-encrypted
> > > frame which iwm passed up without decrypting it first.
> > 
> > How does the stack crashes?
> 
> Jesper only sent me a screen shot and no public bug report :(
> 
> The trace looks roughly like this, where AES_Encrypt_ECB crashes because
> ieee80211_ccmp_set_key() is never called since iwm_set_key overrides it.

Could we call ieee80211_ccmp_set_key() with a default value when
initializing the stack then?  Would that prevent all the possible family
of bugs?

What problems can occur when passing encrypted frames to the stack without
having configure a CCMP key?  Are they packets going to be dropped at some
point?

> AES_Encrypt_ECB()
> ieee80211_ccmp_phase1()
> ieee80211_ccmp_decrypt()
> ieee80211_input()
> ieee80211_ba_move_window()
> ieee80211_input()
> iwm_rx_rx_mpdu()
> iwm_notif_intr()
> iwm_intr()
> 
> What's a bit confusing here is that the interrupt was for an uencrypted
> frame (a Block Ack Request) which moved the BA window forward. When
> this happens we flush frames currently waiting in the Rx BA reordering
> buffer. The encrypted frame which caused the crash was on that queue;
> It must have passed through iwm_rx_rx_mpdu() earlier before the WPA
> handshake was complete, and ended up waiting in the Rx BA queue because
> its sequence number was off.

> > Shouldn't we drop this packet in the stack as well?
> 
> net80211 will always see a 'protected' bit in the frame header
> In ieee80211_input, if rxi has an HW_DEC flag set, the code assumes
> this frame needs no further processing for crypto.
> Otherwise it tries to decrypt.
> 
> So if the driver passes up a frame without decrypting it and without
> setting HW_DEC, we crash. There's nothing that would allow net80211
> to detect an incorrect absence of HW_DEC. The driver overrides the
> set_key function and corresponding frames are expected to arrive with
> the HW_DEC flag set in net80211. (Please don't ask me to change this
> design right now for a better fix, it wasn't my idea...)

See above, if the crash is because of a missing key, put one :)

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-13 Thread Stefan Sperling 
On Tue, Aug 13, 2019 at 09:40:22AM -0300, Martin Pieuchot wrote:
> On 13/08/19(Tue) 13:52, Stefan Sperling wrote:
> > This should hopefully prevent a crash reported to me by Jesper Wallin,
> > where net80211 crashes when it attempts to decrypt a CCMP-encrypted
> > frame which iwm passed up without decrypting it first.
> 
> How does the stack crashes?

Jesper only sent me a screen shot and no public bug report :(

The trace looks roughly like this, where AES_Encrypt_ECB crashes because
ieee80211_ccmp_set_key() is never called since iwm_set_key overrides it.

AES_Encrypt_ECB()
ieee80211_ccmp_phase1()
ieee80211_ccmp_decrypt()
ieee80211_input()
ieee80211_ba_move_window()
ieee80211_input()
iwm_rx_rx_mpdu()
iwm_notif_intr()
iwm_intr()

What's a bit confusing here is that the interrupt was for an uencrypted
frame (a Block Ack Request) which moved the BA window forward. When
this happens we flush frames currently waiting in the Rx BA reordering
buffer. The encrypted frame which caused the crash was on that queue;
It must have passed through iwm_rx_rx_mpdu() earlier before the WPA
handshake was complete, and ended up waiting in the Rx BA queue because
its sequence number was off.

> Shouldn't we drop this packet in the stack as well?

net80211 will always see a 'protected' bit in the frame header
In ieee80211_input, if rxi has an HW_DEC flag set, the code assumes
this frame needs no further processing for crypto.
Otherwise it tries to decrypt.

So if the driver passes up a frame without decrypting it and without
setting HW_DEC, we crash. There's nothing that would allow net80211
to detect an incorrect absence of HW_DEC. The driver overrides the
set_key function and corresponding frames are expected to arrive with
the HW_DEC flag set in net80211. (Please don't ask me to change this
design right now for a better fix, it wasn't my idea...)

> > By code inspection I have determined that this problem could happen
> > in case a CCMP frame is received peer before the WPA handshake has
> > completed (e.g. during re-association). Note how the hardware decryption
> > code path depends on IEEE80211_NODE_RXPROT being set, which is set only
> > once the handshake has been completed.
> > We have to be careful here to avoid breaking non-CCMP ciphers (WEP, TKIP). 
> 
> So it's like working around the firwmare by defining two new state
> related to the CCMP key and dropping packets that should not reach
> us at this point?

Yes.

> > It also attempts to close a small race where we're handing the key to
> > firmware and eventually get an interrupt which confirms key installation,
> > in parallel to sending/receiving unicast data frames.
> > Until the key has been confirmed by firmware, don't transmit data frames
> > and drop received encrypted frames.
> 
> Makes sense.
> 
> The actual solution also has a race if you change the key before
> acknowledging the firmware IWM_ADD_STA_KEY, but I suppose this is
> acceptable.

The stack-side key has already been changed at that point. It is derived
from the WPA handshake. The firmware has to accept it or we can't proceed.

> > This part of the diff has poor error handling; if firmware never confirms
> > they key then iwm will get stuck and the user will have to recover.
> 
> Well up/down should work, right?

Yes it should.

> > But that should be better than the current situation where we'd be
> > sending unencrypted frames or perhaps crash in net80211.
> > 
> > These fixes could be separated but a combined diff is easier to test.
> 
> What should we test?

Just check for regressions.

I have lightly tested it on one machine and it works there.
There's not a lot of testing needed, but if a few people could run the
diff to check if I've missed something, that would be nice.

Re: iwm(4): fix ccmp decrypt edge cases

2019-08-13 Thread Martin Pieuchot 
On 13/08/19(Tue) 13:52, Stefan Sperling wrote:
> This should hopefully prevent a crash reported to me by Jesper Wallin,
> where net80211 crashes when it attempts to decrypt a CCMP-encrypted
> frame which iwm passed up without decrypting it first.

How does the stack crashes?  Shouldn't we drop this packet in the stack
as well?

> By code inspection I have determined that this problem could happen
> in case a CCMP frame is received peer before the WPA handshake has
> completed (e.g. during re-association). Note how the hardware decryption
> code path depends on IEEE80211_NODE_RXPROT being set, which is set only
> once the handshake has been completed.
> We have to be careful here to avoid breaking non-CCMP ciphers (WEP, TKIP). 

So it's like working around the firwmare by defining two new state
related to the CCMP key and dropping packets that should not reach
us at this point?

> It also attempts to close a small race where we're handing the key to
> firmware and eventually get an interrupt which confirms key installation,
> in parallel to sending/receiving unicast data frames.
> Until the key has been confirmed by firmware, don't transmit data frames
> and drop received encrypted frames.

Makes sense.

The actual solution also has a race if you change the key before
acknowledging the firmware IWM_ADD_STA_KEY, but I suppose this is
acceptable.

> This part of the diff has poor error handling; if firmware never confirms
> they key then iwm will get stuck and the user will have to recover.

Well up/down should work, right?

> But that should be better than the current situation where we'd be
> sending unencrypted frames or perhaps crash in net80211.
> 
> These fixes could be separated but a combined diff is easier to test.

What should we test?

> Once we've got this working for iwm(4), the iwn(4) driver will need
> a similar fix.
> 
> diff refs/heads/master refs/heads/ccmp-crash
> blob - 038e6a63dfff113b525bb1e9a30c935996535569
> blob + 0571a3a042c0097002241e2accc026c55af205ed
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -3601,6 +3601,13 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
>   !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
>   (ni->ni_flags & IEEE80211_NODE_RXPROT) &&
>   ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) {
> + if ((sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) {
> + /* Firmware has not installed the key yet. */
> + ic->ic_stats.is_ccmp_dec_errs++;
> + ifp->if_ierrors++;
> + m_freem(m);
> + goto done;
> + }
>   if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) !=
>   IWM_RX_MPDU_RES_STATUS_SEC_CCM_ENC) {
>   ic->ic_stats.is_ccmp_dec_errs++;
> @@ -3625,6 +3632,22 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
>   goto done;
>   }
>   rxi.rxi_flags |= IEEE80211_RXI_HWDEC;
> + } else if ((wh->i_fc[1] & IEEE80211_FC1_PROTECTED) &&
> + !IEEE80211_IS_MULTICAST(wh->i_addr1) &&

Bikesheding: these 5 hardware decryption checks are executed for
non-multicast and if the FC1_PROTECTED bit is set right?  Should
that be a single if statement?

> + (ic->ic_flags & IEEE80211_F_RSNON) &&
> + (ic->ic_rsnprotos & IEEE80211_PROTO_RSN) &&
> + (ic->ic_rsnciphers & IEEE80211_CIPHER_CCMP) &&
> + (ni->ni_rsnciphers & IEEE80211_CIPHER_CCMP) &&
> + (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0) {
> + /*
> +  * We're doing CCMP and have received an encrypted unicast
> +  * frame before 4-way handshake has completed. Don't pass it
> +  * up the stack; net80211 would crash trying to decrypt it.
> +  */
> + ic->ic_stats.is_ccmp_dec_errs++;
> + ifp->if_ierrors++;
> + m_freem(m);
> + goto done;
>   }
>  
>  #if NBPFILTER > 0
> @@ -6090,6 +6113,7 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
>  {
>   struct iwm_softc *sc = ic->ic_softc;
>   struct iwm_add_sta_key_cmd cmd = { 0 };
> + int err;
>  
>   if ((k->k_flags & IEEE80211_KEY_GROUP) ||
>   k->k_cipher != IEEE80211_CIPHER_CCMP)  {
> @@ -6097,6 +6121,8 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
>   return (ieee80211_set_key(ic, ni, k));
>   }
>  
> + sc->sc_flags &= ~IWM_FLAG_CCMP_READY;
> +
>   cmd.key_flags = htole16(IWM_STA_KEY_FLG_CCM |
>   IWM_STA_KEY_FLG_WEP_KEY_MAP |
>   ((k->k_id << IWM_STA_KEY_FLG_KEYID_POS) &
> @@ -6108,8 +6134,11 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
>   cmd.key_offset = 0;
>   cmd.sta_id = IWM_STATION_ID;
>  
> - return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
> + err = iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC,
>   sizeof(cmd), );
> + if