Subject: Re: [PATCH] brcmfmac: detect & reject faked packet generated by a
 firmware
To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>,
        Kalle Valo <kvalo@codeaurora.org>
References: <20180130090922.30346-1-zajec5@gmail.com>
Cc: Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Chi-Hsien Lin <chi-hsien.lin@cypress.com>,
        Wright Feng <wright.feng@cypress.com>,
        Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>,
        linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        brcm80211-dev-list@cypress.com,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
From: Arend van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <5A705741.6050402@broadcom.com> (sfid-20180130_123018_657109_7F218D05)
Date: Tue, 30 Jan 2018 12:30:09 +0100
MIME-Version: 1.0
In-Reply-To: <20180130090922.30346-1-zajec5@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Sender: linux-wireless-owner@vger.kernel.org

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