| Message ID | 20210624195718.170796-1-colin.king@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [next] netfilter: nf_tables: Fix dereference of null pointer flow | expand |
Btw, why is there no clean up if nft_table_validate() fails?
net/netfilter/nf_tables_api.c
3432 list_add_tail_rcu(&rule->list, &old_rule->list);
3433 else
3434 list_add_rcu(&rule->list, &chain->rules);
3435 }
3436 }
3437 kvfree(expr_info);
3438 chain->use++;
3439
3440 if (flow)
3441 nft_trans_flow_rule(trans) = flow;
3442
3443 if (nft_net->validate_state == NFT_VALIDATE_DO)
3444 return nft_table_validate(net, table);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The cleanup for this would be quite involved unfortunately... Not
necessarily something to attempt without being able to test the code.
3445
3446 return 0;
3447
3448 err_destroy_flow_rule:
3449 nft_flow_rule_destroy(flow);
3450 err_release_rule:
3451 nf_tables_rule_release(&ctx, rule);
3452 err_release_expr:
3453 for (i = 0; i < n; i++) {
3454 if (expr_info[i].ops) {
3455 module_put(expr_info[i].ops->type->owner);
3456 if (expr_info[i].ops->type->release_ops)
3457 expr_info[i].ops->type->release_ops(expr_info[i].ops);
3458 }
3459 }
3460 kvfree(expr_info);
3461
3462 return err;
3463 }
regards,
dan carpenter
Hi, On Fri, Jun 25, 2021 at 12:59:01PM +0300, Dan Carpenter wrote: > Btw, why is there no clean up if nft_table_validate() fails? See below. > net/netfilter/nf_tables_api.c > 3432 list_add_tail_rcu(&rule->list, &old_rule->list); > 3433 else > 3434 list_add_rcu(&rule->list, &chain->rules); > 3435 } > 3436 } > 3437 kvfree(expr_info); > 3438 chain->use++; > 3439 > 3440 if (flow) > 3441 nft_trans_flow_rule(trans) = flow; > 3442 > 3443 if (nft_net->validate_state == NFT_VALIDATE_DO) > 3444 return nft_table_validate(net, table); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > The cleanup for this would be quite involved unfortunately... Not > necessarily something to attempt without being able to test the code. At this stage, the transaction has been already registered in the list, and the nf_tables_abort() path takes care of undoing what has been updated in the preparation phase. Having said this, Colin patch is correct, it's fixing up the error path.
On Fri, Jun 25, 2021 at 10:06:26AM +0000, Walter Harms wrote: > hi Colin, > most free_something_functions accept NULL > these days, perhaps it would be more efficient > to add a check in nft_flow_rule_destroy(). > There is a chance that this will catch the same > mistake in future also. I'm fine with Colin patch. Thanks.
On Fri, Jun 25, 2021 at 12:20:21PM +0200, Pablo Neira Ayuso wrote: > Hi, > > On Fri, Jun 25, 2021 at 12:59:01PM +0300, Dan Carpenter wrote: > > Btw, why is there no clean up if nft_table_validate() fails? > > See below. > > > net/netfilter/nf_tables_api.c > > 3432 list_add_tail_rcu(&rule->list, &old_rule->list); > > 3433 else > > 3434 list_add_rcu(&rule->list, &chain->rules); > > 3435 } > > 3436 } > > 3437 kvfree(expr_info); > > 3438 chain->use++; > > 3439 > > 3440 if (flow) > > 3441 nft_trans_flow_rule(trans) = flow; > > 3442 > > 3443 if (nft_net->validate_state == NFT_VALIDATE_DO) > > 3444 return nft_table_validate(net, table); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > The cleanup for this would be quite involved unfortunately... Not > > necessarily something to attempt without being able to test the code. > > At this stage, the transaction has been already registered in the > list, and the nf_tables_abort() path takes care of undoing what has > been updated in the preparation phase. > Ah... Thanks. regards, dan carpenter
On Thu, Jun 24, 2021 at 08:57:18PM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > In the case where chain->flags & NFT_CHAIN_HW_OFFLOAD is false then > nft_flow_rule_create is not called and flow is NULL. The subsequent > error handling execution via label err_destroy_flow_rule will lead > to a null pointer dereference on flow when calling nft_flow_rule_destroy. > Since the error path to err_destroy_flow_rule has to cater for null > and non-null flows, only call nft_flow_rule_destroy if flow is non-null > to fix this issue. Applied, thanks.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 390d4466567f..de182d1f7c4e 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3446,7 +3446,8 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info, return 0; err_destroy_flow_rule: - nft_flow_rule_destroy(flow); + if (flow) + nft_flow_rule_destroy(flow); err_release_rule: nf_tables_rule_release(&ctx, rule); err_release_expr: