On Tue, May 11, 2021 at 04:12:34PM +0200, Stefan Sperling wrote:
> kettenis@ noticed that an athn(4) node cache can contain bogus clients
> with MAC addreses that look like bit-flipped versions of MAC addresses
> of legitimate clients.
>
> I think this is due to my recent fix to allow block ack requests to
> pass through athn(4).
>
> The driver calls ieee80211_find_rxnode() on such frames.
> In hostap mode this function is responsible for creating new node cache
> entries in case the transmitter's address is not known yet. This results
> in bogus cache entries. This patch attempts to fix that issue by searching
> the transmitter address in the cache and dropping the frame if the address
> is unknown.
>
> In client mode ieee80211_find_rxnode() always returns ic->ic_bss.
> We can simply validate the transmitter address in the frame against
> the AP's MAC adress which is always known.
>
> In monitor mode we can pass such frames since they will be dropped
> anyway after bpf_mtap.
>
> Even if the address is correct other parts of the frame might still be
> corrupt. I don't think there is a way to verify the frame's checksum if
> the hardware cannot do it for us.
> I hope that net80211's input path is robust enough to deal with this...
>
> ok?
Currently testing on:
// Wi-Fi client to pce-0041
pce-0067# dmesg | grep athn
athn0 at pci4 dev 0 function 0 "Atheros AR9281" rev 0x01: apic 5 int 16
athn0: AR9280 rev 2 (2T2R), ROM rev 22, address 04:f0:21:45:6a:c2
// accesspoint
pce-0041# dmesg | grep athn
athn0 at pci4 dev 0 function 0 "Atheros AR9281" rev 0x01: apic 5 int 16
athn0: AR9280 rev 2 (2T2R), ROM rev 22, address 04:f0:21:34:e4:23
// accesspoint
pce-0035# dmesg | grep athn
athn0 at pci4 dev 0 function 0 "Atheros AR9281" rev 0x01: apic 5 int 16
athn0: AR9280 rev 2 (2T2R), ROM rev 22, address 04:f0:21:45:6a:c4
At the time of this mail, they have about 20+ minutes of uptime. No
noticable issues so far.
> diff 61810dc72eb09d7c410d5b2c7982a940951e4dd4 /usr/src
> blob - 816a3b09ee21111f59b2a22c4a2d236e39272b10
> file + sys/dev/ic/ar5008.c
> --- sys/dev/ic/ar5008.c
> +++ sys/dev/ic/ar5008.c
> @@ -970,29 +970,52 @@ ar5008_rx_process(struct athn_softc *sc, struct mbuf_l
> /* Finalize mbuf. */
> m->m_pkthdr.len = m->m_len = len;
>
> - /* Grab a reference to the source node. */
> wh = mtod(m, struct ieee80211_frame *);
> - ni = ieee80211_find_rxnode(ic, wh);
>
> if (michael_mic_failure) {
> /*
> * Check that it is not a control frame
> * (invalid MIC failures on valid ctl frames).
> + * Validate the transmitter's address to avoid passing
> + * corrupt frames with bogus addresses to net80211.
> */
> - if (!(wh->i_fc[0] & IEEE80211_FC0_TYPE_CTL) &&
> - (ic->ic_flags & IEEE80211_F_RSNON) &&
> - (ni->ni_rsncipher == IEEE80211_CIPHER_TKIP ||
> - ni->ni_rsngroupcipher == IEEE80211_CIPHER_TKIP)) {
> - /* Report Michael MIC failures to net80211. */
> - ic->ic_stats.is_rx_locmicfail++;
> - ieee80211_michael_mic_failure(ic, 0);
> + if ((wh->i_fc[0] & IEEE80211_FC0_TYPE_CTL)) {
> + switch (ic->ic_opmode) {
> +#ifndef IEEE80211_STA_ONLY
> + case IEEE80211_M_HOSTAP:
> + if (ieee80211_find_node(ic, wh->i_addr2))
> + michael_mic_failure = 0;
> + break;
> +#endif
> + case IEEE80211_M_STA:
> + if (IEEE80211_ADDR_EQ(wh->i_addr2,
> + ic->ic_bss->ni_macaddr))
> + michael_mic_failure = 0;
> + break;
> + case IEEE80211_M_MONITOR:
> + michael_mic_failure = 0;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (michael_mic_failure) {
> + /* Report Michael MIC failures to net80211. */
> + if ((ic->ic_rsnciphers & IEEE80211_CIPHER_TKIP) ||
> + ic->ic_rsngroupcipher == IEEE80211_CIPHER_TKIP) {
> + ic->ic_stats.is_rx_locmicfail++;
> + ieee80211_michael_mic_failure(ic, 0);
> + }
> ifp->if_ierrors++;
> - ieee80211_release_node(ic, ni);
> m_freem(m);
> goto skip;
> }
> }
>
> + /* Grab a reference to the source node. */
> + ni = ieee80211_find_rxnode(ic, wh);
> +
> /* Remove any HW padding after the 802.11 header. */
> if (!(wh->i_fc[0] & IEEE80211_FC0_TYPE_CTL)) {
> u_int hdrlen = ieee80211_get_hdrlen(wh);
>
--
Regards,
Mikolaj
