[PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

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-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 drivers/net/wireless/broadcom/brcm80211/Kconfig    | 20 +++++++++++
 .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 39 ++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig b/drivers/net/wireless/broadcom/brcm80211/Kconfig
index 9d99eb42d917..876787ef991a 100644
--- 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.
+
 config BRCM_TRACING
 	bool "Broadcom device tracing"
 	depends on BRCMSMAC || BRCMFMAC
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 19048526b4af..db6987015fb1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -230,6 +230,34 @@ 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)
+{
+	const u8 iapp_l2_update_packet[6] __aligned(2) = {
+		0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
+	};
+	unsigned char *eth_data = skb_mac_header(skb) + ETH_HLEN;
+#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
+	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)
+	return !(((*(const u32 *)eth_data) ^ (*(const u32 *)iapp_l2_update_packet)) |
+		 ((*(const u16 *)(eth_data + 4)) ^ (*(const u16 *)(iapp_l2_update_packet + 4))));
+#else
+	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 +278,12 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
 		goto done;
 	}
 
+	if (!IS_ENABLED(CONFIG_BRCMFMAC_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 +359,11 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp,
 
 void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
 {
+	if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
+		brcmu_pkt_buf_free_skb(skb);
+		return;
+	}
+
 	if (skb->pkt_type == PACKET_MULTICAST)
 		ifp->ndev->stats.multicast++;
 
-- 
2.11.0

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/14/2018 12:01 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>
> 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.

Thanks for taking this approach. Looks fine except for .... (see below)

Reviewed-by: Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
>   drivers/net/wireless/broadcom/brcm80211/Kconfig    | 20 +++++++++++
>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 39 ++++++++++++++++++++++
>   2 files changed, 59 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig b/drivers/net/wireless/broadcom/brcm80211/Kconfig
> index 9d99eb42d917..876787ef991a 100644
> --- 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.

I do not see any evidence that this would occur only for recent 
firmware. That stuff is old and not touched recently.

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 19048526b4af..db6987015fb1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c

[...]

>   static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>   					   struct net_device *ndev)
>   {
> @@ -250,6 +278,12 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>   		goto done;
>   	}
>
> +	if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
> +		dev_kfree_skb(skb);
> +		ret = -EINVAL;
> +		goto done;
> +	}

This is not right. The function must return netdev_tx_t type. Here is 
kerneldoc of .start_xmit():

  * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
  *                               struct net_device *dev);
  *	Called when a packet needs to be transmitted.
  *	Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you should stop
  *	the queue before that can happen; it's for obsolete devices and weird
  *	corner cases, but the stack really does a non-trivial amount
  *	of useless work if you return NETDEV_TX_BUSY.
  *	Required; cannot be NULL.

You may want to increase dropped netstat or add driver internal 
statistic counter so there is visibility of IAPP packets being dropped.

Regards,
Arend

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Kalle Valo]

Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>
> 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-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

[...]

> --- 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

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

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

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Stephen Hemminger]

On Wed, 14 Mar 2018 12:01:19 +0100
Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 19048526b4af..db6987015fb1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -230,6 +230,34 @@ 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)
> +{
> +	const u8 iapp_l2_update_packet[6] __aligned(2) = {
> +		0x00, 0x01, 0xaf, 0x81, 0x01, 0x00,
> +	};
> +	unsigned char *eth_data = skb_mac_header(skb) + ETH_HLEN;
> +#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> +	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)
> +	return !(((*(const u32 *)eth_data) ^ (*(const u32 *)iapp_l2_update_packet)) |
> +		 ((*(const u16 *)(eth_data + 4)) ^ (*(const u16 *)(iapp_l2_update_packet + 4))));
> +#else
> +	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 +278,12 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>  		goto done;
>  	}
>  
> +	if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
> +		dev_kfree_skb(skb);
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +

The usual way to handle config options in kernel is either inline
stub function or #define

#ifdef CONFIG_BRFMMAC_IAPP
static bool brcmf_skb_is_app(...) {
	real code
}
#else
#define brcmf_skb_is_app  (false)
#endif

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Kalle Valo]

Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> writes:

> 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.

I think Linus doesn't like pointless Kconfig options, me neither for
that matter, so I try to make sure the justifications are really there
before adding anything new.

> 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.

I'm no cpu profile expert but is really one (or two?) if checks of a
cached variable in the datapath really measurable? My guess is that it's
just noise in the results.

But I'm not going to argue about it, if you think it's still needed I'm
fine with that. Just mention in the commit log the justification the new
Kconfig option.

-- 
Kalle Valo

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Stephen Hemminger]

On Wed, 14 Mar 2018 17:08:48 +0200
Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:

> Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> writes:
> 
> > 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.  
> 
> I think Linus doesn't like pointless Kconfig options, me neither for
> that matter, so I try to make sure the justifications are really there
> before adding anything new.
> 
> > 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.  
> 
> I'm no cpu profile expert but is really one (or two?) if checks of a
> cached variable in the datapath really measurable? My guess is that it's
> just noise in the results.
> 
> But I'm not going to argue about it, if you think it's still needed I'm
> fine with that. Just mention in the commit log the justification the new
> Kconfig option.


If you have to disable it a module parameter is not a complete disaster

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Rafał Miłecki]

On 2018-03-14 16:08, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> writes:
> 
>> 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.
> 
> I think Linus doesn't like pointless Kconfig options, me neither for
> that matter, so I try to make sure the justifications are really there
> before adding anything new.
> 
>> 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.
> 
> I'm no cpu profile expert but is really one (or two?) if checks of a
> cached variable in the datapath really measurable? My guess is that 
> it's
> just noise in the results.
> 
> But I'm not going to argue about it, if you think it's still needed I'm
> fine with that. Just mention in the commit log the justification the 
> new
> Kconfig option.

I think you should be right and that's also why I put
skb->len - skb->mac_len != 6
as the first check in that function. That simple (quick?) check should
reject 99.9% of packets.

I could move skb_mac_header() call a bit further which should optimize
this function even more and maybe then we could switch to the module
parameter?

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Rafał Miłecki]

On 2018-03-14 13:58, Arend van Spriel wrote:
> On 3/14/2018 12:01 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> 
>> 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.
> 
> Thanks for taking this approach. Looks fine except for .... (see below)
> 
> Reviewed-by: Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>>   drivers/net/wireless/broadcom/brcm80211/Kconfig    | 20 +++++++++++
>>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 39 
>> ++++++++++++++++++++++
>>   2 files changed, 59 insertions(+)
>> 
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
>> b/drivers/net/wireless/broadcom/brcm80211/Kconfig
>> index 9d99eb42d917..876787ef991a 100644
>> --- 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.
> 
> I do not see any evidence that this would occur only for recent
> firmware. That stuff is old and not touched recently.

My evidence is comparing firmwares for 4366b1: 10.10.69.3309 (r610991)
vs. 10.10 (TOB) (r663589).

The first one is from linux-firmware.git and it doesn't implement IAPP
in the TX path. The later one is what I got from you privately and it
implements it.

Also a firmware for 4366c0: 10.10.122.20 (r683106) which is relatively
new implements IAPP in the TX path.


>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> index 19048526b4af..db6987015fb1 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> 
> [...]
> 
>>   static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>>   					   struct net_device *ndev)
>>   {
>> @@ -250,6 +278,12 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct 
>> sk_buff *skb,
>>   		goto done;
>>   	}
>> 
>> +	if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
>> +		dev_kfree_skb(skb);
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
> 
> This is not right. The function must return netdev_tx_t type. Here is
> kerneldoc of .start_xmit():
> 
>  * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
>  *                               struct net_device *dev);
>  *	Called when a packet needs to be transmitted.
>  *	Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you should 
> stop
>  *	the queue before that can happen; it's for obsolete devices and 
> weird
>  *	corner cases, but the stack really does a non-trivial amount
>  *	of useless work if you return NETDEV_TX_BUSY.
>  *	Required; cannot be NULL.

Please take a closer look at how brcmf_netdev_start_xmit() works. Above
  code *will* return netdev_tx_t.


> You may want to increase dropped netstat or add driver internal
> statistic counter so there is visibility of IAPP packets being
> dropped.

OK, I'll try to find a stat to increase.

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Rafał Miłecki]

On 2018-03-14 16:39, Rafał Miłecki wrote:
> On 2018-03-14 13:58, Arend van Spriel wrote:
>> On 3/14/2018 12:01 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>> 
>>> 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.
>> 
>> Thanks for taking this approach. Looks fine except for .... (see 
>> below)
>> 
>> Reviewed-by: Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>> ---
>>>   drivers/net/wireless/broadcom/brcm80211/Kconfig    | 20 +++++++++++
>>>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 39 
>>> ++++++++++++++++++++++
>>>   2 files changed, 59 insertions(+)
>>> 
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
>>> b/drivers/net/wireless/broadcom/brcm80211/Kconfig
>>> index 9d99eb42d917..876787ef991a 100644
>>> --- 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.
>> 
>> I do not see any evidence that this would occur only for recent
>> firmware. That stuff is old and not touched recently.
> 
> My evidence is comparing firmwares for 4366b1: 10.10.69.3309 (r610991)
> vs. 10.10 (TOB) (r663589).
> 
> The first one is from linux-firmware.git and it doesn't implement IAPP
> in the TX path. The later one is what I got from you privately and it
> implements it.
> 
> Also a firmware for 4366c0: 10.10.122.20 (r683106) which is relatively
> new implements IAPP in the TX path.

Please also take a look at my original patch
[PATCH] brcmfmac: detect & reject faked packet generated by a firmware
https://patchwork.kernel.org/patch/10191451/

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Rafał Miłecki]

On 2018-03-14 15:24, Kalle Valo wrote:
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> 
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> 
>> 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-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> 
> [...]
> 
>> --- 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?

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Rafał Miłecki]

On 2018-03-14 16:39, Rafał Miłecki wrote:
> On 2018-03-14 13:58, Arend van Spriel wrote:
>> On 3/14/2018 12:01 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>> 
>>> 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.
>> 
>> Thanks for taking this approach. Looks fine except for .... (see 
>> below)
>> 
>> Reviewed-by: Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>> ---
>>>   drivers/net/wireless/broadcom/brcm80211/Kconfig    | 20 +++++++++++
>>>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 39 
>>> ++++++++++++++++++++++
>>>   2 files changed, 59 insertions(+)
>>> 
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig 
>>> b/drivers/net/wireless/broadcom/brcm80211/Kconfig
>>> index 9d99eb42d917..876787ef991a 100644
>>> --- 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.
>> 
>> I do not see any evidence that this would occur only for recent
>> firmware. That stuff is old and not touched recently.
> 
> My evidence is comparing firmwares for 4366b1: 10.10.69.3309 (r610991)
> vs. 10.10 (TOB) (r663589).
> 
> The first one is from linux-firmware.git and it doesn't implement IAPP
> in the TX path. The later one is what I got from you privately and it
> implements it.
> 
> Also a firmware for 4366c0: 10.10.122.20 (r683106) which is relatively
> new implements IAPP in the TX path.
> 
> 
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> index 19048526b4af..db6987015fb1 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> 
>> [...]
>> 
>>>   static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>>>   					   struct net_device *ndev)
>>>   {
>>> @@ -250,6 +278,12 @@ static netdev_tx_t 
>>> brcmf_netdev_start_xmit(struct sk_buff *skb,
>>>   		goto done;
>>>   	}
>>> 
>>> +	if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
>>> +		dev_kfree_skb(skb);
>>> +		ret = -EINVAL;
>>> +		goto done;
>>> +	}
>> 
>> This is not right. The function must return netdev_tx_t type. Here is
>> kerneldoc of .start_xmit():
>> 
>>  * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
>>  *                               struct net_device *dev);
>>  *	Called when a packet needs to be transmitted.
>>  *	Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you should 
>> stop
>>  *	the queue before that can happen; it's for obsolete devices and 
>> weird
>>  *	corner cases, but the stack really does a non-trivial amount
>>  *	of useless work if you return NETDEV_TX_BUSY.
>>  *	Required; cannot be NULL.
> 
> Please take a closer look at how brcmf_netdev_start_xmit() works. Above
>  code *will* return netdev_tx_t.
> 
> 
>> You may want to increase dropped netstat or add driver internal
>> statistic counter so there is visibility of IAPP packets being
>> dropped.
> 
> OK, I'll try to find a stat to increase.

So after checking brcmf_netdev_start_xmit() again, I realized I actually
*do* that. Doing:
ret = -EINVAL;
goto done;
results in increasing tx_dropped.

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Kalle Valo]

Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org> writes:

>>> +	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()?

Don't know, maybe just a matter of taste? But it would be nice to know
the background behind #ifdef vs #if defined(), never figured it out why
two different forms.

-- 
Kalle Valo

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/14/2018 4:57 PM, Rafał Miłecki wrote:
> On 2018-03-14 16:39, Rafał Miłecki wrote:
>> On 2018-03-14 13:58, Arend van Spriel wrote:
>>> On 3/14/2018 12:01 PM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>
>>>> 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.
>>>
>>> Thanks for taking this approach. Looks fine except for .... (see below)
>>>
>>> Reviewed-by: Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>> ---
>>>>   drivers/net/wireless/broadcom/brcm80211/Kconfig    | 20 +++++++++++
>>>>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 39
>>>> ++++++++++++++++++++++
>>>>   2 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/Kconfig
>>>> b/drivers/net/wireless/broadcom/brcm80211/Kconfig
>>>> index 9d99eb42d917..876787ef991a 100644
>>>> --- 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.
>>>
>>> I do not see any evidence that this would occur only for recent
>>> firmware. That stuff is old and not touched recently.
>>
>> My evidence is comparing firmwares for 4366b1: 10.10.69.3309 (r610991)
>> vs. 10.10 (TOB) (r663589).
>>
>> The first one is from linux-firmware.git and it doesn't implement IAPP
>> in the TX path. The later one is what I got from you privately and it
>> implements it.
>>
>> Also a firmware for 4366c0: 10.10.122.20 (r683106) which is relatively
>> new implements IAPP in the TX path.
>>
>>
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>> index 19048526b4af..db6987015fb1 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>
>>> [...]
>>>
>>>>   static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>>>>                          struct net_device *ndev)
>>>>   {
>>>> @@ -250,6 +278,12 @@ static netdev_tx_t
>>>> brcmf_netdev_start_xmit(struct sk_buff *skb,
>>>>           goto done;
>>>>       }
>>>>
>>>> +    if (!IS_ENABLED(CONFIG_BRCMFMAC_IAPP) && brcmf_skb_is_iapp(skb)) {
>>>> +        dev_kfree_skb(skb);
>>>> +        ret = -EINVAL;
>>>> +        goto done;
>>>> +    }
>>>
>>> This is not right. The function must return netdev_tx_t type. Here is
>>> kerneldoc of .start_xmit():
>>>
>>>  * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
>>>  *                               struct net_device *dev);
>>>  *    Called when a packet needs to be transmitted.
>>>  *    Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you
>>> should stop
>>>  *    the queue before that can happen; it's for obsolete devices and
>>> weird
>>>  *    corner cases, but the stack really does a non-trivial amount
>>>  *    of useless work if you return NETDEV_TX_BUSY.
>>>  *    Required; cannot be NULL.
>>
>> Please take a closer look at how brcmf_netdev_start_xmit() works. Above
>>  code *will* return netdev_tx_t.
>>
>>
>>> You may want to increase dropped netstat or add driver internal
>>> statistic counter so there is visibility of IAPP packets being
>>> dropped.
>>
>> OK, I'll try to find a stat to increase.
>
> So after checking brcmf_netdev_start_xmit() again, I realized I actually
> *do* that. Doing:
> ret = -EINVAL;
> goto done;
> results in increasing tx_dropped.

Okay, okay. Admittedly I only looked at the patch. Feel free to remove 
the Reviewed-by.

Regards,
Arend

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Arend van Spriel]

On 3/14/2018 5:10 PM, Kalle Valo wrote:
> Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org> writes:
>
>>>> +	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()?
>
> Don't know, maybe just a matter of taste? But it would be nice to know
> the background behind #ifdef vs #if defined(), never figured it out why
> two different forms.

Well. In this case you could use either one, but if you have more 
conditions #if defined() is bit more efficient:

#ifdef A
#ifdef B
#endif
#endif

vs.

#if defined(A) && defined(B)

Regards,
Arend

Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default

[Kalle Valo]

Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> writes:

> On 3/14/2018 5:10 PM, Kalle Valo wrote:
>> Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org> writes:
>>
>>>>> +	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()?
>>
>> Don't know, maybe just a matter of taste? But it would be nice to know
>> the background behind #ifdef vs #if defined(), never figured it out why
>> two different forms.
>
> Well. In this case you could use either one, but if you have more
> conditions #if defined() is bit more efficient:
>
> #ifdef A
> #ifdef B
> #endif
> #endif
>
> vs.
>
> #if defined(A) && defined(B)

Oh yeah, here defined() definitely helps.

-- 
Kalle Valo