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

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 3/14/2018 12:01 PM, 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. 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@broadcom.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   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