Re: [RFC PATCH 4.4] mac80211: fix handling A-MSDUs that start with an RFC 1042 header

Re: [RFC PATCH 4.4] mac80211: fix handling A-MSDUs that start with an RFC 1042 header

[Zhengyejian (Zetta)]

On 7/17/21 19:55, Mathy Vanhoef wrote:
> On 7/16/21 11:11 AM, Zheng Yejian wrote:
> > In v4.4, commit e76511a6fbb5 ("mac80211: properly handle A-MSDUs that
> > start with an RFC 1042 header") looks like an incomplete backport.
> >
> > There is no functional changes in the commit, since
> > __ieee80211_data_to_8023() which defined in net/wireless/util.c is
> > only called by ieee80211_data_to_8023() and parameter 'is_amsdu' is
> > always input as false.
> 
> I don't think there's a problem here. The core commit that prevents the
> A-MSDU attack is "[PATCH 04/18] cfg80211: mitigate A-MSDU aggregation
> attacks":
> https://lore.kernel.org/linux-
> wireless/20210511200110.25d93176ddaf.I9e265b597f2cd23eb44573f35b62594
> 7b386a9de@changeid/
> 
> That commit states: "for kernel 4.9 and above this patch depends on
> "mac80211: properly handle A-MSDUs that start with a rfc1042 header".
> Otherwise this patch has no impact and attacks will remain possible."
> 
> Put differently, when patching v4.4 there was in fact no need to
> backport the patch that we're discussing here. So it makes sense that
> the "backported" patches causes no functional changes.
> 
> Section 3.6 of https://papers.mathyvanhoef.com/usenix2021.pdf briefly
> discusses the wrong behavior of Linux 4.9+ that this patch tries to fix:
> "Linux 4.9 and above .. strip away the first 8 bytes of an A-MSDU frame
> if these bytes look like a valid LLC/SNAP header, and then further
> process the frame. This behavior is not compliant with the 802.11 standard."
> 

How about linux 4.9 below, are they compliant  with 802.11 standard or not?
Would they need additional patches to mitigate the aggregation attack? 
I know little about 802.11 standard, sorry for that : (

> That said, I didn't yet run the test tool against a patched 4.4 kernel,
> so I hope my understanding of this code in this version is correct.
> 
> Best regards,
> Mathy

Thanks,
Zheng Yejian

Re: [RFC PATCH 4.4] mac80211: fix handling A-MSDUs that start with an RFC 1042 header

[Mathy Vanhoef]

>> Section 3.6 of https://papers.mathyvanhoef.com/usenix2021.pdf briefly
>> discusses the wrong behavior of Linux 4.9+ that this patch tries to fix:
>> "Linux 4.9 and above .. strip away the first 8 bytes of an A-MSDU frame
>> if these bytes look like a valid LLC/SNAP header, and then further
>> process the frame. This behavior is not compliant with the 802.11 standard."
>>
> 
> How about linux 4.9 below, are they compliant  with 802.11 standard or not?

They are compliant.

> Would they need additional patches to mitigate the aggregation attack?

They need the backport of "[PATCH 04/18] cfg80211: mitigate A-MSDU
aggregation attacks" to mitigate attacks. This patch has been backported
to 4.4:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v4.4.275&id=daea7ff51861cec93ff7f561095d9048b673b51f

So if you take all the patches that have been backported to 4.4 you
should be OK.

Cheers,
Mathy

Re: [RFC PATCH 4.4] mac80211: fix handling A-MSDUs that start with an RFC 1042 header

[Zhengyejian (Zetta)]

> > How about linux 4.9 below, are they compliant  with 802.11 standard or not?
> 
> They are compliant.
> 
> > Would they need additional patches to mitigate the aggregation attack?
> 
> They need the backport of "[PATCH 04/18] cfg80211: mitigate A-MSDU
> aggregation attacks" to mitigate attacks. This patch has been backported
> to 4.4:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=
> v4.4.275&id=daea7ff51861cec93ff7f561095d9048b673b51f
> 
> So if you take all the patches that have been backported to 4.4 you
> should be OK.
> 
> Cheers,
> Mathy

I get it now, thanks.

Best regards,
Zheng Yejian

Re: [RFC PATCH 4.4] mac80211: fix handling A-MSDUs that start with an RFC 1042 header

[Mathy Vanhoef]

On 7/16/21 11:11 AM, Zheng Yejian wrote:
> In v4.4, commit e76511a6fbb5 ("mac80211: properly handle A-MSDUs that
> start with an RFC 1042 header") looks like an incomplete backport.
> 
> There is no functional changes in the commit, since
> __ieee80211_data_to_8023() which defined in net/wireless/util.c is
> only called by ieee80211_data_to_8023() and parameter 'is_amsdu' is
> always input as false.

I don't think there's a problem here. The core commit that prevents the
A-MSDU attack is "[PATCH 04/18] cfg80211: mitigate A-MSDU aggregation
attacks":
https://lore.kernel.org/linux-wireless/20210511200110.25d93176ddaf.I9e265b597f2cd23eb44573f35b625947b386a9de@changeid/

That commit states: "for kernel 4.9 and above this patch depends on
"mac80211: properly handle A-MSDUs that start with a rfc1042 header".
Otherwise this patch has no impact and attacks will remain possible."

Put differently, when patching v4.4 there was in fact no need to
backport the patch that we're discussing here. So it makes sense that
the "backported" patches causes no functional changes.

Section 3.6 of https://papers.mathyvanhoef.com/usenix2021.pdf briefly
discusses the wrong behavior of Linux 4.9+ that this patch tries to fix:
"Linux 4.9 and above .. strip away the first 8 bytes of an A-MSDU frame
if these bytes look like a valid LLC/SNAP header, and then further
process the frame. This behavior is not compliant with the 802.11 standard."

That said, I didn't yet run the test tool against a patched 4.4 kernel,
so I hope my understanding of this code in this version is correct.

Best regards,
Mathy

[RFC PATCH 4.4] mac80211: fix handling A-MSDUs that start with an RFC 1042 header

[Zheng Yejian]

In v4.4, commit e76511a6fbb5 ("mac80211: properly handle A-MSDUs that
start with an RFC 1042 header") looks like an incomplete backport.

There is no functional changes in the commit, since
__ieee80211_data_to_8023() which defined in net/wireless/util.c is
only called by ieee80211_data_to_8023() and parameter 'is_amsdu' is
always input as false.

By comparing with its upstream, I found that following snippet has not
been backported:
  > --- a/net/mac80211/rx.c
  > +++ b/net/mac80211/rx.c
  > @@ -2682,7 +2682,7 @@ __ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx, u8 data_offset)
  >  	if (ieee80211_data_to_8023_exthdr(skb, &ethhdr,
  >  					  rx->sdata->vif.addr,
  >  					  rx->sdata->vif.type,
  > -					  data_offset))
  > +					  data_offset, true))
  >  		return RX_DROP_UNUSABLE;

I think that's where really causing changes and also needs to be
backported, so I try to do it.

Fixes: e76511a6fbb5 ("mac80211: properly handle A-MSDUs that start with an RFC 1042 header")
Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
---
 net/wireless/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 84c0a96b3cb6d..a2b35e6619697 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -664,7 +664,7 @@ void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
 	u8 dst[ETH_ALEN], src[ETH_ALEN];
 
 	if (has_80211_header) {
-		err = ieee80211_data_to_8023(skb, addr, iftype);
+		err = __ieee80211_data_to_8023(skb, addr, iftype, true);
 		if (err)
 			goto out;
 
-- 
2.31.1