Kalle Valo
2018-Mar-14 14:24 UTC
[Bridge] [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default
Rafa? Mi?ecki <zajec5 at gmail.com> writes:> From: Rafa? Mi?ecki <rafal at 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? Mi?ecki <rafal at milecki.pl>[...]> --- 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?> + unsigned char *eth_data = skb_mac_header(skb) + ETH_HLEN; > +#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)#ifndef?> + 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? -- Kalle Valo
Arend van Spriel
2018-Mar-14 14:44 UTC
[Bridge] [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default
On 3/14/2018 3:24 PM, Kalle Valo wrote:>> +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?Hi Kalle, Good to be wary about Kconfig option. So my reason for asking a Kconfig option is that this is directly in the datapaths (tx and rx) so I prefer to disable/enable it compile time rather then runtime. Regards, Arend
Rafał Miłecki
2018-Mar-14 15:45 UTC
[Bridge] [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default
On 2018-03-14 15:24, Kalle Valo wrote:> Rafa? Mi?ecki <zajec5 at gmail.com> writes: > >> From: Rafa? Mi?ecki <rafal at 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? Mi?ecki <rafal at milecki.pl> > > [...] > >> --- 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?