netfilter: nf_tables: can't fail after linking rule into active rule list

rules in nftables a free'd using kfree, but protected by rcu, i.e. we
must wait for a grace period to elapse.

Normal removal patch does this, but nf_tables_newrule() doesn't obey
this rule during error handling.

It calls nft_trans_rule_add() *after* linking rule, and, if that
fails to allocate memory, it unlinks the rule and then kfree() it --
this is unsafe.

Switch order -- first add rule to transaction list, THEN link it
to public list.

Note: nft_trans_rule_add() uses GFP_KERNEL; it will not fail so this
is not a problem in practice (spotted only during code review).

Fixes: 0628b123c96d12 ("netfilter: nfnetlink: add batch support and use it from nf_tables")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 9134cc429ad485aa86301019f50fb4f5d2172b52..b1984f8f72535b11937be9108693f8402b5b4c74 100644 (file)
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2361,41 +2361,46 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
        }
 
        if (nlh->nlmsg_flags & NLM_F_REPLACE) {
-               if (nft_is_active_next(net, old_rule)) {
-                       trans = nft_trans_rule_add(&ctx, NFT_MSG_DELRULE,
-                                                  old_rule);
-                       if (trans == NULL) {
-                               err = -ENOMEM;
-                               goto err2;
-                       }
-                       nft_deactivate_next(net, old_rule);
-                       chain->use--;
-                       list_add_tail_rcu(&rule->list, &old_rule->list);
-               } else {
+               if (!nft_is_active_next(net, old_rule)) {
                        err = -ENOENT;
                        goto err2;
                }
-       } else if (nlh->nlmsg_flags & NLM_F_APPEND)
-               if (old_rule)
-                       list_add_rcu(&rule->list, &old_rule->list);
-               else
-                       list_add_tail_rcu(&rule->list, &chain->rules);
-       else {
-               if (old_rule)
-                       list_add_tail_rcu(&rule->list, &old_rule->list);
-               else
-                       list_add_rcu(&rule->list, &chain->rules);
-       }
+               trans = nft_trans_rule_add(&ctx, NFT_MSG_DELRULE,
+                                          old_rule);
+               if (trans == NULL) {
+                       err = -ENOMEM;
+                       goto err2;
+               }
+               nft_deactivate_next(net, old_rule);
+               chain->use--;
 
-       if (nft_trans_rule_add(&ctx, NFT_MSG_NEWRULE, rule) == NULL) {
-               err = -ENOMEM;
-               goto err3;
+               if (nft_trans_rule_add(&ctx, NFT_MSG_NEWRULE, rule) == NULL) {
+                       err = -ENOMEM;
+                       goto err2;
+               }
+
+               list_add_tail_rcu(&rule->list, &old_rule->list);
+       } else {
+               if (nft_trans_rule_add(&ctx, NFT_MSG_NEWRULE, rule) == NULL) {
+                       err = -ENOMEM;
+                       goto err2;
+               }
+
+               if (nlh->nlmsg_flags & NLM_F_APPEND) {
+                       if (old_rule)
+                               list_add_rcu(&rule->list, &old_rule->list);
+                       else
+                               list_add_tail_rcu(&rule->list, &chain->rules);
+                } else {
+                       if (old_rule)
+                               list_add_tail_rcu(&rule->list, &old_rule->list);
+                       else
+                               list_add_rcu(&rule->list, &chain->rules);
+               }
        }
        chain->use++;
        return 0;
 
-err3:
-       list_del_rcu(&rule->list);
 err2:
        nf_tables_rule_destroy(&ctx, rule);
 err1: