LinuxLists.cc - [PATCH] brcmfmac: detect & reject faked packet generated by a firmware

2018-01-30 09:09:36

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

From: Rafał Miłecki <[email protected]>

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 <[email protected]>
---
.../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
+}
+
void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
{
+ if (!brcmf_is_valid_skb(skb)) {
+ brcmu_pkt_buf_free_skb(skb);
+ return;
+ }
+
if (skb->pkt_type == PACKET_MULTICAST)
ifp->ndev->stats.multicast++;

--
2.11.0

2018-01-30 11:47:45

by Arend Van Spriel

[permalink] [raw]

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

2018-01-31 18:02:20

by Arend Van Spriel

[permalink] [raw]

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

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

2018-01-31 14:19:58

by Arend Van Spriel

[permalink] [raw]

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 <[email protected]>
>>>
>>> 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);
> }
>

2018-01-31 16:14:41

by Hante Meuleman

[permalink] [raw]

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

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-co=
ntrol-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=E2=80=99t know what they are for. Th=
e 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:[email protected]]
Sent: Wednesday, January 31, 2018 3:20 PM
To: Rafa=C5=82 Mi=C5=82ecki
Cc: Rafa=C5=82 Mi=C5=82ecki; Kalle Valo; Franky Lin; Hante Meuleman; Chi-Hs=
ien Lin;
Wright Feng; Pieter-Paul Giesberts; [email protected];
[email protected]; [email protected]
Subject: Re: [PATCH] brcmfmac: detect & reject faked packet generated by a
firmware

On 1/31/2018 2:14 PM, Rafa=C5=82 Mi=C5=82ecki wrote:
> On 2018-01-30 12:47, Arend van Spriel wrote:
>> On 1/30/2018 10:09 AM, Rafa=C5=82 Mi=C5=82ecki wrote:
>>> From: Rafa=C5=82 Mi=C5=82ecki <[email protected]>
>>>
>>> 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 =3D=3D 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 =3D skb->data;
> + pr_info("[%s] skb->data:\t\t", __func__);
> + for (i =3D 0; i < 32 && i < skb->len; i++) {
> + pr_cont("%02x ", data[i]);
> + if (i % 4 =3D=3D 3)
> + pr_cont(" ");
> + }
> + pr_cont("\n");
> + }
> +
> skb->protocol =3D eth_type_trans(skb, ifp->ndev);
> +
> + if (skb->len =3D=3D 6) {
> + pr_info("[%s] skb->protocol:\t0x%04x\n", __func__,
> skb->protocol);
> + }
> +
> brcmf_netif_rx(ifp, skb);
> }
>

2018-01-31 14:00:23

by Arend Van Spriel

[permalink] [raw]

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

On 1/31/2018 2:11 PM, Rafał Miłecki wrote:
>>> 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?

Those are actually netdev stats, right? Not sure I want to have those,
but if any I would say 3) fits best.

Regards,
Arend

2018-01-31 18:00:42

by Rafał Miłecki

[permalink] [raw]

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

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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../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?

2018-01-31 13:53:24

by Rafał Miłecki

[permalink] [raw]

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

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 <[email protected]>
>>
>> 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)

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

2018-01-30 11:30:13

by Arend Van Spriel

[permalink] [raw]

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

On 1/30/2018 10:09 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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.

> 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.

Regards,
Arend

2018-02-01 11:04:13

by Arend Van Spriel

[permalink] [raw]

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

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

2018-02-01 11:48:33

by Rafał Miłecki

[permalink] [raw]

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

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.

2018-02-01 11:01:33

by Rafał Miłecki

[permalink] [raw]

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

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 :)

2018-02-01 12:23:48

by Arend Van Spriel

[permalink] [raw]

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

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

2018-02-01 11:25:02

by Rafał Miłecki

[permalink] [raw]

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

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.