Linux Ethernet Bridging - Mar 2018 - [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

Arend van Spriel <arend.vanspriel at broadcom.com> 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

On Wed, 14 Mar 2018 17:08:48 +0200
Kalle Valo <kvalo at codeaurora.org> wrote:
> Arend van Spriel <arend.vanspriel at broadcom.com> 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