[PATCH] wifi: prevent A-MSDU attacks in mesh networks

[PATCH] wifi: prevent A-MSDU attacks in mesh networks

[Mathy Vanhoef]

This patch is a mitigation to prevent the A-MSDU spoofing vulnerability
for mesh networks. The initial update to the IEEE 802.11 standard, in
response to the FragAttacks, missed this case (CVE-2025-27558). It can
be considered a variant of CVE-2020-24588 but for mesh networks.

This patch tries to detect if a standard MSDU was turned into an A-MSDU
by an adversary. This is done by parsing a received A-MSDU as a standard
MSDU, calculating the length of the Mesh Control header, and seeing if
the 6 bytes after this header equal the start of an rfc1042 header. If
equal, this is a strong indication of an ongoing attack attempt.

This defense was tested with mac80211_hwsim against a mesh network that
uses an empty Mesh Address Extension field, i.e., when four addresses
are used, and when using a 12-byte Mesh Address Extension field, i.e.,
when six addresses are used. Functionality of normal MSDUs and A-MSDUs
was also tested, and confirmed working, when using both an empty and
12-byte Mesh Address Extension field.

It was also tested with mac80211_hwsim that A-MSDU attacks in non-mesh
networks keep being detected and prevented.

Note that the vulnerability being patched, and the defense being
implemented, was also discussed in the following paper and in the
following IEEE 802.11 presentation:

https://papers.mathyvanhoef.com/wisec2025.pdf
https://mentor.ieee.org/802.11/dcn/25/11-25-0949-00-000m-a-msdu-mesh-spoof-protection.docx

Signed-off-by: Mathy Vanhoef <Mathy.Vanhoef@kuleuven.be>
---
 net/wireless/util.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index ed868c0f7..639c75c74 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -820,6 +820,31 @@ bool ieee80211_is_valid_amsdu(struct sk_buff *skb, u8 mesh_hdr)
 }
 EXPORT_SYMBOL(ieee80211_is_valid_amsdu);
 
+static bool
+is_amsdu_aggregation_attack(struct ethhdr *eth, struct sk_buff *skb,
+			    enum nl80211_iftype iftype)
+{
+	int offset;
+
+	/* Non-mesh case can be directly compared */
+	if (iftype != NL80211_IFTYPE_MESH_POINT)
+		return ether_addr_equal(eth->h_dest, rfc1042_header);
+
+	offset = __ieee80211_get_mesh_hdrlen(eth->h_dest[0]);
+	if (offset == 6) {
+		/* Mesh case with empty address extension field */
+		return ether_addr_equal(eth->h_source, rfc1042_header);
+	} else if (offset + ETH_ALEN <= skb->len) {
+		/* Mesh case with non-empty address extension field */
+		u8 temp[ETH_ALEN];
+
+		skb_copy_bits(skb, offset, temp, ETH_ALEN);
+		return ether_addr_equal(temp, rfc1042_header);
+	}
+
+	return false;
+}
+
 void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
 			      const u8 *addr, enum nl80211_iftype iftype,
 			      const unsigned int extra_headroom,
@@ -861,8 +886,10 @@ void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
 		/* the last MSDU has no padding */
 		if (subframe_len > remaining)
 			goto purge;
-		/* mitigate A-MSDU aggregation injection attacks */
-		if (ether_addr_equal(hdr.eth.h_dest, rfc1042_header))
+		/* mitigate A-MSDU aggregation injection attacks, to be
+		 * checked when processing first subframe (offset == 0).
+		 */
+		if (offset == 0 && is_amsdu_aggregation_attack(&hdr.eth, skb, iftype))
 			goto purge;
 
 		offset += sizeof(struct ethhdr);
-- 
2.25.1

Re: [PATCH] wifi: prevent A-MSDU attacks in mesh networks

[Johannes Berg]

> +static bool
> +is_amsdu_aggregation_attack(struct ethhdr *eth, struct sk_buff *skb,
> +			    enum nl80211_iftype iftype)
> +{
> +	int offset;
> +
> +	/* Non-mesh case can be directly compared */
> +	if (iftype != NL80211_IFTYPE_MESH_POINT)
> +		return ether_addr_equal(eth->h_dest, rfc1042_header);
> +
> +	offset = __ieee80211_get_mesh_hdrlen(eth->h_dest[0]);

This seems awkward? Not only was this calculated by the caller before:

                if (iftype == NL80211_IFTYPE_MESH_POINT)
                        mesh_len = __ieee80211_get_mesh_hdrlen(hdr.flags);

but also h_dest[0] is just taking advantage of the aliasing when we
already had a union in the caller to avoid exactly that.

We could just pass 'mesh_len' or hdr.flags from the caller? I'd prefer
mesh_len, and perhaps also changing the code to pass mesh_len to
ieee80211_amsdu_subframe_length() instead of recalculating it, since
it's not obvious (without looking into that) right now that we even
check for the necessary length of the frame.

> +	if (offset == 6) {
> +		/* Mesh case with empty address extension field */
> +		return ether_addr_equal(eth->h_source, rfc1042_header);
> +	} else if (offset + ETH_ALEN <= skb->len) {

And it looks like you didn't really understand that either, or am I
completely confused? I don't see how this test could ever fail:

We previously have

                len = ieee80211_amsdu_subframe_length(&hdr.eth.h_proto, hdr.flags,
                                                      mesh_control);
                subframe_len = sizeof(struct ethhdr) + len;
                padding = (4 - subframe_len) & 0x3;

                /* the last MSDU has no padding */
                if (subframe_len > remaining)
                        goto purge;

where 'len' includes __ieee80211_get_mesh_hdrlen() if the mesh_control
is non-zero (where admittedly it's a bit messy to have different kinds
of checks for mesh - iftype and mesh_control), so we definitely have a
full ethhdr after the 'offset' you calculated?

Or maybe it should just not be separated out into a new function, then
it might be easier to see that indeed the length check isn't necessary,
and also easier to reuse the mesh_len.

In fact, given that mesh_len==0 for non-mesh, doesn't that make the
change at least theoretically simply collapse down to pulling 6 bytes at
mesh_len offset and comparing those? In practice probably better to
compare against the already-pulled bytes if mesh_len==0, but that'd also
be simpler to understand as a check?

I think we probably want some validation here with iftype mesh vs.
mesh_control not being 0 and really now looking at this I wonder why I
let Felix get away without an enum ... but we can fix all that
separately.

johannes

Re: [PATCH] wifi: prevent A-MSDU attacks in mesh networks

[Mathy Vanhoef]

On 6/16/25 13:27, Johannes Berg wrote:
> 
>> +static bool
>> +is_amsdu_aggregation_attack(struct ethhdr *eth, struct sk_buff *skb,
>> +			    enum nl80211_iftype iftype)
>> +{
>> +	int offset;
>> +
>> +	/* Non-mesh case can be directly compared */
>> +	if (iftype != NL80211_IFTYPE_MESH_POINT)
>> +		return ether_addr_equal(eth->h_dest, rfc1042_header);
>> +
>> +	offset = __ieee80211_get_mesh_hdrlen(eth->h_dest[0]);
> 
> This seems awkward? Not only was this calculated by the caller before:
> 
>                  if (iftype == NL80211_IFTYPE_MESH_POINT)
>                          mesh_len = __ieee80211_get_mesh_hdrlen(hdr.flags);
> 
> but also h_dest[0] is just taking advantage of the aliasing when we
> already had a union in the caller to avoid exactly that.

Here hdr.flags is not an alias of h_dest[0].

The caller is parsing the frame as an A-MSDU, and because of that, the 
caller assumes that the Mesh Control header starts after the A-MSDU 
subframe header, i.e., after the `struct ethhdr`.

The added function is_amsdu_aggregation_attack is instead treating the 
frame as if it were an MSDU, to detect if an attacker flipped the 
"is-AMSDU" flag of an MSDU frame. For mesh networks, the MSDU 
immediately starts with the Mesh Control header.

So the caller is passing the byte at offset 14 as argument to 
__ieee80211_get_mesh_hdrlen while the added function is passing the byte 
at offset 0 as an argument to __ieee80211_get_mesh_hdrlen.

> We could just pass 'mesh_len' or hdr.flags from the caller? I'd prefer
> mesh_len, and perhaps also changing the code to pass mesh_len to
> ieee80211_amsdu_subframe_length() instead of recalculating it, since
> it's not obvious (without looking into that) right now that we even
> check for the necessary length of the frame.
> 
>> +	if (offset == 6) {
>> +		/* Mesh case with empty address extension field */
>> +		return ether_addr_equal(eth->h_source, rfc1042_header);
>> +	} else if (offset + ETH_ALEN <= skb->len) {
> 
> And it looks like you didn't really understand that either, or am I
> completely confused? I don't see how this test could ever fail:

Those checks are largely irrelevant. The caller performs those length 
checks based on an A-MSDU while the added function parses the frame as 
an MSDU. This means the caller is checking a different (optional) Mesh 
Address Extension header length.

> We previously have
> 
>                  len = ieee80211_amsdu_subframe_length(&hdr.eth.h_proto, hdr.flags,
>                                                        mesh_control);
>                  subframe_len = sizeof(struct ethhdr) + len;
>                  padding = (4 - subframe_len) & 0x3;
> 
>                  /* the last MSDU has no padding */
>                  if (subframe_len > remaining)
>                          goto purge;
> 
> where 'len' includes __ieee80211_get_mesh_hdrlen() if the mesh_control
> is non-zero (where admittedly it's a bit messy to have different kinds
> of checks for mesh - iftype and mesh_control), so we definitely have a
> full ethhdr after the 'offset' you calculated?

Is there really a *guarantee* that mesh_control is non-zero? A 
misbehaving mesh client could still be sending frames such that
__ieee80211_rx_h_amsdu will set amsdu_mesh_control to zero, even though 
its iftype is a mesh client.

 From what I see, this code in the caller guarantees space for the 
following bytes:
- sizeof(struct ethhdr): 6+6+2 bytes A-MSDU subframe header
- ieee80211_amsdu_subframe_length: can return zero
- padding: presence of enough padding is not yet checked
That's 14 bytes that are guaranteed to be there when calling the 
function is_amsdu_aggregation_attack.

Note that the earlier check `if (copy_len > remaining)` actually 
guarantees 15 bytes to be present.

Then is_amsdu_aggregation_attack needs the following to be present:
- Mesh Control field and Mesh Address Extension field with a combined 
length equal to 'offset'
- The first 6 bytes of the LCC/SNAP header (when treating the frame as 
an MSDU)
When 'offset' is 6 this means we know there is enough space. But when 
offset > 6 there has to be a length check.

I'd actually be inclined to just always perform this length check for 
mesh clients in is_amsdu_aggregation_attack, since this function is 
parsing the frame as an MSDU instead, and because it's too easy to make 
a mistake in these length checks (either now or in the future, e.g., if 
the caller code ever changes).

> Or maybe it should just not be separated out into a new function, then
> it might be easier to see that indeed the length check isn't necessary,
> and also easier to reuse the mesh_len.

I'd still be inclined to keep it a different function. The new function 
is parsing the frame as an MSDU instead, to detect a possible attack. 
Perhaps a comment can be added that this function is now treating the 
frame as an MSDU instead.

> In fact, given that mesh_len==0 for non-mesh, doesn't that make the
> change at least theoretically simply collapse down to pulling 6 bytes at
> mesh_len offset and comparing those? In practice probably better to
> compare against the already-pulled bytes if mesh_len==0, but that'd also
> be simpler to understand as a check?

In non-mesh networks the check is indeed much easier. The existing code 
always assumed non-mesh networks and did a comparison against the 
already-pulled bytes. This comparison against the already-pulled bytes 
is still present at the start of is_amsdu_aggregation_attack

> I think we probably want some validation here with iftype mesh vs.
> mesh_control not being 0 and really now looking at this I wonder why I
> let Felix get away without an enum ... but we can fix all that
> separately.
> 
> johannes

Re: [PATCH] wifi: prevent A-MSDU attacks in mesh networks

[Johannes Berg]

Hi,

Uh, sorry, looks like I dropped the ball on this.

On Mon, 2025-06-16 at 18:19 +0200, Mathy Vanhoef wrote:
> On 6/16/25 13:27, Johannes Berg wrote:
> > 
> > > +static bool
> > > +is_amsdu_aggregation_attack(struct ethhdr *eth, struct sk_buff *skb,
> > > +			    enum nl80211_iftype iftype)
> > > +{
> > > +	int offset;
> > > +
> > > +	/* Non-mesh case can be directly compared */
> > > +	if (iftype != NL80211_IFTYPE_MESH_POINT)
> > > +		return ether_addr_equal(eth->h_dest, rfc1042_header);
> > > +
> > > +	offset = __ieee80211_get_mesh_hdrlen(eth->h_dest[0]);
> > 
> > This seems awkward? Not only was this calculated by the caller before:
> > 
> >                  if (iftype == NL80211_IFTYPE_MESH_POINT)
> >                          mesh_len = __ieee80211_get_mesh_hdrlen(hdr.flags);
> > 
> > but also h_dest[0] is just taking advantage of the aliasing when we
> > already had a union in the caller to avoid exactly that.
> 
> Here hdr.flags is not an alias of h_dest[0].

Huh, indeed, I misread the struct as a union.

> The caller is parsing the frame as an A-MSDU, and because of that, the 
> caller assumes that the Mesh Control header starts after the A-MSDU 
> subframe header, i.e., after the `struct ethhdr`.
> 
> The added function is_amsdu_aggregation_attack is instead treating the 
> frame as if it were an MSDU, to detect if an attacker flipped the 
> "is-AMSDU" flag of an MSDU frame. For mesh networks, the MSDU 
> immediately starts with the Mesh Control header.
> 
> So the caller is passing the byte at offset 14 as argument to 
> __ieee80211_get_mesh_hdrlen while the added function is passing the byte 
> at offset 0 as an argument to __ieee80211_get_mesh_hdrlen.

Right.

> > We could just pass 'mesh_len' or hdr.flags from the caller? I'd prefer
> > mesh_len, and perhaps also changing the code to pass mesh_len to
> > ieee80211_amsdu_subframe_length() instead of recalculating it, since
> > it's not obvious (without looking into that) right now that we even
> > check for the necessary length of the frame.
> > 
> > > +	if (offset == 6) {
> > > +		/* Mesh case with empty address extension field */
> > > +		return ether_addr_equal(eth->h_source, rfc1042_header);
> > > +	} else if (offset + ETH_ALEN <= skb->len) {
> > 
> > And it looks like you didn't really understand that either, or am I
> > completely confused? I don't see how this test could ever fail:
> 
> Those checks are largely irrelevant. The caller performs those length 
> checks based on an A-MSDU while the added function parses the frame as 
> an MSDU. This means the caller is checking a different (optional) Mesh 
> Address Extension header length.

Ah, so the caller check could've resulted in no AE flags (6 bytes) and
the other one 18 bytes, which is a large enough difference that we _do_
need to check explicitly I guess.

> > We previously have
> > 
> >                  len = ieee80211_amsdu_subframe_length(&hdr.eth.h_proto, hdr.flags,
> >                                                        mesh_control);
> >                  subframe_len = sizeof(struct ethhdr) + len;
> >                  padding = (4 - subframe_len) & 0x3;
> > 
> >                  /* the last MSDU has no padding */
> >                  if (subframe_len > remaining)
> >                          goto purge;
> > 
> > where 'len' includes __ieee80211_get_mesh_hdrlen() if the mesh_control
> > is non-zero (where admittedly it's a bit messy to have different kinds
> > of checks for mesh - iftype and mesh_control), so we definitely have a
> > full ethhdr after the 'offset' you calculated?
> 
> Is there really a *guarantee* that mesh_control is non-zero? A 
> misbehaving mesh client could still be sending frames such that
> __ieee80211_rx_h_amsdu will set amsdu_mesh_control to zero, even though 
> its iftype is a mesh client.

Yeah, there should be, but I agree it's a mess now.

>  From what I see, this code in the caller guarantees space for the 
> following bytes:
> - sizeof(struct ethhdr): 6+6+2 bytes A-MSDU subframe header
> - ieee80211_amsdu_subframe_length: can return zero
> - padding: presence of enough padding is not yet checked
> That's 14 bytes that are guaranteed to be there when calling the 
> function is_amsdu_aggregation_attack.
> 
> Note that the earlier check `if (copy_len > remaining)` actually 
> guarantees 15 bytes to be present.
> 
> Then is_amsdu_aggregation_attack needs the following to be present:
> - Mesh Control field and Mesh Address Extension field with a combined 
> length equal to 'offset'
> - The first 6 bytes of the LCC/SNAP header (when treating the frame as 
> an MSDU)
> When 'offset' is 6 this means we know there is enough space. But when 
> offset > 6 there has to be a length check.

Right.

> I'd actually be inclined to just always perform this length check for 
> mesh clients in is_amsdu_aggregation_attack, since this function is 
> parsing the frame as an MSDU instead, and because it's too easy to make 
> a mistake in these length checks (either now or in the future, e.g., if 
> the caller code ever changes).

Makes sense.

> > Or maybe it should just not be separated out into a new function, then
> > it might be easier to see that indeed the length check isn't necessary,
> > and also easier to reuse the mesh_len.
> 
> I'd still be inclined to keep it a different function. The new function 
> is parsing the frame as an MSDU instead, to detect a possible attack. 
> Perhaps a comment can be added that this function is now treating the 
> frame as an MSDU instead.

Yeah, I'll add something.

> > In fact, given that mesh_len==0 for non-mesh, doesn't that make the
> > change at least theoretically simply collapse down to pulling 6 bytes at
> > mesh_len offset and comparing those? In practice probably better to
> > compare against the already-pulled bytes if mesh_len==0, but that'd also
> > be simpler to understand as a check?
> 
> In non-mesh networks the check is indeed much easier. The existing code 
> always assumed non-mesh networks and did a comparison against the 
> already-pulled bytes. This comparison against the already-pulled bytes 
> is still present at the start of is_amsdu_aggregation_attack
> 

Right.

johannes

Re: [PATCH] wifi: prevent A-MSDU attacks in mesh networks

[Mathy Vanhoef]

Hello,

Looking back at this, the added function could have indeed used extra 
documentation, since this is a special case. Others trying to understand 
this function will likely have the same questions otherwise. A first 
suggestion as a comment to add is the following:

/*
  * Detects if an MSDU frame was maliciously converted into an A-MSDU
  * frame by an adversary. This is done by parsing the received frame
  * as if it were a regular MSDU, even though the A-MSDU flag is set.
  *
  * For non-mesh interfaces, detection involves checking whether the
  * payload, when interpreted as an MSDU, begins with a valid RFC1042
  * header. This is done by comparing the A-MSDU subheader's destination
  * address to the start of the RFC1042 header.
  *
  * For mesh interfaces, the MSDU includes a 6-byte Mesh Control field
  * and an optional variable-length Mesh Address Extension field before
  * the RFC1042 header. The position of the RFC1042 header must therefore
  * be calculated based on the mesh header length.
  *
  * Since this function intentionally parses an A-MSDU frame as an MSDU,
  * it only assumes that the A-MSDU subframe header is present, and
  * beyond this it performs its own bounds checks under the assumption
  * that the frame is instead parsed as a non-aggregated MSDU.
  */

It's a longer comment, but has the most important info, you can see what 
makes sense to add/remove. Let me know if you want me to resubmit a 
patch or whether you can include this directly.

Best regards,
Mathy

On 6/24/25 10:10, Johannes Berg wrote:
> Hi,
> 
> Uh, sorry, looks like I dropped the ball on this.
> 
> On Mon, 2025-06-16 at 18:19 +0200, Mathy Vanhoef wrote:
>> On 6/16/25 13:27, Johannes Berg wrote:
>>>
>>>> +static bool
>>>> +is_amsdu_aggregation_attack(struct ethhdr *eth, struct sk_buff *skb,
>>>> +			    enum nl80211_iftype iftype)
>>>> +{
>>>> +	int offset;
>>>> +
>>>> +	/* Non-mesh case can be directly compared */
>>>> +	if (iftype != NL80211_IFTYPE_MESH_POINT)
>>>> +		return ether_addr_equal(eth->h_dest, rfc1042_header);
>>>> +
>>>> +	offset = __ieee80211_get_mesh_hdrlen(eth->h_dest[0]);
>>>
>>> This seems awkward? Not only was this calculated by the caller before:
>>>
>>>                   if (iftype == NL80211_IFTYPE_MESH_POINT)
>>>                           mesh_len = __ieee80211_get_mesh_hdrlen(hdr.flags);
>>>
>>> but also h_dest[0] is just taking advantage of the aliasing when we
>>> already had a union in the caller to avoid exactly that.
>>
>> Here hdr.flags is not an alias of h_dest[0].
> 
> Huh, indeed, I misread the struct as a union.
> 
>> The caller is parsing the frame as an A-MSDU, and because of that, the
>> caller assumes that the Mesh Control header starts after the A-MSDU
>> subframe header, i.e., after the `struct ethhdr`.
>>
>> The added function is_amsdu_aggregation_attack is instead treating the
>> frame as if it were an MSDU, to detect if an attacker flipped the
>> "is-AMSDU" flag of an MSDU frame. For mesh networks, the MSDU
>> immediately starts with the Mesh Control header.
>>
>> So the caller is passing the byte at offset 14 as argument to
>> __ieee80211_get_mesh_hdrlen while the added function is passing the byte
>> at offset 0 as an argument to __ieee80211_get_mesh_hdrlen.
> 
> Right.
> 
>>> We could just pass 'mesh_len' or hdr.flags from the caller? I'd prefer
>>> mesh_len, and perhaps also changing the code to pass mesh_len to
>>> ieee80211_amsdu_subframe_length() instead of recalculating it, since
>>> it's not obvious (without looking into that) right now that we even
>>> check for the necessary length of the frame.
>>>
>>>> +	if (offset == 6) {
>>>> +		/* Mesh case with empty address extension field */
>>>> +		return ether_addr_equal(eth->h_source, rfc1042_header);
>>>> +	} else if (offset + ETH_ALEN <= skb->len) {
>>>
>>> And it looks like you didn't really understand that either, or am I
>>> completely confused? I don't see how this test could ever fail:
>>
>> Those checks are largely irrelevant. The caller performs those length
>> checks based on an A-MSDU while the added function parses the frame as
>> an MSDU. This means the caller is checking a different (optional) Mesh
>> Address Extension header length.
> 
> Ah, so the caller check could've resulted in no AE flags (6 bytes) and
> the other one 18 bytes, which is a large enough difference that we _do_
> need to check explicitly I guess.
> 
>>> We previously have
>>>
>>>                   len = ieee80211_amsdu_subframe_length(&hdr.eth.h_proto, hdr.flags,
>>>                                                         mesh_control);
>>>                   subframe_len = sizeof(struct ethhdr) + len;
>>>                   padding = (4 - subframe_len) & 0x3;
>>>
>>>                   /* the last MSDU has no padding */
>>>                   if (subframe_len > remaining)
>>>                           goto purge;
>>>
>>> where 'len' includes __ieee80211_get_mesh_hdrlen() if the mesh_control
>>> is non-zero (where admittedly it's a bit messy to have different kinds
>>> of checks for mesh - iftype and mesh_control), so we definitely have a
>>> full ethhdr after the 'offset' you calculated?
>>
>> Is there really a *guarantee* that mesh_control is non-zero? A
>> misbehaving mesh client could still be sending frames such that
>> __ieee80211_rx_h_amsdu will set amsdu_mesh_control to zero, even though
>> its iftype is a mesh client.
> 
> Yeah, there should be, but I agree it's a mess now.
> 
>>   From what I see, this code in the caller guarantees space for the
>> following bytes:
>> - sizeof(struct ethhdr): 6+6+2 bytes A-MSDU subframe header
>> - ieee80211_amsdu_subframe_length: can return zero
>> - padding: presence of enough padding is not yet checked
>> That's 14 bytes that are guaranteed to be there when calling the
>> function is_amsdu_aggregation_attack.
>>
>> Note that the earlier check `if (copy_len > remaining)` actually
>> guarantees 15 bytes to be present.
>>
>> Then is_amsdu_aggregation_attack needs the following to be present:
>> - Mesh Control field and Mesh Address Extension field with a combined
>> length equal to 'offset'
>> - The first 6 bytes of the LCC/SNAP header (when treating the frame as
>> an MSDU)
>> When 'offset' is 6 this means we know there is enough space. But when
>> offset > 6 there has to be a length check.
> 
> Right.
> 
>> I'd actually be inclined to just always perform this length check for
>> mesh clients in is_amsdu_aggregation_attack, since this function is
>> parsing the frame as an MSDU instead, and because it's too easy to make
>> a mistake in these length checks (either now or in the future, e.g., if
>> the caller code ever changes).
> 
> Makes sense.
> 
>>> Or maybe it should just not be separated out into a new function, then
>>> it might be easier to see that indeed the length check isn't necessary,
>>> and also easier to reuse the mesh_len.
>>
>> I'd still be inclined to keep it a different function. The new function
>> is parsing the frame as an MSDU instead, to detect a possible attack.
>> Perhaps a comment can be added that this function is now treating the
>> frame as an MSDU instead.
> 
> Yeah, I'll add something.
> 
>>> In fact, given that mesh_len==0 for non-mesh, doesn't that make the
>>> change at least theoretically simply collapse down to pulling 6 bytes at
>>> mesh_len offset and comparing those? In practice probably better to
>>> compare against the already-pulled bytes if mesh_len==0, but that'd also
>>> be simpler to understand as a check?
>>
>> In non-mesh networks the check is indeed much easier. The existing code
>> always assumed non-mesh networks and did a comparison against the
>> already-pulled bytes. This comparison against the already-pulled bytes
>> is still present at the start of is_amsdu_aggregation_attack
>>
> 
> Right.
> 
> johannes

Re: [PATCH] wifi: prevent A-MSDU attacks in mesh networks

[Johannes Berg]

Hi,

> Looking back at this, the added function could have indeed used extra 
> documentation, since this is a special case. Others trying to understand 
> this function will likely have the same questions otherwise. A first 
> suggestion as a comment to add is the following:

[snip]

Thanks for that!
> 
> It's a longer comment, but has the most important info, you can see what 
> makes sense to add/remove. Let me know if you want me to resubmit a 
> patch or whether you can include this directly.

I'll include it.

johannes