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
