| From: Florian Westphal <fw@strlen.de> |
| Date: Tue, 10 Apr 2018 09:30:27 +0200 |
| Subject: netfilter: nf_tables: can't fail after linking rule into active rule |
| list |
| |
| commit 569ccae68b38654f04b6842b034aa33857f605fe upstream. |
| |
| 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> |
| [bwh: Backported to 3.16: Some function names are different] |
| Signed-off-by: Ben Hutchings <ben@decadent.org.uk> |
| --- |
| net/netfilter/nf_tables_api.c | 59 +++++++++++++++++++---------------- |
| 1 file changed, 32 insertions(+), 27 deletions(-) |
| |
| --- a/net/netfilter/nf_tables_api.c |
| +++ b/net/netfilter/nf_tables_api.c |
| @@ -1829,41 +1829,46 @@ static int nf_tables_newrule(struct sock |
| } |
| |
| if (nlh->nlmsg_flags & NLM_F_REPLACE) { |
| - if (nft_rule_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_rule_disactivate_next(net, old_rule); |
| - chain->use--; |
| - list_add_tail_rcu(&rule->list, &old_rule->list); |
| - } else { |
| + if (!nft_rule_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_rule_disactivate_next(net, old_rule); |
| + chain->use--; |
| + |
| + if (nft_trans_rule_add(&ctx, NFT_MSG_NEWRULE, rule) == NULL) { |
| + err = -ENOMEM; |
| + goto err2; |
| + } |
| |
| - if (nft_trans_rule_add(&ctx, NFT_MSG_NEWRULE, rule) == NULL) { |
| - err = -ENOMEM; |
| - goto err3; |
| + 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: |