lkml.org 
[lkml]   [2016]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 253/305] batman-adv: Fix use-after-free/double-free of tt_req_node
    3.16.37-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Sven Eckelmann <sven@narfation.org>

    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.16:
    - Adjust context
    - Use list_empty() instead of hlist_unhashed()]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    net/batman-adv/translation-table.c | 43 ++++++++++++++++++++++++++++++++------
    net/batman-adv/types.h | 2 ++
    2 files changed, 39 insertions(+), 6 deletions(-)

    --- a/net/batman-adv/translation-table.c
    +++ b/net/batman-adv/translation-table.c
    @@ -2165,6 +2165,29 @@ static uint32_t batadv_tt_local_crc(stru
    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)
    {
    struct batadv_tt_req_node *node, *safe;
    @@ -2173,7 +2196,7 @@ static void batadv_tt_req_list_free(stru

    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);
    @@ -2209,7 +2232,7 @@ static void batadv_tt_req_purge(struct b
    if (batadv_has_timed_out(node->issued_at,
    BATADV_TT_REQUEST_TIMEOUT)) {
    list_del(&node->list);
    - kfree(node);
    + batadv_tt_req_node_put(node);
    }
    }
    spin_unlock_bh(&bat_priv->tt.req_list_lock);
    @@ -2236,9 +2259,11 @@ batadv_new_tt_req_node(struct batadv_pri
    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);
    list_add(&tt_req_node->list, &bat_priv->tt.req_list);
    unlock:
    spin_unlock_bh(&bat_priv->tt.req_list_lock);
    @@ -2488,12 +2513,19 @@ static int batadv_send_tt_request(struct
    out:
    if (primary_if)
    batadv_hardif_free_ref(primary_if);
    +
    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);
    +
    kfree(tvlv_tt_data);
    return ret;
    }
    @@ -2929,7 +2961,7 @@ static void batadv_handle_tt_response(st
    if (!batadv_compare_eth(node->addr, resp_src))
    continue;
    list_del(&node->list);
    - kfree(node);
    + batadv_tt_req_node_put(node);
    }

    spin_unlock_bh(&bat_priv->tt.req_list_lock);
    --- a/net/batman-adv/types.h
    +++ b/net/batman-adv/types.h
    @@ -988,11 +988,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 {
    uint8_t addr[ETH_ALEN];
    unsigned long issued_at;
    + struct kref refcount;
    struct list_head list;
    };

    \
     
     \ /
      Last update: 2016-09-17 09:56    [W:4.120 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site