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
