[maint] batman-adv: Fix use-after-free of tt_req_node
Commit Message
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.
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 frees the list.
Fixes: cea194d90b11 ("batman-adv: improved client announcement mechanism")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Cc: Matthias Schiffer <mschiffer@universe-factory.net>:
this can also be interesting for batman-adv-legacy installations on servers
with more than one core
net/batman-adv/translation-table.c | 27 +++++++++++++++++++++++----
net/batman-adv/types.h | 2 ++
2 files changed, 25 insertions(+), 4 deletions(-)
Comments
On Sunday 29 May 2016 22:33:39 Sven Eckelmann wrote:
> 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.
>
> 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 frees the list.
Marek please replace the "frees the list" with "frees the object". I can also
resent the patch if you want to.
I will not resent it today because maybe Antonio or someone else still has
some problems with the current version.
Kind regards,
Sven
@@ -2272,6 +2272,19 @@ static u32 batadv_tt_local_crc(struct batadv_priv *bat_priv,
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 *node;
+
+ node = container_of(ref, struct batadv_tt_req_node, refcount);
+
+ kfree(node);
+}
+
static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
{
struct batadv_tt_req_node *node;
@@ -2281,7 +2294,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_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2318,7 +2331,7 @@ static void batadv_tt_req_purge(struct batadv_priv *bat_priv)
if (batadv_has_timed_out(node->issued_at,
BATADV_TT_REQUEST_TIMEOUT)) {
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2350,9 +2363,11 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv,
if (!tt_req_node)
goto unlock;
+ kref_init(&tt_req_node->refcount);
ether_addr_copy(tt_req_node->addr, orig_node->orig);
tt_req_node->issued_at = jiffies;
+ kref_get(&tt_req_node->refcount);
hlist_add_head(&tt_req_node->list, &bat_priv->tt.req_list);
unlock:
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2619,9 +2634,13 @@ out:
spin_lock_bh(&bat_priv->tt.req_list_lock);
/* hlist_del_init() verifies tt_req_node still is in the list */
hlist_del_init(&tt_req_node->list);
+ kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
spin_unlock_bh(&bat_priv->tt.req_list_lock);
- kfree(tt_req_node);
}
+
+ if (tt_req_node)
+ kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
+
kfree(tvlv_tt_data);
return ret;
}
@@ -3057,7 +3076,7 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
if (!batadv_compare_eth(node->addr, resp_src))
continue;
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -1129,11 +1129,13 @@ struct batadv_tt_change_node {
* 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
* @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
*/
struct batadv_tt_req_node {
u8 addr[ETH_ALEN];
unsigned long issued_at;
+ struct kref refcount;
struct hlist_node list;
};