On 2018-03-14 15:24, Kalle Valo wrote:
Rafał Miłecki <zajec5@xxxxxxxxx> writes:
From: Rafał Miłecki <rafal@xxxxxxxxxx>
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ł Miłecki <rafal@xxxxxxxxxx>
[...]
--- 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.
+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) = {
+ 0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+ };
static?
Sure
+ unsigned char *eth_data = skb_mac_header(skb) + ETH_HLEN;
+#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
#ifndef?
I followed what is used in the include/linux/etherdevice.h. Is that a
good exceuse? Could it be there any some good reason for #if defined()?
+ const u16 *a = (const u16 *)eth_data;
+ const u16 *b = (const u16 *)iapp_l2_update_packet;
+#endif
+
+ if (skb->len - skb->mac_len != 6 ||
+ !is_multicast_ether_addr(eth_hdr(skb)->h_dest))
+ return false;
+
+#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
#ifdef?
