Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:50550 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752061AbeAaOT6 (ORCPT ); Wed, 31 Jan 2018 09:19:58 -0500 Received: by mail-wm0-f66.google.com with SMTP id f71so8520313wmf.0 for ; Wed, 31 Jan 2018 06:19:57 -0800 (PST) Subject: Re: [PATCH] brcmfmac: detect & reject faked packet generated by a firmware To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= References: <20180130090922.30346-1-zajec5@gmail.com> <5A705B5E.5070906@broadcom.com> Cc: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , 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 From: Arend van Spriel Message-ID: <5A71D08B.7090905@broadcom.com> (sfid-20180131_152004_135642_C1773B70) Date: Wed, 31 Jan 2018 15:19:55 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 1/31/2018 2:14 PM, Rafał Miłecki wrote: > On 2018-01-30 12:47, 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. >> >> I am actually wondering what this packet is. Have you checked in >> brcmf_msgbuf_process_rx_complete(). I am curious what buflen is there >> and what eth_type_trans() will do to the packet, ie. what protocol. As >> everything should be 802.3 we could/should add a length check of 14 >> bytes. > > Did you find anything? I was going to say no, but below I see I misinterpreted your commit message and thought we were getting 6 bytes from firmware, but it is 6 bytes + ETH_HLEN. > I got some debugging info, hopefully this is what you expected and more ... :-) > [ 144.356648] brcmfmac: [brcmf_msgbuf_process_rx_complete] msg.msgtype: > 0x12 > [ 144.363559] brcmfmac: [brcmf_msgbuf_process_rx_complete] msg.ifidx: > 0x00 > [ 144.370374] brcmfmac: [brcmf_msgbuf_process_rx_complete] msg.flags: > 0x80 > [ 144.377179] brcmfmac: [brcmf_msgbuf_process_rx_complete] msg.rsvd0: > 0x00 > [ 144.383986] brcmfmac: [brcmf_msgbuf_process_rx_complete] > msg.request_id: 0x00000041 > [ 144.391661] brcmfmac: [brcmf_msgbuf_process_rx_complete] > compl_hdr.status: 0x0000 > [ 144.399156] brcmfmac: [brcmf_msgbuf_process_rx_complete] > compl_hdr.flow_ring_id: 0x0000 > [ 144.407179] brcmfmac: [brcmf_msgbuf_process_rx_complete] > metadata_len: 0x0000 > [ 144.414334] brcmfmac: [brcmf_msgbuf_process_rx_complete] data_len: > 0x0014 > [ 144.421227] brcmfmac: [brcmf_msgbuf_process_rx_complete] data_offset: > 0x0000 > [ 144.428288] brcmfmac: [brcmf_msgbuf_process_rx_complete] flags: > 0x0001 > [ 144.434918] brcmfmac: [brcmf_msgbuf_process_rx_complete] rx_status_0: > 0x00000000 > [ 144.442334] brcmfmac: [brcmf_msgbuf_process_rx_complete] rx_status_1: > 0x00000000 > [ 144.449750] brcmfmac: [brcmf_msgbuf_process_rx_complete] rsvd0: > 0x00000001 > [ 144.456724] brcmfmac: [brcmf_msgbuf_process_rx_complete] skb->data: > ff ff ff ff ff ff ec 10 7b 5f ?? ?? 00 06 00 01 af 81 01 00 > [ 144.467883] brcmfmac: [brcmf_msgbuf_process_rx_complete] > skb->protocol: 0x0400 Not sure what protocol that is. Can not find it in if_ether.h. Will look in our firmware repo for it. Thanks, Arend > (just masked 2 bytes of my MAC) > > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c > index 1bd4b96..08cdcaf 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c > @@ -1172,7 +1172,43 @@ brcmf_msgbuf_process_rx_complete(struct > brcmf_msgbuf *msgbuf, void *buf) > return; > } > > + if (skb->len == ETH_HLEN + 6) { > + uint8_t *data; > + int i; > + > + pr_info("[%s] msg.msgtype:\t0x%02x\n", __func__, > rx_complete->msg.msgtype); > + pr_info("[%s] msg.ifidx:\t\t0x%02x\n", __func__, > rx_complete->msg.ifidx); > + pr_info("[%s] msg.flags:\t\t0x%02x\n", __func__, > rx_complete->msg.flags); > + pr_info("[%s] msg.rsvd0:\t\t0x%02x\n", __func__, > rx_complete->msg.rsvd0); > + pr_info("[%s] msg.request_id:\t0x%08x\n", __func__, > le32_to_cpu(rx_complete->msg.request_id)); > + > + pr_info("[%s] compl_hdr.status:\t0x%04x\n", __func__, > le16_to_cpu(rx_complete->compl_hdr.status)); > + pr_info("[%s] compl_hdr.flow_ring_id:\t0x%04x\n", __func__, > le16_to_cpu(rx_complete->compl_hdr.flow_ring_id)); > + > + pr_info("[%s] metadata_len:\t0x%04x\n", __func__, > le16_to_cpu(rx_complete->metadata_len)); > + pr_info("[%s] data_len:\t\t0x%04x\n", __func__, > le16_to_cpu(rx_complete->data_len)); > + pr_info("[%s] data_offset:\t0x%04x\n", __func__, > le16_to_cpu(rx_complete->data_offset)); > + pr_info("[%s] flags:\t\t0x%04x\n", __func__, > le16_to_cpu(rx_complete->flags)); > + pr_info("[%s] rx_status_0:\t0x%08x\n", __func__, > le32_to_cpu(rx_complete->rx_status_0)); > + pr_info("[%s] rx_status_1:\t0x%08x\n", __func__, > le32_to_cpu(rx_complete->rx_status_1)); > + pr_info("[%s] rsvd0:\t\t0x%08x\n", __func__, > le32_to_cpu(rx_complete->rsvd0)); > + > + data = skb->data; > + pr_info("[%s] skb->data:\t\t", __func__); > + for (i = 0; i < 32 && i < skb->len; i++) { > + pr_cont("%02x ", data[i]); > + if (i % 4 == 3) > + pr_cont(" "); > + } > + pr_cont("\n"); > + } > + > skb->protocol = eth_type_trans(skb, ifp->ndev); > + > + if (skb->len == 6) { > + pr_info("[%s] skb->protocol:\t0x%04x\n", __func__, skb->protocol); > + } > + > brcmf_netif_rx(ifp, skb); > } >