lkml.org 
[lkml]   [2017]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 3.18 21/38] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table
    Date
    3.18-stable review patch.  If anyone has any objections, please let me know.

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

    From: Liping Zhang <zlpnobody@gmail.com>


    [ Upstream commit 83d90219a5df8d950855ce73229a97b63605c317 ]

    The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
    nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
    So it's possible that one CPU is walking the nf_ct_helper_hash for
    cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
    at the same time. This is dangrous, and may cause use after free error.

    Note, delete operation will flush all cthelpers added via nfnetlink, so
    using rcu to do protect is not easy.

    Now introduce a dummy list to record all the cthelpers added via
    nfnetlink, then we can walk the dummy list instead of walking the
    nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
    may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.

    Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
    Signed-off-by: Sasha Levin <alexander.levin@verizon.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    net/netfilter/nfnetlink_cthelper.c | 185 +++++++++++++++++--------------------
    1 file changed, 85 insertions(+), 100 deletions(-)

    --- a/net/netfilter/nfnetlink_cthelper.c
    +++ b/net/netfilter/nfnetlink_cthelper.c
    @@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
    MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
    MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");

    +struct nfnl_cthelper {
    + struct list_head list;
    + struct nf_conntrack_helper helper;
    +};
    +
    +static LIST_HEAD(nfnl_cthelper_list);
    +
    static int
    nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
    struct nf_conn *ct, enum ip_conntrack_info ctinfo)
    @@ -205,14 +212,16 @@ nfnl_cthelper_create(const struct nlattr
    struct nf_conntrack_tuple *tuple)
    {
    struct nf_conntrack_helper *helper;
    + struct nfnl_cthelper *nfcth;
    int ret;

    if (!tb[NFCTH_TUPLE] || !tb[NFCTH_POLICY] || !tb[NFCTH_PRIV_DATA_LEN])
    return -EINVAL;

    - helper = kzalloc(sizeof(struct nf_conntrack_helper), GFP_KERNEL);
    - if (helper == NULL)
    + nfcth = kzalloc(sizeof(*nfcth), GFP_KERNEL);
    + if (nfcth == NULL)
    return -ENOMEM;
    + helper = &nfcth->helper;

    ret = nfnl_cthelper_parse_expect_policy(helper, tb[NFCTH_POLICY]);
    if (ret < 0)
    @@ -249,11 +258,12 @@ nfnl_cthelper_create(const struct nlattr
    if (ret < 0)
    goto err2;

    + list_add_tail(&nfcth->list, &nfnl_cthelper_list);
    return 0;
    err2:
    kfree(helper->expect_policy);
    err1:
    - kfree(helper);
    + kfree(nfcth);
    return ret;
    }

    @@ -379,7 +389,8 @@ nfnl_cthelper_new(struct sock *nfnl, str
    const char *helper_name;
    struct nf_conntrack_helper *cur, *helper = NULL;
    struct nf_conntrack_tuple tuple;
    - int ret = 0, i;
    + struct nfnl_cthelper *nlcth;
    + int ret = 0;

    if (!tb[NFCTH_NAME] || !tb[NFCTH_TUPLE])
    return -EINVAL;
    @@ -390,31 +401,22 @@ nfnl_cthelper_new(struct sock *nfnl, str
    if (ret < 0)
    return ret;

    - rcu_read_lock();
    - for (i = 0; i < nf_ct_helper_hsize && !helper; i++) {
    - hlist_for_each_entry_rcu(cur, &nf_ct_helper_hash[i], hnode) {
    + list_for_each_entry(nlcth, &nfnl_cthelper_list, list) {
    + cur = &nlcth->helper;

    - /* skip non-userspace conntrack helpers. */
    - if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
    - continue;
    + if (strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
    + continue;

    - if (strncmp(cur->name, helper_name,
    - NF_CT_HELPER_NAME_LEN) != 0)
    - continue;
    + if ((tuple.src.l3num != cur->tuple.src.l3num ||
    + tuple.dst.protonum != cur->tuple.dst.protonum))
    + continue;

    - if ((tuple.src.l3num != cur->tuple.src.l3num ||
    - tuple.dst.protonum != cur->tuple.dst.protonum))
    - continue;
    + if (nlh->nlmsg_flags & NLM_F_EXCL)
    + return -EEXIST;

    - if (nlh->nlmsg_flags & NLM_F_EXCL) {
    - ret = -EEXIST;
    - goto err;
    - }
    - helper = cur;
    - break;
    - }
    + helper = cur;
    + break;
    }
    - rcu_read_unlock();

    if (helper == NULL)
    ret = nfnl_cthelper_create(tb, &tuple);
    @@ -422,9 +424,6 @@ nfnl_cthelper_new(struct sock *nfnl, str
    ret = nfnl_cthelper_update(tb, helper);

    return ret;
    -err:
    - rcu_read_unlock();
    - return ret;
    }

    static int
    @@ -588,11 +587,12 @@ static int
    nfnl_cthelper_get(struct sock *nfnl, struct sk_buff *skb,
    const struct nlmsghdr *nlh, const struct nlattr * const tb[])
    {
    - int ret = -ENOENT, i;
    + int ret = -ENOENT;
    struct nf_conntrack_helper *cur;
    struct sk_buff *skb2;
    char *helper_name = NULL;
    struct nf_conntrack_tuple tuple;
    + struct nfnl_cthelper *nlcth;
    bool tuple_set = false;

    if (nlh->nlmsg_flags & NLM_F_DUMP) {
    @@ -613,45 +613,39 @@ nfnl_cthelper_get(struct sock *nfnl, str
    tuple_set = true;
    }

    - for (i = 0; i < nf_ct_helper_hsize; i++) {
    - hlist_for_each_entry_rcu(cur, &nf_ct_helper_hash[i], hnode) {
    -
    - /* skip non-userspace conntrack helpers. */
    - if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
    - continue;
    -
    - if (helper_name && strncmp(cur->name, helper_name,
    - NF_CT_HELPER_NAME_LEN) != 0) {
    - continue;
    - }
    - if (tuple_set &&
    - (tuple.src.l3num != cur->tuple.src.l3num ||
    - tuple.dst.protonum != cur->tuple.dst.protonum))
    - continue;
    -
    - skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
    - if (skb2 == NULL) {
    - ret = -ENOMEM;
    - break;
    - }
    + list_for_each_entry(nlcth, &nfnl_cthelper_list, list) {
    + cur = &nlcth->helper;
    + if (helper_name &&
    + strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
    + continue;
    +
    + if (tuple_set &&
    + (tuple.src.l3num != cur->tuple.src.l3num ||
    + tuple.dst.protonum != cur->tuple.dst.protonum))
    + continue;
    +
    + skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
    + if (skb2 == NULL) {
    + ret = -ENOMEM;
    + break;
    + }

    - ret = nfnl_cthelper_fill_info(skb2, NETLINK_CB(skb).portid,
    - nlh->nlmsg_seq,
    - NFNL_MSG_TYPE(nlh->nlmsg_type),
    - NFNL_MSG_CTHELPER_NEW, cur);
    - if (ret <= 0) {
    - kfree_skb(skb2);
    - break;
    - }
    + ret = nfnl_cthelper_fill_info(skb2, NETLINK_CB(skb).portid,
    + nlh->nlmsg_seq,
    + NFNL_MSG_TYPE(nlh->nlmsg_type),
    + NFNL_MSG_CTHELPER_NEW, cur);
    + if (ret <= 0) {
    + kfree_skb(skb2);
    + break;
    + }

    - ret = netlink_unicast(nfnl, skb2, NETLINK_CB(skb).portid,
    - MSG_DONTWAIT);
    - if (ret > 0)
    - ret = 0;
    + ret = netlink_unicast(nfnl, skb2, NETLINK_CB(skb).portid,
    + MSG_DONTWAIT);
    + if (ret > 0)
    + ret = 0;

    - /* this avoids a loop in nfnetlink. */
    - return ret == -EAGAIN ? -ENOBUFS : ret;
    - }
    + /* this avoids a loop in nfnetlink. */
    + return ret == -EAGAIN ? -ENOBUFS : ret;
    }
    return ret;
    }
    @@ -662,10 +656,10 @@ nfnl_cthelper_del(struct sock *nfnl, str
    {
    char *helper_name = NULL;
    struct nf_conntrack_helper *cur;
    - struct hlist_node *tmp;
    struct nf_conntrack_tuple tuple;
    bool tuple_set = false, found = false;
    - int i, j = 0, ret;
    + struct nfnl_cthelper *nlcth, *n;
    + int j = 0, ret;

    if (tb[NFCTH_NAME])
    helper_name = nla_data(tb[NFCTH_NAME]);
    @@ -678,30 +672,27 @@ nfnl_cthelper_del(struct sock *nfnl, str
    tuple_set = true;
    }

    - for (i = 0; i < nf_ct_helper_hsize; i++) {
    - hlist_for_each_entry_safe(cur, tmp, &nf_ct_helper_hash[i],
    - hnode) {
    - /* skip non-userspace conntrack helpers. */
    - if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
    - continue;
    -
    - j++;
    -
    - if (helper_name && strncmp(cur->name, helper_name,
    - NF_CT_HELPER_NAME_LEN) != 0) {
    - continue;
    - }
    - if (tuple_set &&
    - (tuple.src.l3num != cur->tuple.src.l3num ||
    - tuple.dst.protonum != cur->tuple.dst.protonum))
    - continue;
    + list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
    + cur = &nlcth->helper;
    + j++;
    +
    + if (helper_name &&
    + strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
    + continue;
    +
    + if (tuple_set &&
    + (tuple.src.l3num != cur->tuple.src.l3num ||
    + tuple.dst.protonum != cur->tuple.dst.protonum))
    + continue;
    +
    + found = true;
    + nf_conntrack_helper_unregister(cur);
    + kfree(cur->expect_policy);

    - found = true;
    - nf_conntrack_helper_unregister(cur);
    - kfree(cur->expect_policy);
    - kfree(cur);
    - }
    + list_del(&nlcth->list);
    + kfree(nlcth);
    }
    +
    /* Make sure we return success if we flush and there is no helpers */
    return (found || j == 0) ? 0 : -ENOENT;
    }
    @@ -750,22 +741,16 @@ err_out:
    static void __exit nfnl_cthelper_exit(void)
    {
    struct nf_conntrack_helper *cur;
    - struct hlist_node *tmp;
    - int i;
    + struct nfnl_cthelper *nlcth, *n;

    nfnetlink_subsys_unregister(&nfnl_cthelper_subsys);

    - for (i=0; i<nf_ct_helper_hsize; i++) {
    - hlist_for_each_entry_safe(cur, tmp, &nf_ct_helper_hash[i],
    - hnode) {
    - /* skip non-userspace conntrack helpers. */
    - if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
    - continue;
    + list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
    + cur = &nlcth->helper;

    - nf_conntrack_helper_unregister(cur);
    - kfree(cur->expect_policy);
    - kfree(cur);
    - }
    + nf_conntrack_helper_unregister(cur);
    + kfree(cur->expect_policy);
    + kfree(nlcth);
    }
    }


    \
     
     \ /
      Last update: 2017-12-22 11:59    [W:2.482 / U:0.440 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site