Commit 9c4604a2 authored by Sven Eckelmann's avatar Sven Eckelmann Committed by David S. Miller

batman-adv: Fix use-after-free/double-free of tt_req_node

The tt_req_node is added and removed from a list inside a spinlock. But the
locking is sometimes removed even when the object is still referenced and
will be used later via this reference. For example batadv_send_tt_request
can create a new tt_req_node (including add to a list) and later
re-acquires the lock to remove it from the list and to free it. But at this
time another context could have already removed this tt_req_node from the
list and freed it.

CPU#0

    batadv_batman_skb_recv from net_device 0
    -> batadv_iv_ogm_receive
      -> batadv_iv_ogm_process
        -> batadv_iv_ogm_process_per_outif
          -> batadv_tvlv_ogm_receive
            -> batadv_tvlv_ogm_receive
              -> batadv_tvlv_containers_process
                -> batadv_tvlv_call_handler
                  -> batadv_tt_tvlv_ogm_handler_v1
                    -> batadv_tt_update_orig
                      -> batadv_send_tt_request
                        -> batadv_tt_req_node_new
                           spin_lock(...)
                           allocates new tt_req_node and adds it to list
                           spin_unlock(...)
                           return tt_req_node

CPU#1

    batadv_batman_skb_recv from net_device 1
    -> batadv_recv_unicast_tvlv
      -> batadv_tvlv_containers_process
        -> batadv_tvlv_call_handler
          -> batadv_tt_tvlv_unicast_handler_v1
            -> batadv_handle_tt_response
               spin_lock(...)
               tt_req_node gets removed from list and is freed
               spin_unlock(...)

CPU#0

                      <- returned to batadv_send_tt_request
                         spin_lock(...)
                         tt_req_node gets removed from list and is freed
                         MEMORY CORRUPTION/SEGFAULT/...
                         spin_unlock(...)

This can only be solved via reference counting to allow multiple contexts
to handle the list manipulation while making sure that only the last
context holding a reference will free the object.

Fixes: a73105b8 ("batman-adv: improved client announcement mechanism")
Signed-off-by: default avatarSven Eckelmann <sven@narfation.org>
Tested-by: default avatarMartin Weinelt <martin@darmstadt.freifunk.net>
Tested-by: default avatarAmadeus Alfa <amadeus@chemnitz.freifunk.net>
Signed-off-by: default avatarMarek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 0b3dd7df
...@@ -2271,6 +2271,29 @@ static u32 batadv_tt_local_crc(struct batadv_priv *bat_priv, ...@@ -2271,6 +2271,29 @@ static u32 batadv_tt_local_crc(struct batadv_priv *bat_priv,
return crc; return crc;
} }
/**
* batadv_tt_req_node_release - free tt_req node entry
* @ref: kref pointer of the tt req_node entry
*/
static void batadv_tt_req_node_release(struct kref *ref)
{
struct batadv_tt_req_node *tt_req_node;
tt_req_node = container_of(ref, struct batadv_tt_req_node, refcount);
kfree(tt_req_node);
}
/**
* batadv_tt_req_node_put - decrement the tt_req_node refcounter and
* possibly release it
* @tt_req_node: tt_req_node to be free'd
*/
static void batadv_tt_req_node_put(struct batadv_tt_req_node *tt_req_node)
{
kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
}
static void batadv_tt_req_list_free(struct batadv_priv *bat_priv) static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
{ {
struct batadv_tt_req_node *node; struct batadv_tt_req_node *node;
...@@ -2280,7 +2303,7 @@ static void batadv_tt_req_list_free(struct batadv_priv *bat_priv) ...@@ -2280,7 +2303,7 @@ static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
hlist_del_init(&node->list); hlist_del_init(&node->list);
kfree(node); batadv_tt_req_node_put(node);
} }
spin_unlock_bh(&bat_priv->tt.req_list_lock); spin_unlock_bh(&bat_priv->tt.req_list_lock);
...@@ -2317,7 +2340,7 @@ static void batadv_tt_req_purge(struct batadv_priv *bat_priv) ...@@ -2317,7 +2340,7 @@ static void batadv_tt_req_purge(struct batadv_priv *bat_priv)
if (batadv_has_timed_out(node->issued_at, if (batadv_has_timed_out(node->issued_at,
BATADV_TT_REQUEST_TIMEOUT)) { BATADV_TT_REQUEST_TIMEOUT)) {
hlist_del_init(&node->list); hlist_del_init(&node->list);
kfree(node); batadv_tt_req_node_put(node);
} }
} }
spin_unlock_bh(&bat_priv->tt.req_list_lock); spin_unlock_bh(&bat_priv->tt.req_list_lock);
...@@ -2349,9 +2372,11 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv, ...@@ -2349,9 +2372,11 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv,
if (!tt_req_node) if (!tt_req_node)
goto unlock; goto unlock;
kref_init(&tt_req_node->refcount);
ether_addr_copy(tt_req_node->addr, orig_node->orig); ether_addr_copy(tt_req_node->addr, orig_node->orig);
tt_req_node->issued_at = jiffies; tt_req_node->issued_at = jiffies;
kref_get(&tt_req_node->refcount);
hlist_add_head(&tt_req_node->list, &bat_priv->tt.req_list); hlist_add_head(&tt_req_node->list, &bat_priv->tt.req_list);
unlock: unlock:
spin_unlock_bh(&bat_priv->tt.req_list_lock); spin_unlock_bh(&bat_priv->tt.req_list_lock);
...@@ -2615,13 +2640,19 @@ static bool batadv_send_tt_request(struct batadv_priv *bat_priv, ...@@ -2615,13 +2640,19 @@ static bool batadv_send_tt_request(struct batadv_priv *bat_priv,
out: out:
if (primary_if) if (primary_if)
batadv_hardif_put(primary_if); batadv_hardif_put(primary_if);
if (ret && tt_req_node) { if (ret && tt_req_node) {
spin_lock_bh(&bat_priv->tt.req_list_lock); spin_lock_bh(&bat_priv->tt.req_list_lock);
/* hlist_del_init() verifies tt_req_node still is in the list */ if (!hlist_unhashed(&tt_req_node->list)) {
hlist_del_init(&tt_req_node->list); hlist_del_init(&tt_req_node->list);
batadv_tt_req_node_put(tt_req_node);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock); spin_unlock_bh(&bat_priv->tt.req_list_lock);
kfree(tt_req_node);
} }
if (tt_req_node)
batadv_tt_req_node_put(tt_req_node);
kfree(tvlv_tt_data); kfree(tvlv_tt_data);
return ret; return ret;
} }
...@@ -3057,7 +3088,7 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv, ...@@ -3057,7 +3088,7 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
if (!batadv_compare_eth(node->addr, resp_src)) if (!batadv_compare_eth(node->addr, resp_src))
continue; continue;
hlist_del_init(&node->list); hlist_del_init(&node->list);
kfree(node); batadv_tt_req_node_put(node);
} }
spin_unlock_bh(&bat_priv->tt.req_list_lock); spin_unlock_bh(&bat_priv->tt.req_list_lock);
......
...@@ -1137,11 +1137,13 @@ struct batadv_tt_change_node { ...@@ -1137,11 +1137,13 @@ struct batadv_tt_change_node {
* struct batadv_tt_req_node - data to keep track of the tt requests in flight * struct batadv_tt_req_node - data to keep track of the tt requests in flight
* @addr: mac address address of the originator this request was sent to * @addr: mac address address of the originator this request was sent to
* @issued_at: timestamp used for purging stale tt requests * @issued_at: timestamp used for purging stale tt requests
* @refcount: number of contexts the object is used by
* @list: list node for batadv_priv_tt::req_list * @list: list node for batadv_priv_tt::req_list
*/ */
struct batadv_tt_req_node { struct batadv_tt_req_node {
u8 addr[ETH_ALEN]; u8 addr[ETH_ALEN];
unsigned long issued_at; unsigned long issued_at;
struct kref refcount;
struct hlist_node list; struct hlist_node list;
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment