Commit 3db0decf authored by Sven Eckelmann's avatar Sven Eckelmann Committed by Simon Wunderlich
Browse files

batman-adv: Fix non-atomic bla_claim::backbone_gw access 



The pointer batadv_bla_claim::backbone_gw can be changed at any time.
Therefore, access to it must be protected to ensure that two function
accessing the same backbone_gw are actually accessing the same. This is
especially important when the crc_lock is used or when the backbone_gw of a
claim is exchanged.

Not doing so leads to invalid memory access and/or reference leaks.

Fixes: 23721387 ("batman-adv: add basic bridge loop avoidance code")
Fixes: 5a1dd8a4 ("batman-adv: lock crc access in bridge loop avoidance")
Signed-off-by: default avatarSven Eckelmann <sven@narfation.org>
Signed-off-by: default avatarMarek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: default avatarSimon Wunderlich <sw@simonwunderlich.de>
parent 33fbb1f3

net/batman-adv/bridge_loop_avoidance.c

+88 −23
Original line number Original line Diff line number Diff line
@@ -177,10 +177,21 @@ static void batadv_backbone_gw_put(struct batadv_bla_backbone_gw *backbone_gw) 
static void batadv_claim_release(struct kref *ref)

static void batadv_claim_release(struct kref *ref)

{

{

	struct batadv_bla_claim *claim;

	struct batadv_bla_claim *claim;

	struct batadv_bla_backbone_gw *old_backbone_gw;





	claim = container_of(ref, struct batadv_bla_claim, refcount);

	claim = container_of(ref, struct batadv_bla_claim, refcount);





	batadv_backbone_gw_put(claim->backbone_gw);

	spin_lock_bh(&claim->backbone_lock);

	old_backbone_gw = claim->backbone_gw;

	claim->backbone_gw = NULL;

	spin_unlock_bh(&claim->backbone_lock);



	spin_lock_bh(&old_backbone_gw->crc_lock);

	old_backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);

	spin_unlock_bh(&old_backbone_gw->crc_lock);



	batadv_backbone_gw_put(old_backbone_gw);



	kfree_rcu(claim, rcu);

	kfree_rcu(claim, rcu);

}

}
@@ -677,8 +688,10 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv, 
				 const u8 *mac, const unsigned short vid,

				 const u8 *mac, const unsigned short vid,

				 struct batadv_bla_backbone_gw *backbone_gw)

				 struct batadv_bla_backbone_gw *backbone_gw)

{

{

	struct batadv_bla_backbone_gw *old_backbone_gw;

	struct batadv_bla_claim *claim;

	struct batadv_bla_claim *claim;

	struct batadv_bla_claim search_claim;

	struct batadv_bla_claim search_claim;

	bool remove_crc = false;

	int hash_added;

	int hash_added;





	ether_addr_copy(search_claim.addr, mac);

	ether_addr_copy(search_claim.addr, mac);
@@ -692,8 +705,10 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv, 
			return;

			return;





		ether_addr_copy(claim->addr, mac);

		ether_addr_copy(claim->addr, mac);

		spin_lock_init(&claim->backbone_lock);

		claim->vid = vid;

		claim->vid = vid;

		claim->lasttime = jiffies;

		claim->lasttime = jiffies;

		kref_get(&backbone_gw->refcount);

		claim->backbone_gw = backbone_gw;

		claim->backbone_gw = backbone_gw;





		kref_init(&claim->refcount);

		kref_init(&claim->refcount);
@@ -721,15 +736,26 @@ static void batadv_bla_add_claim(struct batadv_priv *bat_priv, 
			   "bla_add_claim(): changing ownership for %pM, vid %d\n",

			   "bla_add_claim(): changing ownership for %pM, vid %d\n",

			   mac, BATADV_PRINT_VID(vid));

			   mac, BATADV_PRINT_VID(vid));





		spin_lock_bh(&claim->backbone_gw->crc_lock);

		remove_crc = true;

		claim->backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);

		spin_unlock_bh(&claim->backbone_gw->crc_lock);

		batadv_backbone_gw_put(claim->backbone_gw);

	}

	}

	/* set (new) backbone gw */



	/* replace backbone_gw atomically and adjust reference counters */

	spin_lock_bh(&claim->backbone_lock);

	old_backbone_gw = claim->backbone_gw;

	kref_get(&backbone_gw->refcount);

	kref_get(&backbone_gw->refcount);

	claim->backbone_gw = backbone_gw;

	claim->backbone_gw = backbone_gw;

	spin_unlock_bh(&claim->backbone_lock);



	if (remove_crc) {

		/* remove claim address from old backbone_gw */

		spin_lock_bh(&old_backbone_gw->crc_lock);

		old_backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);

		spin_unlock_bh(&old_backbone_gw->crc_lock);

	}



	batadv_backbone_gw_put(old_backbone_gw);





	/* add claim address to new backbone_gw */

	spin_lock_bh(&backbone_gw->crc_lock);

	spin_lock_bh(&backbone_gw->crc_lock);

	backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);

	backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);

	spin_unlock_bh(&backbone_gw->crc_lock);

	spin_unlock_bh(&backbone_gw->crc_lock);
@@ -739,6 +765,26 @@ claim_free_ref: 
	batadv_claim_put(claim);

	batadv_claim_put(claim);

}

}





/**

 * batadv_bla_claim_get_backbone_gw - Get valid reference for backbone_gw of

 *  claim

 * @claim: claim whose backbone_gw should be returned

 *

 * Return: valid reference to claim::backbone_gw

 */

static struct batadv_bla_backbone_gw *

batadv_bla_claim_get_backbone_gw(struct batadv_bla_claim *claim)

{

	struct batadv_bla_backbone_gw *backbone_gw;



	spin_lock_bh(&claim->backbone_lock);

	backbone_gw = claim->backbone_gw;

	kref_get(&backbone_gw->refcount);

	spin_unlock_bh(&claim->backbone_lock);



	return backbone_gw;

}



/**

/**

 * batadv_bla_del_claim - delete a claim from the claim hash

 * batadv_bla_del_claim - delete a claim from the claim hash

 * @bat_priv: the bat priv with all the soft interface information

 * @bat_priv: the bat priv with all the soft interface information
@@ -763,10 +809,6 @@ static void batadv_bla_del_claim(struct batadv_priv *bat_priv, 
			   batadv_choose_claim, claim);

			   batadv_choose_claim, claim);

	batadv_claim_put(claim); /* reference from the hash is gone */

	batadv_claim_put(claim); /* reference from the hash is gone */





	spin_lock_bh(&claim->backbone_gw->crc_lock);

	claim->backbone_gw->crc ^= crc16(0, claim->addr, ETH_ALEN);

	spin_unlock_bh(&claim->backbone_gw->crc_lock);



	/* don't need the reference from hash_find() anymore */

	/* don't need the reference from hash_find() anymore */

	batadv_claim_put(claim);

	batadv_claim_put(claim);

}

}
@@ -1219,6 +1261,7 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv, 
				    struct batadv_hard_iface *primary_if,

				    struct batadv_hard_iface *primary_if,

				    int now)

				    int now)

{

{

	struct batadv_bla_backbone_gw *backbone_gw;

	struct batadv_bla_claim *claim;

	struct batadv_bla_claim *claim;

	struct hlist_head *head;

	struct hlist_head *head;

	struct batadv_hashtable *hash;

	struct batadv_hashtable *hash;
@@ -1233,14 +1276,17 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv, 




		rcu_read_lock();

		rcu_read_lock();

		hlist_for_each_entry_rcu(claim, head, hash_entry) {

		hlist_for_each_entry_rcu(claim, head, hash_entry) {

			backbone_gw = batadv_bla_claim_get_backbone_gw(claim);

			if (now)

			if (now)

				goto purge_now;

				goto purge_now;

			if (!batadv_compare_eth(claim->backbone_gw->orig,



			if (!batadv_compare_eth(backbone_gw->orig,

						primary_if->net_dev->dev_addr))

						primary_if->net_dev->dev_addr))

				continue;

				goto skip;



			if (!batadv_has_timed_out(claim->lasttime,

			if (!batadv_has_timed_out(claim->lasttime,

						  BATADV_BLA_CLAIM_TIMEOUT))

						  BATADV_BLA_CLAIM_TIMEOUT))

				continue;

				goto skip;





			batadv_dbg(BATADV_DBG_BLA, bat_priv,

			batadv_dbg(BATADV_DBG_BLA, bat_priv,

				   "bla_purge_claims(): %pM, vid %d, time out\n",

				   "bla_purge_claims(): %pM, vid %d, time out\n",
@@ -1248,8 +1294,10 @@ static void batadv_bla_purge_claims(struct batadv_priv *bat_priv, 




purge_now:

purge_now:

			batadv_handle_unclaim(bat_priv, primary_if,

			batadv_handle_unclaim(bat_priv, primary_if,

					      claim->backbone_gw->orig,

					      backbone_gw->orig,

					      claim->addr, claim->vid);

					      claim->addr, claim->vid);

skip:

			batadv_backbone_gw_put(backbone_gw);

		}

		}

		rcu_read_unlock();

		rcu_read_unlock();

	}

	}
@@ -1760,9 +1808,11 @@ batadv_bla_loopdetect_check(struct batadv_priv *bat_priv, struct sk_buff *skb, 
bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,

bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb,

		   unsigned short vid, bool is_bcast)

		   unsigned short vid, bool is_bcast)

{

{

	struct batadv_bla_backbone_gw *backbone_gw;

	struct ethhdr *ethhdr;

	struct ethhdr *ethhdr;

	struct batadv_bla_claim search_claim, *claim = NULL;

	struct batadv_bla_claim search_claim, *claim = NULL;

	struct batadv_hard_iface *primary_if;

	struct batadv_hard_iface *primary_if;

	bool own_claim;

	bool ret;

	bool ret;





	ethhdr = eth_hdr(skb);

	ethhdr = eth_hdr(skb);
@@ -1797,8 +1847,12 @@ bool batadv_bla_rx(struct batadv_priv *bat_priv, struct sk_buff *skb, 
	}

	}





	/* if it is our own claim ... */

	/* if it is our own claim ... */

	if (batadv_compare_eth(claim->backbone_gw->orig,

	backbone_gw = batadv_bla_claim_get_backbone_gw(claim);

			       primary_if->net_dev->dev_addr)) {

	own_claim = batadv_compare_eth(backbone_gw->orig,

				       primary_if->net_dev->dev_addr);

	batadv_backbone_gw_put(backbone_gw);



	if (own_claim) {

		/* ... allow it in any case */

		/* ... allow it in any case */

		claim->lasttime = jiffies;

		claim->lasttime = jiffies;

		goto allow;

		goto allow;
@@ -1862,7 +1916,9 @@ bool batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb, 
{

{

	struct ethhdr *ethhdr;

	struct ethhdr *ethhdr;

	struct batadv_bla_claim search_claim, *claim = NULL;

	struct batadv_bla_claim search_claim, *claim = NULL;

	struct batadv_bla_backbone_gw *backbone_gw;

	struct batadv_hard_iface *primary_if;

	struct batadv_hard_iface *primary_if;

	bool client_roamed;

	bool ret = false;

	bool ret = false;





	primary_if = batadv_primary_if_get_selected(bat_priv);

	primary_if = batadv_primary_if_get_selected(bat_priv);
@@ -1892,8 +1948,12 @@ bool batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb, 
		goto allow;

		goto allow;





	/* check if we are responsible. */

	/* check if we are responsible. */

	if (batadv_compare_eth(claim->backbone_gw->orig,

	backbone_gw = batadv_bla_claim_get_backbone_gw(claim);

			       primary_if->net_dev->dev_addr)) {

	client_roamed = batadv_compare_eth(backbone_gw->orig,

					   primary_if->net_dev->dev_addr);

	batadv_backbone_gw_put(backbone_gw);



	if (client_roamed) {

		/* if yes, the client has roamed and we have

		/* if yes, the client has roamed and we have

		 * to unclaim it.

		 * to unclaim it.

		 */

		 */
@@ -1941,6 +2001,7 @@ int batadv_bla_claim_table_seq_print_text(struct seq_file *seq, void *offset) 
	struct net_device *net_dev = (struct net_device *)seq->private;

	struct net_device *net_dev = (struct net_device *)seq->private;

	struct batadv_priv *bat_priv = netdev_priv(net_dev);

	struct batadv_priv *bat_priv = netdev_priv(net_dev);

	struct batadv_hashtable *hash = bat_priv->bla.claim_hash;

	struct batadv_hashtable *hash = bat_priv->bla.claim_hash;

	struct batadv_bla_backbone_gw *backbone_gw;

	struct batadv_bla_claim *claim;

	struct batadv_bla_claim *claim;

	struct batadv_hard_iface *primary_if;

	struct batadv_hard_iface *primary_if;

	struct hlist_head *head;

	struct hlist_head *head;
@@ -1965,17 +2026,21 @@ int batadv_bla_claim_table_seq_print_text(struct seq_file *seq, void *offset) 




		rcu_read_lock();

		rcu_read_lock();

		hlist_for_each_entry_rcu(claim, head, hash_entry) {

		hlist_for_each_entry_rcu(claim, head, hash_entry) {

			is_own = batadv_compare_eth(claim->backbone_gw->orig,

			backbone_gw = batadv_bla_claim_get_backbone_gw(claim);



			is_own = batadv_compare_eth(backbone_gw->orig,

						    primary_addr);

						    primary_addr);





			spin_lock_bh(&claim->backbone_gw->crc_lock);

			spin_lock_bh(&backbone_gw->crc_lock);

			backbone_crc = claim->backbone_gw->crc;

			backbone_crc = backbone_gw->crc;

			spin_unlock_bh(&claim->backbone_gw->crc_lock);

			spin_unlock_bh(&backbone_gw->crc_lock);

			seq_printf(seq, " * %pM on %5d by %pM [%c] (%#.4x)\n",

			seq_printf(seq, " * %pM on %5d by %pM [%c] (%#.4x)\n",

				   claim->addr, BATADV_PRINT_VID(claim->vid),

				   claim->addr, BATADV_PRINT_VID(claim->vid),

				   claim->backbone_gw->orig,

				   backbone_gw->orig,

				   (is_own ? 'x' : ' '),

				   (is_own ? 'x' : ' '),

				   backbone_crc);

				   backbone_crc);



			batadv_backbone_gw_put(backbone_gw);

		}

		}

		rcu_read_unlock();

		rcu_read_unlock();

	}

	}

net/batman-adv/types.h

+2 −0
Original line number Original line Diff line number Diff line
@@ -1042,6 +1042,7 @@ struct batadv_bla_backbone_gw { 
 * @addr: mac address of claimed non-mesh client

 * @addr: mac address of claimed non-mesh client

 * @vid: vlan id this client was detected on

 * @vid: vlan id this client was detected on

 * @backbone_gw: pointer to backbone gw claiming this client

 * @backbone_gw: pointer to backbone gw claiming this client

 * @backbone_lock: lock protecting backbone_gw pointer

 * @lasttime: last time we heard of claim (locals only)

 * @lasttime: last time we heard of claim (locals only)

 * @hash_entry: hlist node for batadv_priv_bla::claim_hash

 * @hash_entry: hlist node for batadv_priv_bla::claim_hash

 * @refcount: number of contexts the object is used

 * @refcount: number of contexts the object is used
@@ -1051,6 +1052,7 @@ struct batadv_bla_claim { 
	u8 addr[ETH_ALEN];

	u8 addr[ETH_ALEN];

	unsigned short vid;

	unsigned short vid;

	struct batadv_bla_backbone_gw *backbone_gw;

	struct batadv_bla_backbone_gw *backbone_gw;

	spinlock_t backbone_lock; /* protects backbone_gw */

	unsigned long lasttime;

	unsigned long lasttime;

	struct hlist_node hash_entry;

	struct hlist_node hash_entry;

	struct rcu_head rcu;

	struct rcu_head rcu;

CRA Git | Maintained and supported by SUSTech CRA and CCSE