[B.A.T.M.A.N.] batman-adv: Multiple NULL derefence bugs (+ 1 bonus)

[B.A.T.M.A.N.] batman-adv: Multiple NULL derefence bugs (+ 1 bonus)

[Sven Eckelmann]

All taken from batman-adv master (b08dc3d9452d28944e7db09c2005c633b63f55e9)


1. multicast.c (dereference of NULL orig_node on at least the first round):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

orig_node = NULL before the first entry in hlist_for_each_entry_rcu. NULL
dereferenced in atomic_inc_not_zero(...) to get the refcount

Was introduced with 45dab5f5509023946793fc9d1d0566c12ca8bb52
("batman-adv: fix more code style")

Code section:
============

static struct batadv_orig_node *
batadv_mcast_forw_unsnoop_node_get(struct batadv_priv *bat_priv)
{
	struct batadv_orig_node *tmp_orig_node, *orig_node = NULL;

	rcu_read_lock();
	hlist_for_each_entry_rcu(tmp_orig_node,
				 &bat_priv->mcast.want_all_unsnoopables_list,
				 mcast_want_all_unsnoopables_node) {
		if (!atomic_inc_not_zero(&orig_node->refcount))
			continue;

		orig_node = tmp_orig_node;
		break;
	}
	rcu_read_unlock();

	return orig_node;
}

Solution:
========

Change atomic_inc_not_zero from orig_node to tmp_orig_node

2. multicast.c (dereference of NULL orig_node on at least the first round):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

orig_node = NULL before the first entry in hlist_for_each_entry_rcu. NULL
dereferenced in atomic_inc_not_zero(...) to get the refcount

Was introduced with 45dab5f5509023946793fc9d1d0566c12ca8bb52
("batman-adv: fix more code style")

Code section:
============

static struct batadv_orig_node *
batadv_mcast_forw_ipv6_node_get(struct batadv_priv *bat_priv)
{
	struct batadv_orig_node *tmp_orig_node, *orig_node = NULL;

	rcu_read_lock();
	hlist_for_each_entry_rcu(tmp_orig_node,
				 &bat_priv->mcast.want_all_ipv6_list,
				 mcast_want_all_ipv6_node) {
		if (!atomic_inc_not_zero(&orig_node->refcount))
			continue;

		orig_node = tmp_orig_node;
		break;
	}
	rcu_read_unlock();

	return orig_node;
}

Solution:
========

Change atomic_inc_not_zero from orig_node to tmp_orig_node

3. multicast.c (dereference of NULL orig_node on at least the first round):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

orig_node = NULL before the first entry in hlist_for_each_entry_rcu. NULL
dereferenced in atomic_inc_not_zero(...) to get the refcount

Was introduced with 45dab5f5509023946793fc9d1d0566c12ca8bb52
("batman-adv: fix more code style")

Code section:
============

static struct batadv_orig_node *
batadv_mcast_forw_ipv4_node_get(struct batadv_priv *bat_priv)
{
	struct batadv_orig_node *tmp_orig_node, *orig_node = NULL;

	rcu_read_lock();
	hlist_for_each_entry_rcu(tmp_orig_node,
				 &bat_priv->mcast.want_all_ipv4_list,
				 mcast_want_all_ipv4_node) {
		if (!atomic_inc_not_zero(&orig_node->refcount))
			continue;

		orig_node = tmp_orig_node;
		break;
	}
	rcu_read_unlock();

	return orig_node;
}

Solution:
========

Change atomic_inc_not_zero from orig_node to tmp_orig_node

4. originator.c (Indirect hard_iface NULL dereference):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

hard_iface is NULL and goto out is made. batadv_hardif_free_ref doesn't
check for NULL before dereferencing it to get to refcount.

Introduced in f13f960797fd1969b3c0470cc97435ddfb6aecb4
("batman-adv: add debugfs support to view multiif tables").

Code section:
============

int batadv_orig_hardif_seq_print_text(struct seq_file *seq, void *offset)
{
	struct net_device *net_dev = (struct net_device *)seq->private;
	struct batadv_hard_iface *hard_iface;
	struct batadv_priv *bat_priv;

	hard_iface = batadv_hardif_get_by_netdev(net_dev);

	if (!hard_iface || !hard_iface->soft_iface) {
		seq_puts(seq, "Interface not known to B.A.T.M.A.N.\n");
		goto out;
	}

[...]

out:
	batadv_hardif_free_ref(hard_iface);
	return 0;
}

Solution:
========

	if (hard_iface)
		batadv_hardif_free_ref(hard_iface);

Related code section:
====================

static inline void
batadv_hardif_free_ref_now(struct batadv_hard_iface *hard_iface)
{
	if (atomic_dec_and_test(&hard_iface->refcount))
		batadv_hardif_free_rcu(&hard_iface->rcu);
}


5. BONUS: fragmentation.c (Impossible code section marked likely):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

frag_entry_curr is iterator used for list chain->head. This loop is
only stopped when the list ends and frag_entry_curr is NULL. The next
section following the loop says that it is likely that frag_entry_curr
is NOT NULL. This is a contradiction.

Introduced in 9b3eab61754d74a93c9840c296013fe3b4a1b606
("batman-adv: Receive fragmented packets and merge")

Code section:
============

static bool batadv_frag_insert_packet(struct batadv_orig_node *orig_node,
				      struct sk_buff *skb,
				      struct hlist_head *chain_out)
{
[...]

	/* Find the position for the new fragment. */
	hlist_for_each_entry(frag_entry_curr, &chain->head, list) {
		/* Drop packet if fragment already exists. */
		if (frag_entry_curr->no == frag_entry_new->no)
			goto err_unlock;

		/* Order fragments from highest to lowest. */
		if (frag_entry_curr->no < frag_entry_new->no) {
			hlist_add_before(&frag_entry_new->list,
					 &frag_entry_curr->list);
			chain->size += skb->len - hdr_size;
			chain->timestamp = jiffies;
			ret = true;
			goto out;
		}
	}

	/* Reached the end of the list, so insert after 'frag_entry_curr'. */
	if (likely(frag_entry_curr)) {
		hlist_add_after(&frag_entry_curr->list, &frag_entry_new->list);
		chain->size += skb->len - hdr_size;
		chain->timestamp = jiffies;
		ret = true;
	}

[...]
}

Solution:
========

Rewrite this part to track the last seen batadv_frag_list_entry and add it
after that. The empty list should already handled earlier in the scope
starting with
	if (batadv_frag_init_chain(chain, seqno)) {

Related code section:
====================

#define hlist_for_each_entry(pos, head, member)                         \
        for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
             pos;                                                       \
             pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), 
member))