Return-path: Received: from 6.mo68.mail-out.ovh.net ([46.105.63.100]:53651 "EHLO 6.mo68.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751410AbeCNPir (ORCPT ); Wed, 14 Mar 2018 11:38:47 -0400 Received: from player726.ha.ovh.net (unknown [10.109.105.161]) by mo68.mail-out.ovh.net (Postfix) with ESMTP id 9C301D03CA for ; Wed, 14 Mar 2018 16:28:49 +0100 (CET) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Date: Wed, 14 Mar 2018 16:28:30 +0100 From: =?UTF-8?Q?Rafa=C5=82_Mi=C5=82ecki?= To: Kalle Valo Cc: Arend van Spriel , =?UTF-8?Q?Rafa=C5=82?= =?UTF-8?Q?_Mi=C5=82ecki?= , Franky Lin , Hante Meuleman , Chi-Hsien Lin , Wright Feng , Pieter-Paul Giesberts , James Hughes , linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, brcm80211-dev-list@cypress.com, netdev@vger.kernel.org, =?UTF-8?Q?Linus_L=C3=BCssing?= , Felix Fietkau , bridge@lists.linux-foundation.org Subject: Re: [PATCH] brcmfmac: drop Inter-Access Point Protocol packets by default In-Reply-To: <87o9jq66f3.fsf@codeaurora.org> References: <20180314110119.13631-1-zajec5@gmail.com> <878tau7n23.fsf@codeaurora.org> <5AA93530.5040001@broadcom.com> <87o9jq66f3.fsf@codeaurora.org> Message-ID: (sfid-20180314_163903_208535_84539B6B) Sender: linux-wireless-owner@vger.kernel.org List-ID: On 2018-03-14 16:08, Kalle Valo wrote: > Arend van Spriel 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?