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