Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default
From: Kalle Valo <hidden>
Date: 2018-03-14 14:24:04
Also in:
bridge, netdev
Rafa=C5=82 Mi=C5=82ecki [off-list ref] writes:
From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl> Testing brcmfmac with more recent firmwares resulted in AP interfaces not working in some specific setups. Debugging resulted in discovering support for IAPP in Broadcom's firmwares. This is an obsoleted standard and its implementation is something that: 1) Most people don't need / want to use 2) Can allow local DoS attacks 3) Breaks AP interfaces in some specific bridge setups To solve issues it can cause this commit modifies brcmfmac to drop IAPP packets. If affects: 1) Rx path: driver won't be sending these unwanted packets up. 2) Tx path: driver will reject packets that would trigger STA disassociation perfromed by a firmware (possible local DoS attack). It appears there are some Broadcom's clients/users who care about this feature despite the drawbacks. They can switch it on by a newly added Kconfig option. Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
[...]
quoted hunk
--- a/drivers/net/wireless/broadcom/brcm80211/Kconfig +++ b/drivers/net/wireless/broadcom/brcm80211/Kconfig@@ -68,6 +68,26 @@ config BRCMFMAC_PCIE IEEE802.11ac embedded FullMAC WLAN driver. Say Y if you want to use the driver for an PCIE wireless card.=20=20 +config BRCMFMAC_IAPP + bool "Partial support for obsoleted Inter-Access Point Protocol" + depends on BRCMFMAC + ---help--- + Most of Broadcom's firmwares can send 802.11f ADD frame every + time new STA connects to the AP interface. Some recent ones + can also disassociate STA when they receive such a frame. + + It's important to understand this behavior can lead to a local + DoS security issue. Attacker may trigger disassociation of any + STA by sending a proper Ethernet frame to the wireless + interface. + + Moreover this feature may break AP interfaces in some specific + setups. This applies e.g. to the bridge with hairpin mode + enabled and IFLA_BRPORT_MCAST_TO_UCAST set. IAPP packet + generated by a firmware will get passed back to the wireless + interface and cause immediate disassociation of just-connected + STA.
Sorry for jumping late, but does it really make sense to have a Kconfig option for this? I don't think we should add a Kconfig option for every strange feature, there should be stronger reasons (size savings etc) before adding a Kconfig option. And in this case the size savings can't be much. Wouldn't a module parameter be simpler for a functionality change like this?
+/**
+ * brcmf_skb_is_iapp - checks if skb is an IAPP packet
+ *
+ * @skb: skb to check
+ */
+static bool brcmf_skb_is_iapp(struct sk_buff *skb)
+{
+ const u8 iapp_l2_update_packet[6] __aligned(2) =3D {
+ 0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+ };static?
+ unsigned char *eth_data =3D skb_mac_header(skb) + ETH_HLEN; +#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
#ifndef?
+ const u16 *a =3D (const u16 *)eth_data; + const u16 *b =3D (const u16 *)iapp_l2_update_packet; +#endif + + if (skb->len - skb->mac_len !=3D 6 || + !is_multicast_ether_addr(eth_hdr(skb)->h_dest)) + return false; + +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
#ifdef? --=20 Kalle Valo