Project

General

Profile

Actions

Feature #419

open

BLA: redundant and superficial GW check

Added by Linus Lüssing about 6 years ago. Updated 4 months ago.

Status:
New
Priority:
Normal
Target version:
-
Start date:
09/14/2020
Due date:
% Done:

0%

Estimated time:

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 

https://git.open-mesh.org/batman-adv.git/blob/f2a2e0310dc1c570bdd1439553e897649b000292:/net/batman-adv/routing.c#l1000

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.)

Actions #1

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)?

Actions

Also available in: Atom PDF