Feature #419
openBLA: redundant and superficial GW check
0%
Description
The source address check in batadv_recv_unicast_packet() here is both superficial and redundant:
989 /* packet for me */
990 if (batadv_is_my_mac(bat_priv, unicast_packet->dest)) {
991 /* If this is a unicast packet from another backgone gw,
992 * drop it.
993 */
994 orig_addr_gw = eth_hdr(skb)->h_source;
995 orig_node_gw = batadv_orig_hash_find(bat_priv, orig_addr_gw);
996 if (orig_node_gw) {
997 is_gw = batadv_bla_is_backbone_gw(skb, orig_node_gw,
998 hdr_size);
999 batadv_orig_node_put(orig_node_gw);
1000 if (is_gw) {
1001 batadv_dbg(BATADV_DBG_BLA, bat_priv,
1002 "%s(): Dropped unicast pkt received from another backbone gw %pM.\n",
1003 __func__, orig_addr_gw);
1004 goto free_skb;
1005 }
1006 }
1007
Redundant, because the sender is already supposed to perform this check, so no need to do it again on reception.
Superficial, because it only works if:- The BLA backbone gateway we share a LAN with is a direct neighbor of us.
- The BLA backbone gateway we share a LAN with transmits the packet via its primary interface to us.
In all other cases, like received via multiple hops or via a secondary interface from the other BLA gateway does not work.
Suggestion:- Either remove this check.
- Or turn the according batadv_dbg() into a pr_warn_ratelimited() to help in spotting potential bugs
(This check initially made it hard to reproduce the issue this patch is supposed to fix: https://patchwork.open-mesh.org/project/b.a.t.m.a.n./patch/20200914012136.5278-2-linus.luessing@c0d3.blue/. Initially it was easy to reproduce in a physical setup but then difficult to reproduce in a virtual one, because they had different configurations regarding primary vs. secondary interfaces.)
Updated by Sven Eckelmann 4 months ago
Redundant, because the sender is already supposed to perform this check, so no need to do it again on reception.
If we would remove it, would this add some kind of attack surface (for a bad actor)?