| Message ID | 20180315072909.1512-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1259055170287a350cad453e9eac139c81609860 |
| Delegated to: | Kalle Valo |
| Headers | show |
On 15 March 2018 at 08:29, Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <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. > > Older firmwares were only generating 802.11f frames. Newer ones like: > 1) 10.10 (TOB) (r663589) > 2) 10.10.122.20 (r683106) > for 4366b1 and 4366c0 respectively seem to also /respect/ 802.11f frames > in the Tx path by performing a STA disassociation. > > This 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 using a new module > param. > > This change results in only two more comparisons (check for module param > and check for Ethernet packet length) for 99.9% of packets. Its overhead > should be very minimal. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- I forgot to include the changelog, sorry. V2: Use module param to don't /abuse/ Kconfig Slightly optimize brcmf_skb_is_iapp Move some description from Kconfig to the code Update commit description: specify affected fws & mention impact
On 3/15/2018 8:29 AM, Rafał Miłecki wrote: > From: Rafał Miłecki <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. > > Older firmwares were only generating 802.11f frames. Newer ones like: > 1) 10.10 (TOB) (r663589) > 2) 10.10.122.20 (r683106) > for 4366b1 and 4366c0 respectively seem to also /respect/ 802.11f frames > in the Tx path by performing a STA disassociation. > > This 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 using a new module > param. > > This change results in only two more comparisons (check for module param > and check for Ethernet packet length) for 99.9% of packets. Its overhead > should be very minimal. Hi Rafał, Thanks for this patch. Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../wireless/broadcom/brcm80211/brcmfmac/common.c | 5 ++ > .../wireless/broadcom/brcm80211/brcmfmac/common.h | 1 + > .../wireless/broadcom/brcm80211/brcmfmac/core.c | 57 ++++++++++++++++++++++ > 3 files changed, 63 insertions(+)
Rafał Miłecki wrote: > From: Rafał Miłecki <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. > > Older firmwares were only generating 802.11f frames. Newer ones like: > 1) 10.10 (TOB) (r663589) > 2) 10.10.122.20 (r683106) > for 4366b1 and 4366c0 respectively seem to also /respect/ 802.11f frames > in the Tx path by performing a STA disassociation. > > This 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 using a new module > param. > > This change results in only two more comparisons (check for module param > and check for Ethernet packet length) for 99.9% of packets. Its overhead > should be very minimal. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> Patch applied to wireless-drivers.git, thanks. 125905517028 brcmfmac: drop Inter-Access Point Protocol packets by default
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index 70ef9835b647..5532ef39439f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -75,6 +75,10 @@ static int brcmf_roamoff; module_param_named(roamoff, brcmf_roamoff, int, S_IRUSR); MODULE_PARM_DESC(roamoff, "Do not use internal roaming engine"); +static int brcmf_iapp_enable; +module_param_named(iapp, brcmf_iapp_enable, int, 0); +MODULE_PARM_DESC(iapp, "Enable partial support for the obsoleted Inter-Access Point Protocol"); + #ifdef DEBUG /* always succeed brcmf_bus_started() */ static int brcmf_ignore_probe_fail; @@ -438,6 +442,7 @@ struct brcmf_mp_device *brcmf_get_module_param(struct device *dev, settings->feature_disable = brcmf_feature_disable; settings->fcmode = brcmf_fcmode; settings->roamoff = !!brcmf_roamoff; + settings->iapp = !!brcmf_iapp_enable; #ifdef DEBUG settings->ignore_probe_fail = !!brcmf_ignore_probe_fail; #endif diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h index a62f8e70b320..ef914619e8e1 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h @@ -58,6 +58,7 @@ struct brcmf_mp_device { unsigned int feature_disable; int fcmode; bool roamoff; + bool iapp; bool ignore_probe_fail; struct brcmfmac_pd_cc *country_codes; union { diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c index 19048526b4af..ca97a8b4c59f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c @@ -230,6 +230,37 @@ static void brcmf_netdev_set_multicast_list(struct net_device *ndev) schedule_work(&ifp->multicast_work); } +/** + * 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) +{ + static const u8 iapp_l2_update_packet[6] __aligned(2) = { + 0x00, 0x01, 0xaf, 0x81, 0x01, 0x00, + }; + unsigned char *eth_data; +#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) + const u16 *a, *b; +#endif + + if (skb->len - skb->mac_len != 6 || + !is_multicast_ether_addr(eth_hdr(skb)->h_dest)) + return false; + + eth_data = skb_mac_header(skb) + ETH_HLEN; +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) + return !(((*(const u32 *)eth_data) ^ (*(const u32 *)iapp_l2_update_packet)) | + ((*(const u16 *)(eth_data + 4)) ^ (*(const u16 *)(iapp_l2_update_packet + 4)))); +#else + a = (const u16 *)eth_data; + b = (const u16 *)iapp_l2_update_packet; + + return !((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])); +#endif +} + static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, struct net_device *ndev) { @@ -250,6 +281,23 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, goto done; } + /* Some recent Broadcom's firmwares disassociate STA when they receive + * an 802.11f ADD frame. 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 a just-connected STA. + */ + if (!drvr->settings->iapp && brcmf_skb_is_iapp(skb)) { + dev_kfree_skb(skb); + ret = -EINVAL; + goto done; + } + /* Make sure there's enough writeable headroom */ if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) { head_delta = max_t(int, drvr->hdrlen - skb_headroom(skb), 0); @@ -325,6 +373,15 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp, void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb) { + /* Most of Broadcom's firmwares send 802.11f ADD frame every time a new + * STA connects to the AP interface. This is an obsoleted standard most + * users don't use, so don't pass these frames up unless requested. + */ + if (!ifp->drvr->settings->iapp && brcmf_skb_is_iapp(skb)) { + brcmu_pkt_buf_free_skb(skb); + return; + } + if (skb->pkt_type == PACKET_MULTICAST) ifp->ndev->stats.multicast++;