diff mbox

brcmfmac: detect & reject faked packet generated by a firmware

Message ID e6719fcc4080afc43d62d370ad9cfd34@milecki.pl (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Rafał Miłecki Jan. 31, 2018, 1:14 p.m. UTC
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 got some debugging info, hopefully this is what you expected

[  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

(just masked 2 bytes of my MAC)


+			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);
  }

Comments

Arend Van Spriel Jan. 31, 2018, 2:19 p.m. UTC | #1
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);
>   }
>
Hante Meuleman Jan. 31, 2018, 4:14 p.m. UTC | #2
It is an 802.2 frame, more specifically a LLC XID frames. So why it exists?
And more over, why would we crash as an result? Decoding info can be found
here:

https://www.cisco.com/c/en/us/support/docs/ibm-technologies/logical-link-control-llc/12247-45.html#con3

The frame was likely sent by the stack from remote site PC, should be
possible to capture with tcpdump.

I've seen these frames before, but don’t know what they are for. The frame
appears to be correctly encoded. The ethertype, is not a type, but a len
field. The only protocol with such a short len allowed is llc, see also

https://www.savvius.com/networking-glossary/ethernet/frame_formats/

So it is 802.2 (also known as LLC)

Regads,
Hante



-----Original Message-----
From: Arend van Spriel [mailto:arend.vanspriel@broadcom.com]
Sent: Wednesday, January 31, 2018 3:20 PM
To: Rafał Miłecki
Cc: Rafał Miłecki; 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

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);
>   }
>
Arend Van Spriel Jan. 31, 2018, 6:02 p.m. UTC | #3
On 1/31/2018 5:14 PM, Hante Meuleman wrote:
> It is an 802.2 frame, more specifically a LLC XID frames. So why it exists?
> And more over, why would we crash as an result? Decoding info can be found
> here:
>
> https://www.cisco.com/c/en/us/support/docs/ibm-technologies/logical-link-control-llc/12247-45.html#con3
>
> The frame was likely sent by the stack from remote site PC, should be
> possible to capture with tcpdump.
>
> I've seen these frames before, but don’t know what they are for. The frame
> appears to be correctly encoded. The ethertype, is not a type, but a len
> field. The only protocol with such a short len allowed is llc, see also

Could it be related to the fact that the interface is put in a bridge 
and hence the device is put in promiscuous mode? Anyway, I did not read 
anything about a firmware crash. Just that clients could not associate.

Regards,
Arend
Rafał Miłecki Feb. 1, 2018, 10:42 a.m. UTC | #4
On 2018-01-31 17:14, Hante Meuleman wrote:
> It is an 802.2 frame, more specifically a LLC XID frames. So why it 
> exists?
> And more over, why would we crash as an result? Decoding info can be 
> found
> here:
> 
> https://www.cisco.com/c/en/us/support/docs/ibm-technologies/logical-link-control-llc/12247-45.html#con3
> 
> The frame was likely sent by the stack from remote site PC, should be
> possible to capture with tcpdump.
> 
> I've seen these frames before, but don’t know what they are for. The 
> frame
> appears to be correctly encoded. The ethertype, is not a type, but a 
> len
> field. The only protocol with such a short len allowed is llc, see also
> 
> https://www.savvius.com/networking-glossary/ethernet/frame_formats/
> 
> So it is 802.2 (also known as LLC)

Please, try to accept for a moment that it may be really a *firmware*
doing something unexpected. I feel you don't really want to trust my
research and conclusions ;)

Maybe you can spend a moment and try to reproduce this problem? It
should be rather simple, I see this packet every time.

Why I'm blaming a firmware:

1) I see that packet being sent no matter what device tries to connect 
(Linux, Android, Windows).

2) I can't see that packet when connecting the same devices to a 
non-Broadcom AP.

3) Running Wireshark on my Linux notebook never shows that packet 
leaving my notebook

4) Running independent device in monitor mode never catches that packet 
in the air

I really tried to do my homework well before sending this patch. I see
no other explanation for this packet's existence.

Could you try grepping your firmware source looking some LLC references?
Maybe there is really something you can find there to confirm this
issue?

P.S.
Arend's right, firmware isn't crashing, I never said that :)
Arend Van Spriel Feb. 1, 2018, 11:04 a.m. UTC | #5
On 2/1/2018 11:42 AM, Rafał Miłecki wrote:
> On 2018-01-31 17:14, Hante Meuleman wrote:
>> It is an 802.2 frame, more specifically a LLC XID frames. So why it
>> exists?
>> And more over, why would we crash as an result? Decoding info can be
>> found
>> here:
>>
>> https://www.cisco.com/c/en/us/support/docs/ibm-technologies/logical-link-control-llc/12247-45.html#con3
>>
>>
>> The frame was likely sent by the stack from remote site PC, should be
>> possible to capture with tcpdump.
>>
>> I've seen these frames before, but don’t know what they are for. The
>> frame
>> appears to be correctly encoded. The ethertype, is not a type, but a len
>> field. The only protocol with such a short len allowed is llc, see also
>>
>> https://www.savvius.com/networking-glossary/ethernet/frame_formats/
>>
>> So it is 802.2 (also known as LLC)
>
> Please, try to accept for a moment that it may be really a *firmware*
> doing something unexpected. I feel you don't really want to trust my
> research and conclusions ;)

We do. What Hante is saying is that it is a valid packet and we should 
not discard it.

> Maybe you can spend a moment and try to reproduce this problem? It
> should be rather simple, I see this packet every time.

I tried on my OpenWrt box, which is a bridged config, but did not see it.

> Why I'm blaming a firmware:
>
> 1) I see that packet being sent no matter what device tries to connect
> (Linux, Android, Windows).
>
> 2) I can't see that packet when connecting the same devices to a
> non-Broadcom AP.
>
> 3) Running Wireshark on my Linux notebook never shows that packet
> leaving my notebook
>
> 4) Running independent device in monitor mode never catches that packet
> in the air
>
> I really tried to do my homework well before sending this patch. I see
> no other explanation for this packet's existence.

Ok.

> Could you try grepping your firmware source looking some LLC references?
> Maybe there is really something you can find there to confirm this
> issue?

Will do.

> P.S.
> Arend's right, firmware isn't crashing, I never said that :)

Regards,
Arend
Rafał Miłecki Feb. 1, 2018, 11:16 a.m. UTC | #6
On 2018-02-01 12:04, Arend van Spriel wrote:
> On 2/1/2018 11:42 AM, Rafał Miłecki wrote:
>> On 2018-01-31 17:14, Hante Meuleman wrote:
>>> It is an 802.2 frame, more specifically a LLC XID frames. So why it
>>> exists?
>>> And more over, why would we crash as an result? Decoding info can be
>>> found
>>> here:
>>> 
>>> https://www.cisco.com/c/en/us/support/docs/ibm-technologies/logical-link-control-llc/12247-45.html#con3
>>> 
>>> 
>>> The frame was likely sent by the stack from remote site PC, should be
>>> possible to capture with tcpdump.
>>> 
>>> I've seen these frames before, but don’t know what they are for. The
>>> frame
>>> appears to be correctly encoded. The ethertype, is not a type, but a 
>>> len
>>> field. The only protocol with such a short len allowed is llc, see 
>>> also
>>> 
>>> https://www.savvius.com/networking-glossary/ethernet/frame_formats/
>>> 
>>> So it is 802.2 (also known as LLC)
>> 
>> Please, try to accept for a moment that it may be really a *firmware*
>> doing something unexpected. I feel you don't really want to trust my
>> research and conclusions ;)
> 
> We do. What Hante is saying is that it is a valid packet and we should
> not discard it.

I think Hante believed it's a packet "sent by the stack from remote site
PC" which would make it completely valid.

If that packet was never sent by a STA and firmware is just making it
up, including putting STA's MAC as source address (in the Ethernet
header) it smells fishy. Do we still find it a valid packet?


>> Maybe you can spend a moment and try to reproduce this problem? It
>> should be rather simple, I see this packet every time.
> 
> I tried on my OpenWrt box, which is a bridged config, but did not see 
> it.

Can you try my debugging diff I sent yesterday?


>> Why I'm blaming a firmware:
>> 
>> 1) I see that packet being sent no matter what device tries to connect
>> (Linux, Android, Windows).
>> 
>> 2) I can't see that packet when connecting the same devices to a
>> non-Broadcom AP.
>> 
>> 3) Running Wireshark on my Linux notebook never shows that packet
>> leaving my notebook
>> 
>> 4) Running independent device in monitor mode never catches that 
>> packet
>> in the air
>> 
>> I really tried to do my homework well before sending this patch. I see
>> no other explanation for this packet's existence.
> 
> Ok.
> 
>> Could you try grepping your firmware source looking some LLC 
>> references?
>> Maybe there is really something you can find there to confirm this
>> issue?
> 
> Will do.
Rafał Miłecki Feb. 1, 2018, 11:48 a.m. UTC | #7
On 2018-01-31 17:14, Hante Meuleman wrote:
> It is an 802.2 frame, more specifically a LLC XID frames. So why it 
> exists?
> And more over, why would we crash as an result? Decoding info can be 
> found
> here:
> 
> https://www.cisco.com/c/en/us/support/docs/ibm-technologies/logical-link-control-llc/12247-45.html#con3
> 
> The frame was likely sent by the stack from remote site PC, should be
> possible to capture with tcpdump.
> 
> I've seen these frames before, but don’t know what they are for. The 
> frame
> appears to be correctly encoded. The ethertype, is not a type, but a 
> len
> field. The only protocol with such a short len allowed is llc, see also
> 
> https://www.savvius.com/networking-glossary/ethernet/frame_formats/
> 
> So it is 802.2 (also known as LLC)

This was actually quite helpful, thanks! Googling for "802.11 LLC XID
association" pointed me to some Google-indexed books:
1) Internet Protocols: Advances, Technologies and Applications
2) Broadband Wireless Access and Local Networks: Mobile WiMax and WiFi

Both of them describe IAPP standard which appears as IEEE 802.11f on
Wikipedia. It seems to be some old & obsolete roaming standard that was
replaced by 802.11r.

There is ADD operation defined by the 802.11f which is triggered "when a
station is newly associated". It also says "The frame is sent using a
MAC source address equal to the MAC address of the station".

So far it seems to match what I'm seeing. My guess is that Broadcom's
firmware includes some kind of support for the 802.11f. I'm still not
sure if that is really firmware's responsibility to handle that though.
Arend Van Spriel Feb. 1, 2018, 12:23 p.m. UTC | #8
On 2/1/2018 12:48 PM, Rafał Miłecki wrote:
> On 2018-01-31 17:14, Hante Meuleman wrote:
>> It is an 802.2 frame, more specifically a LLC XID frames. So why it
>> exists?
>> And more over, why would we crash as an result? Decoding info can be
>> found
>> here:
>>
>> https://www.cisco.com/c/en/us/support/docs/ibm-technologies/logical-link-control-llc/12247-45.html#con3
>>
>>
>> The frame was likely sent by the stack from remote site PC, should be
>> possible to capture with tcpdump.
>>
>> I've seen these frames before, but don’t know what they are for. The
>> frame
>> appears to be correctly encoded. The ethertype, is not a type, but a len
>> field. The only protocol with such a short len allowed is llc, see also
>>
>> https://www.savvius.com/networking-glossary/ethernet/frame_formats/
>>
>> So it is 802.2 (also known as LLC)
>
> This was actually quite helpful, thanks! Googling for "802.11 LLC XID
> association" pointed me to some Google-indexed books:
> 1) Internet Protocols: Advances, Technologies and Applications
> 2) Broadband Wireless Access and Local Networks: Mobile WiMax and WiFi
>
> Both of them describe IAPP standard which appears as IEEE 802.11f on
> Wikipedia. It seems to be some old & obsolete roaming standard that was
> replaced by 802.11r.
>
> There is ADD operation defined by the 802.11f which is triggered "when a
> station is newly associated". It also says "The frame is sent using a
> MAC source address equal to the MAC address of the station".
>
> So far it seems to match what I'm seeing. My guess is that Broadcom's
> firmware includes some kind of support for the 802.11f. I'm still not
> sure if that is really firmware's responsibility to handle that though.

I was just writing up a reply. It is indeed an IAPP packet. So it is a 
802.11f packet and our firmware implements the 802.11 stack. What makes 
you think it is not firmware responsibility. It goes along with MLME 
states. The firmware change has been made centuries ago as far as I can 
tell.

Anyway, the packet should not have been sent back to us as it will 
result in intended disassociate. So what kernel are you running on. I am 
not seeing it on 4.4 kernel, but I am in the middle of another debugging 
session. However, I was able to associate just fine.

Regards,
Arend
diff mbox

Patch

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]);