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

[Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

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