| Message ID | 20221007090509.18503-1-nbd@nbd.name (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [6.1,1/2] wifi: cfg80211: fix ieee80211_data_to_8023_exthdr handling of small packets | expand |
On Fri, 2022-10-07 at 11:05 +0200, Felix Fietkau wrote: > STP topology change notification packets only have a payload of 7 bytes, > so they get dropped due to the skb->len < hdrlen + 8 check. > Fix this by removing skb->len based checks and instead check the return code > on the skb_copy_bits calls. > > Fixes: 2d1c304cb2d5 ("cfg80211: add function for 802.3 conversion with separate output buffer") > Reported-by: Chad Monroe <chad.monroe@smartrg.com> > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > net/wireless/util.c | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/net/wireless/util.c b/net/wireless/util.c > index 01493568a21d..35f630c6de11 100644 > --- a/net/wireless/util.c > +++ b/net/wireless/util.c > @@ -559,8 +559,6 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr, > return -1; > > hdrlen = ieee80211_hdrlen(hdr->frame_control) + data_offset; > - if (skb->len < hdrlen + 8) > - return -1; Can't decide if I'm happy with that vs. just dropping the "+ 8" ... On the one hand, we already assume at least that the header is in the linear portion for when we access it, but on the other hand it feels safer to keep the check? > @@ -628,16 +629,15 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr, > break; > } > > - skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)); > - tmp.h_proto = payload.proto; > - > - if (likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) && > + if (likely(skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)) == 0) && > + likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) && > tmp.h_proto != htons(ETH_P_AARP) && > tmp.h_proto != htons(ETH_P_IPX)) || > ether_addr_equal(payload.hdr, bridge_tunnel_header))) { > /* remove RFC1042 or Bridge-Tunnel encapsulation and > * replace EtherType */ > hdrlen += ETH_ALEN + 2; > + tmp.h_proto = payload.proto; As you can see on the last line, this is broken. Double likely() also seems weird. I think it needs to be + if (likely(skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)) == 0 && + (!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) && + payload.proto != htons(ETH_P_AARP) && + payload.proto != htons(ETH_P_IPX)) || but better if you check and resubmit. johannes
diff --git a/net/wireless/util.c b/net/wireless/util.c index 01493568a21d..35f630c6de11 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -559,8 +559,6 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr, return -1; hdrlen = ieee80211_hdrlen(hdr->frame_control) + data_offset; - if (skb->len < hdrlen + 8) - return -1; /* convert IEEE 802.11 header + possible LLC headers into Ethernet * header @@ -574,8 +572,9 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr, memcpy(tmp.h_dest, ieee80211_get_DA(hdr), ETH_ALEN); memcpy(tmp.h_source, ieee80211_get_SA(hdr), ETH_ALEN); - if (iftype == NL80211_IFTYPE_MESH_POINT) - skb_copy_bits(skb, hdrlen, &mesh_flags, 1); + if (iftype == NL80211_IFTYPE_MESH_POINT && + skb_copy_bits(skb, hdrlen, &mesh_flags, 1) < 0) + return -1; mesh_flags &= MESH_FLAGS_AE; @@ -595,11 +594,12 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr, if (iftype == NL80211_IFTYPE_MESH_POINT) { if (mesh_flags == MESH_FLAGS_AE_A4) return -1; - if (mesh_flags == MESH_FLAGS_AE_A5_A6) { - skb_copy_bits(skb, hdrlen + - offsetof(struct ieee80211s_hdr, eaddr1), - tmp.h_dest, 2 * ETH_ALEN); - } + if (mesh_flags == MESH_FLAGS_AE_A5_A6 && + skb_copy_bits(skb, hdrlen + + offsetof(struct ieee80211s_hdr, eaddr1), + tmp.h_dest, 2 * ETH_ALEN) < 0) + return -1; + hdrlen += __ieee80211_get_mesh_hdrlen(mesh_flags); } break; @@ -613,10 +613,11 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr, if (iftype == NL80211_IFTYPE_MESH_POINT) { if (mesh_flags == MESH_FLAGS_AE_A5_A6) return -1; - if (mesh_flags == MESH_FLAGS_AE_A4) - skb_copy_bits(skb, hdrlen + - offsetof(struct ieee80211s_hdr, eaddr1), - tmp.h_source, ETH_ALEN); + if (mesh_flags == MESH_FLAGS_AE_A4 && + skb_copy_bits(skb, hdrlen + + offsetof(struct ieee80211s_hdr, eaddr1), + tmp.h_source, ETH_ALEN) < 0) + return -1; hdrlen += __ieee80211_get_mesh_hdrlen(mesh_flags); } break; @@ -628,16 +629,15 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr, break; } - skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)); - tmp.h_proto = payload.proto; - - if (likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) && + if (likely(skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)) == 0) && + likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) && tmp.h_proto != htons(ETH_P_AARP) && tmp.h_proto != htons(ETH_P_IPX)) || ether_addr_equal(payload.hdr, bridge_tunnel_header))) { /* remove RFC1042 or Bridge-Tunnel encapsulation and * replace EtherType */ hdrlen += ETH_ALEN + 2; + tmp.h_proto = payload.proto; skb_postpull_rcsum(skb, &payload, ETH_ALEN + 2); } else { tmp.h_proto = htons(skb->len - hdrlen);
STP topology change notification packets only have a payload of 7 bytes, so they get dropped due to the skb->len < hdrlen + 8 check. Fix this by removing skb->len based checks and instead check the return code on the skb_copy_bits calls. Fixes: 2d1c304cb2d5 ("cfg80211: add function for 802.3 conversion with separate output buffer") Reported-by: Chad Monroe <chad.monroe@smartrg.com> Signed-off-by: Felix Fietkau <nbd@nbd.name> --- net/wireless/util.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)