| Message ID | 20180130090922.30346-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../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
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. Regards, Arend > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../wireless/broadcom/brcm80211/brcmfmac/core.c | 46 ++++++++++++++++++++++ > 1 file changed, 46 insertions(+)
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 <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. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> .../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?
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
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++;