> +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
