[RFC PATCH] ath10k: fix RX of frames with broken FCS in monitor mode

[RFC PATCH] ath10k: fix RX of frames with broken FCS in monitor mode

[Linus Lüssing]

From: Linus Lüssing <ll@simonwunderlich.de>

So far, frames were forwarded regardless of the FCS correctness leading
to userspace applications listening on the monitor mode interface to
receive potentially broken frames, even with the "fcsfail" flag unset.

By default, with the "fcsfail" flag of a monitor mode interface
unset, frames with FCS errors should be dropped. With this patch, the
fcsfail flag is taken into account correctly.

Signed-off-by: Linus Lüssing <ll@simonwunderlich.de>
---
This was tested on an Open Mesh A41 device, featuring a QCA4019. And
with this firmware:

https://www.candelatech.com/downloads/ath10k-4019-10-4b/firmware-5-ct-full-community-12.bin-lede.011

But from looking at the code it seems that the vanilla ath10k has the
same issue, therefore submitting it here.

I'm also not that familiar with the ath10k code yet. So not 100% sure if
it's the right place for this check. Therefore sending as RFC.
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 53f1095de8ff..ce0a16ebb8bb 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1285,6 +1285,12 @@ static void ath10k_process_rx(struct ath10k *ar, struct sk_buff *skb)
 
 	status = IEEE80211_SKB_RXCB(skb);
 
+	if (ar->monitor && !(ar->filter_flags & FIF_FCSFAIL) &&
+	    status->flag & RX_FLAG_FAILED_FCS_CRC) {
+		dev_kfree_skb_any(skb);
+		return;
+	}
+
 	ath10k_dbg(ar, ATH10K_DBG_DATA,
 		   "rx skb %pK len %u peer %pM %s %s sn %u %s%s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
 		   skb,
-- 
2.24.0.rc2


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC PATCH] ath10k: fix RX of frames with broken FCS in monitor mode

[Ben Greear]

On 11/1/19 4:11 AM, Linus Lüssing wrote:
> From: Linus Lüssing <ll@simonwunderlich.de>
> 
> So far, frames were forwarded regardless of the FCS correctness leading
> to userspace applications listening on the monitor mode interface to
> receive potentially broken frames, even with the "fcsfail" flag unset.
> 
> By default, with the "fcsfail" flag of a monitor mode interface
> unset, frames with FCS errors should be dropped. With this patch, the
> fcsfail flag is taken into account correctly.
> 
> Signed-off-by: Linus Lüssing <ll@simonwunderlich.de>
> ---
> This was tested on an Open Mesh A41 device, featuring a QCA4019. And
> with this firmware:
> 
> https://www.candelatech.com/downloads/ath10k-4019-10-4b/firmware-5-ct-full-community-12.bin-lede.011
> 
> But from looking at the code it seems that the vanilla ath10k has the
> same issue, therefore submitting it here.
> 
> I'm also not that familiar with the ath10k code yet. So not 100% sure if
> it's the right place for this check. Therefore sending as RFC.
> ---
>   drivers/net/wireless/ath/ath10k/htt_rx.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> index 53f1095de8ff..ce0a16ebb8bb 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -1285,6 +1285,12 @@ static void ath10k_process_rx(struct ath10k *ar, struct sk_buff *skb)
>   
>   	status = IEEE80211_SKB_RXCB(skb);
>   
> +	if (ar->monitor && !(ar->filter_flags & FIF_FCSFAIL) &&
> +	    status->flag & RX_FLAG_FAILED_FCS_CRC) {
> +		dev_kfree_skb_any(skb);
> +		return;
> +	}

Maybe worth adding a counter like 'rx_drop_crc' to the ath10k_debug struct and increment it here
and also show in debugfs and/or ethtool stats?

And, maybe no check for ar->monitor, in case somehow the frame is still received
with bad CRC even without monitor mode?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC PATCH] ath10k: fix RX of frames with broken FCS in monitor mode

[Linus Lüssing]

On Fri, Nov 01, 2019 at 08:46:53AM -0700, Ben Greear wrote:
> > diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> > index 53f1095de8ff..ce0a16ebb8bb 100644
> > --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> > +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> > @@ -1285,6 +1285,12 @@ static void ath10k_process_rx(struct ath10k *ar, struct sk_buff *skb)
> >   	status = IEEE80211_SKB_RXCB(skb);
> > +	if (ar->monitor && !(ar->filter_flags & FIF_FCSFAIL) &&
> > +	    status->flag & RX_FLAG_FAILED_FCS_CRC) {
> > +		dev_kfree_skb_any(skb);
> > +		return;
> > +	}
> 
> Maybe worth adding a counter like 'rx_drop_crc' to the ath10k_debug struct and increment it here
> and also show in debugfs and/or ethtool stats?

Ok.

> 
> And, maybe no check for ar->monitor, in case somehow the frame is still received
> with bad CRC even without monitor mode?

I think ath9k is also checking for "ah->is_monitoring" here:

https://elixir.bootlin.com/linux/v5.3.8/source/drivers/net/wireless/ath/ath9k/common.c#L96

And I didn't want to divert from this. Should I remove it anyway?

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k