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
