Commit f32bb2aa authored by Florian Westphal's avatar Florian Westphal Committed by Greg Kroah-Hartman

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

commit 569ccae6 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: 0628b123 ("netfilter: nfnetlink: add batch support and use it from nf_tables")
Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 1af681da
...@@ -2200,41 +2200,46 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk, ...@@ -2200,41 +2200,46 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
} }
if (nlh->nlmsg_flags & NLM_F_REPLACE) { if (nlh->nlmsg_flags & NLM_F_REPLACE) {
if (nft_is_active_next(net, old_rule)) { 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 {
err = -ENOENT; err = -ENOENT;
goto err2; goto err2;
} }
} else if (nlh->nlmsg_flags & NLM_F_APPEND) trans = nft_trans_rule_add(&ctx, NFT_MSG_DELRULE,
if (old_rule) old_rule);
list_add_rcu(&rule->list, &old_rule->list); if (trans == NULL) {
else err = -ENOMEM;
list_add_tail_rcu(&rule->list, &chain->rules); goto err2;
else { }
if (old_rule) nft_deactivate_next(net, old_rule);
list_add_tail_rcu(&rule->list, &old_rule->list); chain->use--;
else
list_add_rcu(&rule->list, &chain->rules);
}
if (nft_trans_rule_add(&ctx, NFT_MSG_NEWRULE, rule) == NULL) { if (nft_trans_rule_add(&ctx, NFT_MSG_NEWRULE, rule) == NULL) {
err = -ENOMEM; err = -ENOMEM;
goto err3; 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++; chain->use++;
return 0; return 0;
err3:
list_del_rcu(&rule->list);
err2: err2:
nf_tables_rule_destroy(&ctx, rule); nf_tables_rule_destroy(&ctx, rule);
err1: err1:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment