Skip to content

Commit 0ce53ab

Browse files
PlaidCatPlaidCat
committed
netfilter: nf_tables: fix chain binding transaction logic
jira LE-1907 cve CVE-2023-3610 cve CVE-2023-3390 Rebuild_History Non-Buildable kernel-5.14.0-284.30.1.el9_2 commit-author Pablo Neira Ayuso <pablo@netfilter.org> commit 4bedf9e 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> (cherry picked from commit 4bedf9e) Signed-off-by: Jonathan Maple <jmaple@ciq.com>
1 parent 3868eb8 commit 0ce53ab

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
@@ -964,7 +964,10 @@ static inline struct nft_userdata *nft_userdata(const struct nft_rule *rule)
964964
return (void *)&rule->data[rule->dlen];
965965
}
966966

967-
void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *rule);
967+
void nft_rule_expr_activate(const struct nft_ctx *ctx, struct nft_rule *rule);
968+
void nft_rule_expr_deactivate(const struct nft_ctx *ctx, struct nft_rule *rule,
969+
enum nft_trans_phase phase);
970+
void nf_tables_rule_destroy(const struct nft_ctx *ctx, struct nft_rule *rule);
968971

969972
static inline void nft_set_elem_update_expr(const struct nft_set_ext *ext,
970973
struct nft_regs *regs,
@@ -1033,6 +1036,7 @@ int nft_setelem_validate(const struct nft_ctx *ctx, struct nft_set *set,
10331036
const struct nft_set_iter *iter,
10341037
struct nft_set_elem *elem);
10351038
int nft_set_catchall_validate(const struct nft_ctx *ctx, struct nft_set *set);
1039+
int nf_tables_bind_chain(const struct nft_ctx *ctx, struct nft_chain *chain);
10361040

10371041
enum nft_chain_types {
10381042
NFT_CHAIN_T_DEFAULT = 0,
@@ -1069,11 +1073,17 @@ int nft_chain_validate_dependency(const struct nft_chain *chain,
10691073
int nft_chain_validate_hooks(const struct nft_chain *chain,
10701074
unsigned int hook_flags);
10711075

1076+
static inline bool nft_chain_binding(const struct nft_chain *chain)
1077+
{
1078+
return chain->flags & NFT_CHAIN_BINDING;
1079+
}
1080+
10721081
static inline bool nft_chain_is_bound(struct nft_chain *chain)
10731082
{
10741083
return (chain->flags & NFT_CHAIN_BINDING) && chain->bound;
10751084
}
10761085

1086+
int nft_chain_add(struct nft_table *table, struct nft_chain *chain);
10771087
void nft_chain_del(struct nft_chain *chain);
10781088
void nf_tables_chain_destroy(struct nft_ctx *ctx);
10791089

@@ -1508,6 +1518,7 @@ struct nft_trans_rule {
15081518
struct nft_rule *rule;
15091519
struct nft_flow_rule *flow;
15101520
u32 rule_id;
1521+
bool bound;
15111522
};
15121523

15131524
#define nft_trans_rule(trans) \
@@ -1516,6 +1527,8 @@ struct nft_trans_rule {
15161527
(((struct nft_trans_rule *)trans->data)->flow)
15171528
#define nft_trans_rule_id(trans) \
15181529
(((struct nft_trans_rule *)trans->data)->rule_id)
1530+
#define nft_trans_rule_bound(trans) \
1531+
(((struct nft_trans_rule *)trans->data)->bound)
15191532

15201533
struct nft_trans_set {
15211534
struct nft_set *set;
@@ -1540,13 +1553,17 @@ struct nft_trans_set {
15401553
(((struct nft_trans_set *)trans->data)->gc_int)
15411554

15421555
struct nft_trans_chain {
1556+
struct nft_chain *chain;
15431557
bool update;
15441558
char *name;
15451559
struct nft_stats __percpu *stats;
15461560
u8 policy;
1561+
bool bound;
15471562
u32 chain_id;
15481563
};
15491564

1565+
#define nft_trans_chain(trans) \
1566+
(((struct nft_trans_chain *)trans->data)->chain)
15501567
#define nft_trans_chain_update(trans) \
15511568
(((struct nft_trans_chain *)trans->data)->update)
15521569
#define nft_trans_chain_name(trans) \
@@ -1555,6 +1572,8 @@ struct nft_trans_chain {
15551572
(((struct nft_trans_chain *)trans->data)->stats)
15561573
#define nft_trans_chain_policy(trans) \
15571574
(((struct nft_trans_chain *)trans->data)->policy)
1575+
#define nft_trans_chain_bound(trans) \
1576+
(((struct nft_trans_chain *)trans->data)->bound)
15581577
#define nft_trans_chain_id(trans) \
15591578
(((struct nft_trans_chain *)trans->data)->chain_id)
15601579

‎net/netfilter/nf_tables_api.c‎

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

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

@@ -359,8 +402,7 @@ static int nft_delchain(struct nft_ctx *ctx)
359402
return 0;
360403
}
361404

362-
static void nft_rule_expr_activate(const struct nft_ctx *ctx,
363-
struct nft_rule *rule)
405+
void nft_rule_expr_activate(const struct nft_ctx *ctx, struct nft_rule *rule)
364406
{
365407
struct nft_expr *expr;
366408

@@ -373,9 +415,8 @@ static void nft_rule_expr_activate(const struct nft_ctx *ctx,
373415
}
374416
}
375417

376-
static void nft_rule_expr_deactivate(const struct nft_ctx *ctx,
377-
struct nft_rule *rule,
378-
enum nft_trans_phase phase)
418+
void nft_rule_expr_deactivate(const struct nft_ctx *ctx, struct nft_rule *rule,
419+
enum nft_trans_phase phase)
379420
{
380421
struct nft_expr *expr;
381422

@@ -2094,7 +2135,7 @@ static int nft_basechain_init(struct nft_base_chain *basechain, u8 family,
20942135
return 0;
20952136
}
20962137

2097-
static int nft_chain_add(struct nft_table *table, struct nft_chain *chain)
2138+
int nft_chain_add(struct nft_table *table, struct nft_chain *chain)
20982139
{
20992140
int err;
21002141

@@ -3220,8 +3261,7 @@ static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info,
32203261
return err;
32213262
}
32223263

3223-
static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
3224-
struct nft_rule *rule)
3264+
void nf_tables_rule_destroy(const struct nft_ctx *ctx, struct nft_rule *rule)
32253265
{
32263266
struct nft_expr *expr, *next;
32273267

@@ -3238,7 +3278,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
32383278
kfree(rule);
32393279
}
32403280

3241-
void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *rule)
3281+
static void nf_tables_rule_release(const struct nft_ctx *ctx, struct nft_rule *rule)
32423282
{
32433283
nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_RELEASE);
32443284
nf_tables_rule_destroy(ctx, rule);
@@ -6305,23 +6345,13 @@ static int nf_tables_newsetelem(struct sk_buff *skb,
63056345
void nft_data_hold(const struct nft_data *data, enum nft_data_types type)
63066346
{
63076347
struct nft_chain *chain;
6308-
struct nft_rule *rule;
63096348

63106349
if (type == NFT_DATA_VERDICT) {
63116350
switch (data->verdict.code) {
63126351
case NFT_JUMP:
63136352
case NFT_GOTO:
63146353
chain = data->verdict.chain;
63156354
chain->use++;
6316-
6317-
if (!nft_chain_is_bound(chain))
6318-
break;
6319-
6320-
chain->table->use++;
6321-
list_for_each_entry(rule, &chain->rules, list)
6322-
chain->use++;
6323-
6324-
nft_chain_add(chain->table, chain);
63256355
break;
63266356
}
63276357
}
@@ -9127,7 +9157,7 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
91279157
kfree(nft_trans_chain_name(trans));
91289158
nft_trans_destroy(trans);
91299159
} else {
9130-
if (nft_chain_is_bound(trans->ctx.chain)) {
9160+
if (nft_trans_chain_bound(trans)) {
91319161
nft_trans_destroy(trans);
91329162
break;
91339163
}
@@ -9144,6 +9174,10 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
91449174
nft_trans_destroy(trans);
91459175
break;
91469176
case NFT_MSG_NEWRULE:
9177+
if (nft_trans_rule_bound(trans)) {
9178+
nft_trans_destroy(trans);
9179+
break;
9180+
}
91479181
trans->ctx.chain->use--;
91489182
list_del_rcu(&nft_trans_rule(trans)->list);
91499183
nft_rule_expr_deactivate(&trans->ctx,
@@ -9695,22 +9729,12 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
96959729
static void nft_verdict_uninit(const struct nft_data *data)
96969730
{
96979731
struct nft_chain *chain;
9698-
struct nft_rule *rule;
96999732

97009733
switch (data->verdict.code) {
97019734
case NFT_JUMP:
97029735
case NFT_GOTO:
97039736
chain = data->verdict.chain;
97049737
chain->use--;
9705-
9706-
if (!nft_chain_is_bound(chain))
9707-
break;
9708-
9709-
chain->table->use--;
9710-
list_for_each_entry(rule, &chain->rules, list)
9711-
chain->use--;
9712-
9713-
nft_chain_del(chain);
97149738
break;
97159739
}
97169740
}

‎net/netfilter/nft_immediate.c‎

Lines changed: 78 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,9 @@ static int nft_immediate_init(const struct nft_ctx *ctx,
6060
switch (priv->data.verdict.code) {
6161
case NFT_JUMP:
6262
case NFT_GOTO:
63-
if (nft_chain_is_bound(chain)) {
64-
err = -EBUSY;
65-
goto err1;
66-
}
67-
chain->bound = true;
63+
err = nf_tables_bind_chain(ctx, chain);
64+
if (err < 0)
65+
return err;
6866
break;
6967
default:
7068
break;
@@ -82,6 +80,31 @@ static void nft_immediate_activate(const struct nft_ctx *ctx,
8280
const struct nft_expr *expr)
8381
{
8482
const struct nft_immediate_expr *priv = nft_expr_priv(expr);
83+
const struct nft_data *data = &priv->data;
84+
struct nft_ctx chain_ctx;
85+
struct nft_chain *chain;
86+
struct nft_rule *rule;
87+
88+
if (priv->dreg == NFT_REG_VERDICT) {
89+
switch (data->verdict.code) {
90+
case NFT_JUMP:
91+
case NFT_GOTO:
92+
chain = data->verdict.chain;
93+
if (!nft_chain_binding(chain))
94+
break;
95+
96+
chain_ctx = *ctx;
97+
chain_ctx.chain = chain;
98+
99+
list_for_each_entry(rule, &chain->rules, list)
100+
nft_rule_expr_activate(&chain_ctx, rule);
101+
102+
nft_clear(ctx->net, chain);
103+
break;
104+
default:
105+
break;
106+
}
107+
}
85108

86109
return nft_data_hold(&priv->data, nft_dreg_to_type(priv->dreg));
87110
}
@@ -91,6 +114,40 @@ static void nft_immediate_deactivate(const struct nft_ctx *ctx,
91114
enum nft_trans_phase phase)
92115
{
93116
const struct nft_immediate_expr *priv = nft_expr_priv(expr);
117+
const struct nft_data *data = &priv->data;
118+
struct nft_ctx chain_ctx;
119+
struct nft_chain *chain;
120+
struct nft_rule *rule;
121+
122+
if (priv->dreg == NFT_REG_VERDICT) {
123+
switch (data->verdict.code) {
124+
case NFT_JUMP:
125+
case NFT_GOTO:
126+
chain = data->verdict.chain;
127+
if (!nft_chain_binding(chain))
128+
break;
129+
130+
chain_ctx = *ctx;
131+
chain_ctx.chain = chain;
132+
133+
list_for_each_entry(rule, &chain->rules, list)
134+
nft_rule_expr_deactivate(&chain_ctx, rule, phase);
135+
136+
switch (phase) {
137+
case NFT_TRANS_PREPARE:
138+
nft_deactivate_next(ctx->net, chain);
139+
break;
140+
default:
141+
nft_chain_del(chain);
142+
chain->bound = false;
143+
chain->table->use--;
144+
break;
145+
}
146+
break;
147+
default:
148+
break;
149+
}
150+
}
94151

95152
if (phase == NFT_TRANS_COMMIT)
96153
return;
@@ -115,15 +172,27 @@ static void nft_immediate_destroy(const struct nft_ctx *ctx,
115172
case NFT_GOTO:
116173
chain = data->verdict.chain;
117174

118-
if (!nft_chain_is_bound(chain))
175+
if (!nft_chain_binding(chain))
176+
break;
177+
178+
/* Rule construction failed, but chain is already bound:
179+
* let the transaction records release this chain and its rules.
180+
*/
181+
if (chain->bound) {
182+
chain->use--;
119183
break;
184+
}
120185

186+
/* Rule has been deleted, release chain and its rules. */
121187
chain_ctx = *ctx;
122188
chain_ctx.chain = chain;
123189

124-
list_for_each_entry_safe(rule, n, &chain->rules, list)
125-
nf_tables_rule_release(&chain_ctx, rule);
126-
190+
chain->use--;
191+
list_for_each_entry_safe(rule, n, &chain->rules, list) {
192+
chain->use--;
193+
list_del(&rule->list);
194+
nf_tables_rule_destroy(&chain_ctx, rule);
195+
}
127196
nf_tables_chain_destroy(&chain_ctx);
128197
break;
129198
default:

0 commit comments

Comments
 (0)