From patchwork Mon May 30 06:11:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sven Eckelmann X-Patchwork-Id: 16313 X-Patchwork-Delegate: mareklindner@neomailbox.ch Return-Path: X-Original-To: patchwork@open-mesh.org Delivered-To: patchwork@open-mesh.org Received: from open-mesh.org (localhost [IPv6:::1]) by open-mesh.org (Postfix) with ESMTP id 4140B817C4; Mon, 30 May 2016 08:11:42 +0200 (CEST) Authentication-Results: open-mesh.org; dmarc=none header.from=narfation.org Authentication-Results: open-mesh.org; dkim=fail reason="verification failed; unprotected key" header.d=narfation.org header.i=@narfation.org header.b=iUsY0IC5; dkim-adsp=fail (unprotected policy); dkim-atps=neutral Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=79.140.41.39; helo=v3-1039.vlinux.de; envelope-from=sven@narfation.org; receiver=b.a.t.m.a.n@lists.open-mesh.org Authentication-Results: open-mesh.org; dmarc=pass header.from=narfation.org Received: from v3-1039.vlinux.de (narfation.org [79.140.41.39]) by open-mesh.org (Postfix) with ESMTPS id 8AE7D8149F for ; Mon, 30 May 2016 08:11:38 +0200 (CEST) Received: from sven-desktop.home.narfation.org (p200300C593C0BBF90000000000002E16.dip0.t-ipconnect.de [IPv6:2003:c5:93c0:bbf9::2e16]) by v3-1039.vlinux.de (Postfix) with ESMTPSA id F37031100E7; Mon, 30 May 2016 08:11:37 +0200 (CEST) Authentication-Results: v3-1039.vlinux.de; dmarc=none header.from=narfation.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=narfation.org; s=20121; t=1464588698; bh=uOEO0SpKDRWET7z6L6nvUt6D93shKfiZjAB6RfneZh0=; h=From:To:Cc:Subject:Date:From; b=iUsY0IC5zBDoiy+am/VJp2i2/ZxlMvNvak1dhi2DkvvLPXF1l76OG5ctZdk+PCZcz YnoEt8k5YWV5jLV1A9EW64uBIi8wNJdazQBXv+JQ8Kf766av1Q988KvCuUWls1hbEi WpLNU90St6DC35+eJIoakIxUPyFw7QZT6eP2m30Y= From: Sven Eckelmann To: b.a.t.m.a.n@lists.open-mesh.org Date: Mon, 30 May 2016 08:11:34 +0200 Message-Id: <1464588694-19855-1-git-send-email-sven@narfation.org> X-Mailer: git-send-email 2.8.1 Cc: a@unstable.cc Subject: [B.A.T.M.A.N.] [PATCH v2] batman-adv: Fix use-after-free/double-free of tt_req_node X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: The list for a Better Approach To Mobile Ad-hoc Networking List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking Errors-To: b.a.t.m.a.n-bounces@lists.open-mesh.org Sender: "B.A.T.M.A.N" 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: cea194d90b11 ("batman-adv: improved client announcement mechanism") Signed-off-by: Sven Eckelmann Tested-by: Martin Weinelt --- v2: - fixed list->object in commit message - add example what could have gone wrong in commit message --- net/batman-adv/translation-table.c | 27 +++++++++++++++++++++++---- net/batman-adv/types.h | 2 ++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 48adb91..46f90c5 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -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); diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 1e47fbe..d75beef 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -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; };