diff options
author | Sven Eckelmann <sven@narfation.org> | 2016-06-26 11:16:10 +0200 |
---|---|---|
committer | Ben Hutchings <ben@decadent.org.uk> | 2016-08-22 22:37:17 +0100 |
commit | 16e7b8c95fa6f8e57d9ab73597e31e3396f0c00d (patch) | |
tree | 5b8a6b2640ff761598af72e63123d777bde72b31 | |
parent | f2d7195ca0a794b5822f991b5cba78032ccf0f7c (diff) | |
download | linux-stable-16e7b8c95fa6f8e57d9ab73597e31e3396f0c00d.tar.gz linux-stable-16e7b8c95fa6f8e57d9ab73597e31e3396f0c00d.tar.bz2 linux-stable-16e7b8c95fa6f8e57d9ab73597e31e3396f0c00d.zip |
batman-adv: Fix use-after-free/double-free of tt_req_node
commit 9c4604a298e0a9807eaf2cd912d1ebf24d98fbeb upstream.
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: a73105b8d4c7 ("batman-adv: improved client announcement mechanism")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Tested-by: Martin Weinelt <martin@darmstadt.freifunk.net>
Tested-by: Amadeus Alfa <amadeus@chemnitz.freifunk.net>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>
[bwh: Backported to 3.2:
- Adjust context
- Use struct tt_req_node instead of struct batadv_tt_req_node
- Use list_empty() instead of hlist_unhashed()
- Drop kernel-doc change]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
-rw-r--r-- | net/batman-adv/translation-table.c | 42 | ||||
-rw-r--r-- | net/batman-adv/types.h | 1 |
2 files changed, 38 insertions, 5 deletions
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 088af45f45c8..d2d85a6cabcc 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -952,6 +952,29 @@ uint16_t tt_local_crc(struct bat_priv *bat_priv) return total; } +/** + * 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 tt_req_node *tt_req_node; + + tt_req_node = container_of(ref, struct 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 tt_req_node *tt_req_node) +{ + kref_put(&tt_req_node->refcount, batadv_tt_req_node_release); +} + static void tt_req_list_free(struct bat_priv *bat_priv) { struct tt_req_node *node, *safe; @@ -960,7 +983,7 @@ static void tt_req_list_free(struct bat_priv *bat_priv) list_for_each_entry_safe(node, safe, &bat_priv->tt_req_list, list) { list_del(&node->list); - kfree(node); + batadv_tt_req_node_put(node); } spin_unlock_bh(&bat_priv->tt_req_list_lock); @@ -995,7 +1018,7 @@ static void tt_req_purge(struct bat_priv *bat_priv) if (is_out_of_time(node->issued_at, TT_REQUEST_TIMEOUT * 1000)) { list_del(&node->list); - kfree(node); + batadv_tt_req_node_put(node); } } spin_unlock_bh(&bat_priv->tt_req_list_lock); @@ -1020,9 +1043,11 @@ static struct tt_req_node *new_tt_req_node(struct bat_priv *bat_priv, if (!tt_req_node) goto unlock; + kref_init(&tt_req_node->refcount); memcpy(tt_req_node->addr, orig_node->orig, ETH_ALEN); tt_req_node->issued_at = jiffies; + kref_get(&tt_req_node->refcount); list_add(&tt_req_node->list, &bat_priv->tt_req_list); unlock: spin_unlock_bh(&bat_priv->tt_req_list_lock); @@ -1174,12 +1199,19 @@ out: hardif_free_ref(primary_if); if (ret) kfree_skb(skb); + if (ret && tt_req_node) { spin_lock_bh(&bat_priv->tt_req_list_lock); - list_del(&tt_req_node->list); + if (!list_empty(&tt_req_node->list)) { + list_del(&tt_req_node->list); + batadv_tt_req_node_put(tt_req_node); + } 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); + return ret; } @@ -1552,7 +1584,7 @@ void handle_tt_response(struct bat_priv *bat_priv, if (!compare_eth(node->addr, tt_response->src)) continue; list_del(&node->list); - kfree(node); + batadv_tt_req_node_put(node); } spin_unlock_bh(&bat_priv->tt_req_list_lock); diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index ab8d0fe6df5a..1be63ab0ad2b 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -250,6 +250,7 @@ struct tt_change_node { struct tt_req_node { uint8_t addr[ETH_ALEN]; unsigned long issued_at; + struct kref refcount; struct list_head list; }; |