Skip to content

Commit 4bedf9e

Browse files
ummakynesummakynes
committed
netfilter: nf_tables: fix chain binding transaction logic
Add bound flag to rule and chain transactions as in 6a0a8d1 ("netfilter: nf_tables: use-after-free in failing rule with bound set") to skip them in case that the chain is already bound from the abort path. This patch fixes an imbalance in the chain use refcnt that triggers a WARN_ON on the table and chain destroy path. This patch also disallows nested chain bindings, which is not supported from userspace. The logic to deal with chain binding in nft_data_hold() and nft_data_release() is not correct. The NFT_TRANS_PREPARE state needs a special handling in case a chain is bound but next expressions in the same rule fail to initialize as described by 1240eb9 ("netfilter: nf_tables: incorrect error path handling with NFT_MSG_NEWRULE"). The chain is left bound if rule construction fails, so the objects stored in this chain (and the chain itself) are released by the transaction records from the abort path, follow up patch ("netfilter: nf_tables: add NFT_TRANS_PREPARE_ERROR to deal with bound set/chain") completes this error handling. When deleting an existing rule, chain bound flag is set off so the rule expression .destroy path releases the objects. Fixes: d0e2c7d ("netfilter: nf_tables: add NFT_CHAIN_BINDING") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
1 parent d7fce52 commit 4bedf9e

File tree

3 files changed

+153
-41
lines changed

3 files changed

+153
-41
lines changed

‎include/net/netfilter/nf_tables.h‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1009,7 +1009,10 @@ static inline struct nft_userdata *nft_userdata(const struct nft_rule *rule)
10091009
return (void *)&rule->data[rule->dlen];
10101010
}
10111011

1012-
void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *rule);
1012+
void nft_rule_expr_activate(const struct nft_ctx *ctx, struct nft_rule *rule);
1013+
void nft_rule_expr_deactivate(const struct nft_ctx *ctx, struct nft_rule *rule,
1014+
enum nft_trans_phase phase);
1015+
void nf_tables_rule_destroy(const struct nft_ctx *ctx, struct nft_rule *rule);
10131016

10141017
static inline void nft_set_elem_update_expr(const struct nft_set_ext *ext,
10151018
struct nft_regs *regs,
@@ -1104,6 +1107,7 @@ int nft_setelem_validate(const struct nft_ctx *ctx, struct nft_set *set,
11041107
const struct nft_set_iter *iter,
11051108
struct nft_set_elem *elem);
11061109
int nft_set_catchall_validate(const struct nft_ctx *ctx, struct nft_set *set);
1110+
int nf_tables_bind_chain(const struct nft_ctx *ctx, struct nft_chain *chain);
11071111

11081112
enum nft_chain_types {
11091113
NFT_CHAIN_T_DEFAULT = 0,
@@ -1140,11 +1144,17 @@ int nft_chain_validate_dependency(const struct nft_chain *chain,
11401144
int nft_chain_validate_hooks(const struct nft_chain *chain,
11411145
unsigned int hook_flags);
11421146

1147+
static inline bool nft_chain_binding(const struct nft_chain *chain)
1148+
{
1149+
return chain->flags & NFT_CHAIN_BINDING;
1150+
}
1151+
11431152
static inline bool nft_chain_is_bound(struct nft_chain *chain)
11441153
{
11451154
return (chain->flags & NFT_CHAIN_BINDING) && chain->bound;
11461155
}
11471156

1157+
int nft_chain_add(struct nft_table *table, struct nft_chain *chain);
11481158
void nft_chain_del(struct nft_chain *chain);
11491159
void nf_tables_chain_destroy(struct nft_ctx *ctx);
11501160

@@ -1575,6 +1585,7 @@ struct nft_trans_rule {
15751585
struct nft_rule *rule;
15761586
struct nft_flow_rule *flow;
15771587
u32 rule_id;
1588+
bool bound;
15781589
};
15791590

15801591
#define nft_trans_rule(trans) \
@@ -1583,6 +1594,8 @@ struct nft_trans_rule {
15831594
(((struct nft_trans_rule *)trans->data)->flow)
15841595
#define nft_trans_rule_id(trans) \
15851596
(((struct nft_trans_rule *)trans->data)->rule_id)
1597+
#define nft_trans_rule_bound(trans) \
1598+
(((struct nft_trans_rule *)trans->data)->bound)
15861599

15871600
struct nft_trans_set {
15881601
struct nft_set *set;
@@ -1607,15 +1620,19 @@ struct nft_trans_set {
16071620
(((struct nft_trans_set *)trans->data)->gc_int)
16081621

16091622
struct nft_trans_chain {
1623+
struct nft_chain *chain;
16101624
bool update;
16111625
char *name;
16121626
struct nft_stats __percpu *stats;
16131627
u8 policy;
1628+
bool bound;
16141629
u32 chain_id;
16151630
struct nft_base_chain *basechain;
16161631
struct list_head hook_list;
16171632
};
16181633

1634+
#define nft_trans_chain(trans) \
1635+
(((struct nft_trans_chain *)trans->data)->chain)
16191636
#define nft_trans_chain_update(trans) \
16201637
(((struct nft_trans_chain *)trans->data)->update)
16211638
#define nft_trans_chain_name(trans) \
@@ -1624,6 +1641,8 @@ struct nft_trans_chain {
16241641
(((struct nft_trans_chain *)trans->data)->stats)
16251642
#define nft_trans_chain_policy(trans) \
16261643
(((struct nft_trans_chain *)trans->data)->policy)
1644+
#define nft_trans_chain_bound(trans) \
1645+
(((struct nft_trans_chain *)trans->data)->bound)
16271646
#define nft_trans_chain_id(trans) \
16281647
(((struct nft_trans_chain *)trans->data)->chain_id)
16291648
#define nft_trans_basechain(trans) \

‎net/netfilter/nf_tables_api.c‎

Lines changed: 55 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,48 @@ static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set)
193193
}
194194
}
195195

196+
static void nft_chain_trans_bind(const struct nft_ctx *ctx, struct nft_chain *chain)
197+
{
198+
struct nftables_pernet *nft_net;
199+
struct net *net = ctx->net;
200+
struct nft_trans *trans;
201+
202+
if (!nft_chain_binding(chain))
203+
return;
204+
205+
nft_net = nft_pernet(net);
206+
list_for_each_entry_reverse(trans, &nft_net->commit_list, list) {
207+
switch (trans->msg_type) {
208+
case NFT_MSG_NEWCHAIN:
209+
if (nft_trans_chain(trans) == chain)
210+
nft_trans_chain_bound(trans) = true;
211+
break;
212+
case NFT_MSG_NEWRULE:
213+
if (trans->ctx.chain == chain)
214+
nft_trans_rule_bound(trans) = true;
215+
break;
216+
}
217+
}
218+
}
219+
220+
int nf_tables_bind_chain(const struct nft_ctx *ctx, struct nft_chain *chain)
221+
{
222+
if (!nft_chain_binding(chain))
223+
return 0;
224+
225+
if (nft_chain_binding(ctx->chain))
226+
return -EOPNOTSUPP;
227+
228+
if (chain->bound)
229+
return -EBUSY;
230+
231+
chain->bound = true;
232+
chain->use++;
233+
nft_chain_trans_bind(ctx, chain);
234+
235+
return 0;
236+
}
237+
196238
static int nft_netdev_register_hooks(struct net *net,
197239
struct list_head *hook_list)
198240
{
@@ -338,8 +380,9 @@ static struct nft_trans *nft_trans_chain_add(struct nft_ctx *ctx, int msg_type)
338380
ntohl(nla_get_be32(ctx->nla[NFTA_CHAIN_ID]));
339381
}
340382
}
341-
383+
nft_trans_chain(trans) = ctx->chain;
342384
nft_trans_commit_list_add_tail(ctx->net, trans);
385+
343386
return trans;
344387
}
345388

@@ -357,8 +400,7 @@ static int nft_delchain(struct nft_ctx *ctx)
357400
return 0;
358401
}
359402

360-
static void nft_rule_expr_activate(const struct nft_ctx *ctx,
361-
struct nft_rule *rule)
403+
void nft_rule_expr_activate(const struct nft_ctx *ctx, struct nft_rule *rule)
362404
{
363405
struct nft_expr *expr;
364406

@@ -371,9 +413,8 @@ static void nft_rule_expr_activate(const struct nft_ctx *ctx,
371413
}
372414
}
373415

374-
static void nft_rule_expr_deactivate(const struct nft_ctx *ctx,
375-
struct nft_rule *rule,
376-
enum nft_trans_phase phase)
416+
void nft_rule_expr_deactivate(const struct nft_ctx *ctx, struct nft_rule *rule,
417+
enum nft_trans_phase phase)
377418
{
378419
struct nft_expr *expr;
379420

@@ -2226,7 +2267,7 @@ static int nft_basechain_init(struct nft_base_chain *basechain, u8 family,
22262267
return 0;
22272268
}
22282269

2229-
static int nft_chain_add(struct nft_table *table, struct nft_chain *chain)
2270+
int nft_chain_add(struct nft_table *table, struct nft_chain *chain)
22302271
{
22312272
int err;
22322273

@@ -3490,8 +3531,7 @@ static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info,
34903531
return err;
34913532
}
34923533

3493-
static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
3494-
struct nft_rule *rule)
3534+
void nf_tables_rule_destroy(const struct nft_ctx *ctx, struct nft_rule *rule)
34953535
{
34963536
struct nft_expr *expr, *next;
34973537

@@ -3508,7 +3548,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
35083548
kfree(rule);
35093549
}
35103550

3511-
void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *rule)
3551+
static void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *rule)
35123552
{
35133553
nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_RELEASE);
35143554
nf_tables_rule_destroy(ctx, rule);
@@ -6638,23 +6678,13 @@ static int nf_tables_newsetelem(struct sk_buff *skb,
66386678
void nft_data_hold(const struct nft_data *data, enum nft_data_types type)
66396679
{
66406680
struct nft_chain *chain;
6641-
struct nft_rule *rule;
66426681

66436682
if (type == NFT_DATA_VERDICT) {
66446683
switch (data->verdict.code) {
66456684
case NFT_JUMP:
66466685
case NFT_GOTO:
66476686
chain = data->verdict.chain;
66486687
chain->use++;
6649-
6650-
if (!nft_chain_is_bound(chain))
6651-
break;
6652-
6653-
chain->table->use++;
6654-
list_for_each_entry(rule, &chain->rules, list)
6655-
chain->use++;
6656-
6657-
nft_chain_add(chain->table, chain);
66586688
break;
66596689
}
66606690
}
@@ -9677,7 +9707,7 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
96779707
kfree(nft_trans_chain_name(trans));
96789708
nft_trans_destroy(trans);
96799709
} else {
9680-
if (nft_chain_is_bound(trans->ctx.chain)) {
9710+
if (nft_trans_chain_bound(trans)) {
96819711
nft_trans_destroy(trans);
96829712
break;
96839713
}
@@ -9700,6 +9730,10 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
97009730
nft_trans_destroy(trans);
97019731
break;
97029732
case NFT_MSG_NEWRULE:
9733+
if (nft_trans_rule_bound(trans)) {
9734+
nft_trans_destroy(trans);
9735+
break;
9736+
}
97039737
trans->ctx.chain->use--;
97049738
list_del_rcu(&nft_trans_rule(trans)->list);
97059739
nft_rule_expr_deactivate(&trans->ctx,
@@ -10263,22 +10297,12 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
1026310297
static void nft_verdict_uninit(const struct nft_data *data)
1026410298
{
1026510299
struct nft_chain *chain;
10266-
struct nft_rule *rule;
1026710300

1026810301
switch (data->verdict.code) {
1026910302
case NFT_JUMP:
1027010303
case NFT_GOTO:
1027110304
chain = data->verdict.chain;
1027210305
chain->use--;
10273-
10274-
if (!nft_chain_is_bound(chain))
10275-
break;
10276-
10277-
chain->table->use--;
10278-
list_for_each_entry(rule, &chain->rules, list)
10279-
chain->use--;
10280-
10281-
nft_chain_del(chain);
1028210306
break;
1028310307
}
1028410308
}

‎net/netfilter/nft_immediate.c‎

Lines changed: 78 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,9 @@ static int nft_immediate_init(const struct nft_ctx *ctx,
7676
switch (priv->data.verdict.code) {
7777
case NFT_JUMP:
7878
case NFT_GOTO:
79-
if (nft_chain_is_bound(chain)) {
80-
err = -EBUSY;
81-
goto err1;
82-
}
83-
chain->bound = true;
79+
err = nf_tables_bind_chain(ctx, chain);
80+
if (err < 0)
81+
return err;
8482
break;
8583
default:
8684
break;
@@ -98,6 +96,31 @@ static void nft_immediate_activate(const struct nft_ctx *ctx,
9896
const struct nft_expr *expr)
9997
{
10098
const struct nft_immediate_expr *priv = nft_expr_priv(expr);
99+
const struct nft_data *data = &priv->data;
100+
struct nft_ctx chain_ctx;
101+
struct nft_chain *chain;
102+
struct nft_rule *rule;
103+
104+
if (priv->dreg == NFT_REG_VERDICT) {
105+
switch (data->verdict.code) {
106+
case NFT_JUMP:
107+
case NFT_GOTO:
108+
chain = data->verdict.chain;
109+
if (!nft_chain_binding(chain))
110+
break;
111+
112+
chain_ctx = *ctx;
113+
chain_ctx.chain = chain;
114+
115+
list_for_each_entry(rule, &chain->rules, list)
116+
nft_rule_expr_activate(&chain_ctx, rule);
117+
118+
nft_clear(ctx->net, chain);
119+
break;
120+
default:
121+
break;
122+
}
123+
}
101124

102125
return nft_data_hold(&priv->data, nft_dreg_to_type(priv->dreg));
103126
}
@@ -107,6 +130,40 @@ static void nft_immediate_deactivate(const struct nft_ctx *ctx,
107130
enum nft_trans_phase phase)
108131
{
109132
const struct nft_immediate_expr *priv = nft_expr_priv(expr);
133+
const struct nft_data *data = &priv->data;
134+
struct nft_ctx chain_ctx;
135+
struct nft_chain *chain;
136+
struct nft_rule *rule;
137+
138+
if (priv->dreg == NFT_REG_VERDICT) {
139+
switch (data->verdict.code) {
140+
case NFT_JUMP:
141+
case NFT_GOTO:
142+
chain = data->verdict.chain;
143+
if (!nft_chain_binding(chain))
144+
break;
145+
146+
chain_ctx = *ctx;
147+
chain_ctx.chain = chain;
148+
149+
list_for_each_entry(rule, &chain->rules, list)
150+
nft_rule_expr_deactivate(&chain_ctx, rule, phase);
151+
152+
switch (phase) {
153+
case NFT_TRANS_PREPARE:
154+
nft_deactivate_next(ctx->net, chain);
155+
break;
156+
default:
157+
nft_chain_del(chain);
158+
chain->bound = false;
159+
chain->table->use--;
160+
break;
161+
}
162+
break;
163+
default:
164+
break;
165+
}
166+
}
110167

111168
if (phase == NFT_TRANS_COMMIT)
112169
return;
@@ -131,15 +188,27 @@ static void nft_immediate_destroy(const struct nft_ctx *ctx,
131188
case NFT_GOTO:
132189
chain = data->verdict.chain;
133190

134-
if (!nft_chain_is_bound(chain))
191+
if (!nft_chain_binding(chain))
192+
break;
193+
194+
/* Rule construction failed, but chain is already bound:
195+
* let the transaction records release this chain and its rules.
196+
*/
197+
if (chain->bound) {
198+
chain->use--;
135199
break;
200+
}
136201

202+
/* Rule has been deleted, release chain and its rules. */
137203
chain_ctx = *ctx;
138204
chain_ctx.chain = chain;
139205

140-
list_for_each_entry_safe(rule, n, &chain->rules, list)
141-
nf_tables_rule_release(&chain_ctx, rule);
142-
206+
chain->use--;
207+
list_for_each_entry_safe(rule, n, &chain->rules, list) {
208+
chain->use--;
209+
list_del(&rule->list);
210+
nf_tables_rule_destroy(&chain_ctx, rule);
211+
}
143212
nf_tables_chain_destroy(&chain_ctx);
144213
break;
145214
default:

0 commit comments

Comments
 (0)