Return-path: Received: from 7.mo178.mail-out.ovh.net ([46.105.58.91]:59801 "EHLO 7.mo178.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753133AbeAaSAm (ORCPT ); Wed, 31 Jan 2018 13:00:42 -0500 Received: from player728.ha.ovh.net (b6.ovh.net [213.186.33.56]) by mo178.mail-out.ovh.net (Postfix) with ESMTP id AB79772BE4 for ; Wed, 31 Jan 2018 14:12:01 +0100 (CET) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Date: Wed, 31 Jan 2018 14:11:49 +0100 From: =?UTF-8?Q?Rafa=C5=82_Mi=C5=82ecki?= To: Arend van Spriel Cc: =?UTF-8?Q?Rafa=C5=82_Mi=C5=82ecki?= , Kalle Valo , Franky Lin , Hante Meuleman , Chi-Hsien Lin , Wright Feng , Pieter-Paul Giesberts , linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, brcm80211-dev-list@cypress.com Subject: Re: [PATCH] brcmfmac: detect & reject faked packet generated by a firmware In-Reply-To: <5A705741.6050402@broadcom.com> References: <20180130090922.30346-1-zajec5@gmail.com> <5A705741.6050402@broadcom.com> Message-ID: <44f66f2254e7a5950ec32068b31d7b54@milecki.pl> (sfid-20180131_190046_806885_BAB0EBE1) Sender: linux-wireless-owner@vger.kernel.org List-ID: On 2018-01-30 12:30, Arend van Spriel wrote: > On 1/30/2018 10:09 AM, Rafał Miłecki wrote: >> From: Rafał Miłecki >> >> When using 4366b1 and 4366c0 chipsets with more recent firmwares >> 1) 10.10 (TOB) (r663589) >> 2) 10.10.122.20 (r683106) >> respectively, it is impossible to use brcmfmac with interface in AP >> mode. With the AP interface bridged and multicast used, no STA will be >> able to associate; the STA will be immediately disassociated when >> attempting to associate. >> >> Debugging revealed this to be caused by a "faked" packet (generated by >> firmware), that is passed to the networking subsystem and then back to >> the firmware. Fortunately this packet is easily identified and can be >> detected and ignored as a workaround for misbehaving firmware. >> >> Signed-off-by: Rafał Miłecki >> --- >> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 46 >> ++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >> index 930e423f83a8..a98ba9bbc7fe 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >> @@ -323,8 +323,54 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp, >> spin_unlock_irqrestore(&ifp->netif_stop_lock, flags); >> } >> >> +/** >> + * brcmf_is_valid_skb - validates skb received from the hardware >> + * >> + * @skb: skb to check >> + * >> + * Sometimes firmware/hardware can generate broken packets that >> aren't real or >> + * valid and their skb-s shouldn't be passed up to the networking >> subsystem. >> + * >> + * Firmwares for 43602a1, 4366b1 and 4366c0 are known to *generate* a >> faked 6 B >> + * packet whenever a STA associates. The purpose of this fake packet >> remains >> + * unknown but it is clearly not data coming from a station. As such >> it >> + * shouldn't be passed to the networking subsystem. >> + * >> + * Normally such a packet would simply be ignored, but this is not >> the case with >> + * more recent 4366b1 and 4366c0 firmwares. These firmwares seem to >> explicitly >> + * check for this packet and will reject (disassociate) the station, >> making it >> + * impossible to connect to the AP at all. This can happen when using >> a bridged >> + * interface with multicasting. Such a scenario apparently isn't >> tested (or >> + * supported) by Broadcom's internal team. >> + */ >> +static bool brcmf_is_valid_skb(struct sk_buff *skb) >> +{ >> + const u8 fw_faked_packet[6] __aligned(2) = { >> + 0x00, 0x01, 0xaf, 0x81, 0x01, 0x00, >> + }; >> +#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) >> + const u16 *a = (const u16 *)skb->data; >> + const u16 *b = (const u16 *)fw_faked_packet; >> +#endif >> + >> + if (skb->len != 6) >> + return true; >> + >> +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) >> + return !!(((*(const u32 *)skb->data) ^ (*(const u32 >> *)fw_faked_packet)) | >> + ((*(const u16 *)(skb->data + 4)) ^ (*(const u16 >> *)(fw_faked_packet + 4)))); >> +#else >> + return !!((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])); >> +#endif >> +} > > The code above does look very much like ether_addr_equal(). Why not > use that instead of reinventing it. You're right about ether_addr_equal(), I wasn't sure if that is acceptable to use it for comparing any 6 bytes data. I know it'd work, it's just not what it was designed for. Kalle? >> void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb) >> { >> + if (!brcmf_is_valid_skb(skb)) { >> + brcmu_pkt_buf_free_skb(skb); > > Maybe we should add a driver stat for this although I better have a > look into the root cause of this. It seems there are following stats fields we can use: 1) rx_errors 2) rx_dropped 3) rx_length_errors 4) rx_over_errors 5) rx_crc_errors 6) rx_frame_errors 7) rx_fifo_errors 8) rx_missed_errors Which one do you think may fit this case the best?