Re: [PATCH] brcmfmac: detect & reject faked packet generated by a firmware

[Arend van Spriel <arend.vanspriel@broadcom.com>]

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 <rafal@milecki.pl>
>>>
>>> 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);
>   }
>