From patchwork Wed May 30 10:36:52 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pablo Neira Ayuso X-Patchwork-Id: 922700 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netfilter-devel-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=netfilter.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 40wn8Z1gstz9s0q for ; Wed, 30 May 2018 20:37:02 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752095AbeE3KhB (ORCPT ); Wed, 30 May 2018 06:37:01 -0400 Received: from mail.us.es ([193.147.175.20]:51130 "EHLO mail.us.es" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750967AbeE3KhA (ORCPT ); Wed, 30 May 2018 06:37:00 -0400 Received: from antivirus1-rhel7.int (unknown [192.168.2.11]) by mail.us.es (Postfix) with ESMTP id E1354C3480 for ; Wed, 30 May 2018 12:35:49 +0200 (CEST) Received: from antivirus1-rhel7.int (localhost [127.0.0.1]) by antivirus1-rhel7.int (Postfix) with ESMTP id BE7CBDA81D for ; Wed, 30 May 2018 12:35:49 +0200 (CEST) Received: by antivirus1-rhel7.int (Postfix, from userid 99) id BD3D2DA81B; Wed, 30 May 2018 12:35:49 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on antivirus1-rhel7.int X-Spam-Level: X-Spam-Status: No, score=-108.2 required=7.5 tests=ALL_TRUSTED,BAYES_50, SMTPAUTH_US2,USER_IN_WHITELIST autolearn=disabled version=3.4.1 Received: from antivirus1-rhel7.int (localhost [127.0.0.1]) by antivirus1-rhel7.int (Postfix) with ESMTP id 54CE4DA792 for ; Wed, 30 May 2018 12:35:47 +0200 (CEST) Received: from 192.168.1.97 (192.168.1.97) by antivirus1-rhel7.int (F-Secure/fsigk_smtp/550/antivirus1-rhel7.int); Wed, 30 May 2018 12:35:47 +0200 (CEST) X-Virus-Status: clean(F-Secure/fsigk_smtp/550/antivirus1-rhel7.int) Received: from salvia.here (sys.soleta.eu [212.170.55.40]) (Authenticated sender: pneira@us.es) by entrada.int (Postfix) with ESMTPA id 2B04D411A646 for ; Wed, 30 May 2018 12:35:47 +0200 (CEST) X-SMTPAUTHUS: auth mail.us.es From: Pablo Neira Ayuso To: netfilter-devel@vger.kernel.org Subject: [PATCH nf-next, v4] netfilter: nf_tables: fix chain dependency validation Date: Wed, 30 May 2018 12:36:52 +0200 Message-Id: <20180530103652.24411-1-pablo@netfilter.org> X-Mailer: git-send-email 2.11.0 X-Virus-Scanned: ClamAV using ClamSMTP Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org The following ruleset: add table ip filter add chain ip filter input { type filter hook input priority 4; } add chain ip filter ap add rule ip filter input jump ap add rule ip filter ap masquerade results in a panic, because the masquerade extension should be rejected from the filter chain. The existing validation is missing a chain dependency check when the rule is added to the non-base chain. This patch fixes the problem by walking down the rules from the basechains, searching for either immediate or lookup expressions, then jumping to non-base chains and again walking down the rules to perform the expression validation, so we make sure the full ruleset graph is validated. This is done only once from the commit phase, in case of problem, we abort the transaction and perform fine grain validation for error reporting. This patch requires 003087911af2 ("netfilter: nfnetlink: allow commit to fail") to achieve this behaviour. As a result of this patch, nf_tables_check_loops() doesn't use ->validate to check for loops, instead it just checks for immediate expressions. Reported-by: Taehee Yoo Signed-off-by: Pablo Neira Ayuso --- v4: do not check for net->nft.validate from commit phase, this will always evaluate true under normal circunstances, spotted by Florian. Add WARN_ON_ONCE() on case that should never happen. include/net/netfilter/nf_tables.h | 2 + include/net/netfilter/nf_tables_core.h | 8 +++ include/net/netns/nftables.h | 1 + net/netfilter/nf_tables_api.c | 100 +++++++++++++++++++++++++-------- net/netfilter/nft_immediate.c | 27 ++++++--- net/netfilter/nft_lookup.c | 47 ++++++++++++++++ 6 files changed, 153 insertions(+), 32 deletions(-) diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h index 6b9ddb99f3a8..435c32d8a995 100644 --- a/include/net/netfilter/nf_tables.h +++ b/include/net/netfilter/nf_tables.h @@ -874,6 +874,8 @@ struct nft_chain { struct nft_rule **rules_next; }; +int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain); + enum nft_chain_types { NFT_CHAIN_T_DEFAULT = 0, NFT_CHAIN_T_ROUTE, diff --git a/include/net/netfilter/nf_tables_core.h b/include/net/netfilter/nf_tables_core.h index cd6915b6c054..e0c0c2558ec4 100644 --- a/include/net/netfilter/nf_tables_core.h +++ b/include/net/netfilter/nf_tables_core.h @@ -2,6 +2,8 @@ #ifndef _NET_NF_TABLES_CORE_H #define _NET_NF_TABLES_CORE_H +#include + extern struct nft_expr_type nft_imm_type; extern struct nft_expr_type nft_cmp_type; extern struct nft_expr_type nft_lookup_type; @@ -23,6 +25,12 @@ struct nft_cmp_fast_expr { u8 len; }; +struct nft_immediate_expr { + struct nft_data data; + enum nft_registers dreg:8; + u8 dlen; +}; + /* Calculate the mask for the nft_cmp_fast expression. On big endian the * mask needs to include the *upper* bytes when interpreting that data as * something smaller than the full u32, therefore a cpu_to_le32 is done. diff --git a/include/net/netns/nftables.h b/include/net/netns/nftables.h index 29c3851b486a..188b34d542ff 100644 --- a/include/net/netns/nftables.h +++ b/include/net/netns/nftables.h @@ -9,6 +9,7 @@ struct netns_nftables { struct list_head commit_list; unsigned int base_seq; u8 gencursor; + bool validate; }; #endif diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 3b2ad96a9a05..2880db9049bf 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -1921,19 +1921,7 @@ static int nf_tables_newexpr(const struct nft_ctx *ctx, goto err1; } - if (ops->validate) { - const struct nft_data *data = NULL; - - err = ops->validate(ctx, expr, &data); - if (err < 0) - goto err2; - } - return 0; - -err2: - if (ops->destroy) - ops->destroy(ctx, expr); err1: expr->ops = NULL; return err; @@ -2299,6 +2287,53 @@ static void nf_tables_rule_release(const struct nft_ctx *ctx, nf_tables_rule_destroy(ctx, rule); } +int nft_chain_validate(const struct nft_ctx *ctx, const struct nft_chain *chain) +{ + struct nft_expr *expr, *last; + const struct nft_data *data; + struct nft_rule *rule; + int err; + + list_for_each_entry(rule, &chain->rules, list) { + if (!nft_is_active_next(ctx->net, rule)) + continue; + + nft_rule_for_each_expr(expr, last, rule) { + if (!expr->ops->validate) + continue; + + err = expr->ops->validate(ctx, expr, &data); + if (err < 0) + return err; + } + } + + return 0; +} +EXPORT_SYMBOL_GPL(nft_chain_validate); + +static int nf_tables_validate(struct net *net, const struct nft_table *table) +{ + struct nft_chain *chain; + struct nft_ctx ctx = { + .net = net, + .family = table->family, + }; + int err; + + list_for_each_entry(chain, &table->chains, list) { + if (!nft_is_base_chain(chain)) + continue; + + ctx.chain = chain; + err = nft_chain_validate(&ctx, chain); + if (err < 0) + return err; + } + + return 0; +} + #define NFT_RULE_MAXEXPRS 128 static struct nft_expr_info *info; @@ -2469,8 +2504,11 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk, } } chain->use++; - return 0; + if (net->nft.validate) + return nf_tables_validate(net, table); + + return 0; err2: nf_tables_rule_release(&ctx, rule); err1: @@ -4211,7 +4249,7 @@ static int nf_tables_newsetelem(struct net *net, struct sock *nlsk, const struct nlattr *attr; struct nft_set *set; struct nft_ctx ctx; - int rem, err = 0; + int rem, err; if (nla[NFTA_SET_ELEM_LIST_ELEMENTS] == NULL) return -EINVAL; @@ -4232,9 +4270,13 @@ static int nf_tables_newsetelem(struct net *net, struct sock *nlsk, nla_for_each_nested(attr, nla[NFTA_SET_ELEM_LIST_ELEMENTS], rem) { err = nft_add_set_elem(&ctx, set, attr, nlh->nlmsg_flags); if (err < 0) - break; + return err; } - return err; + + if (net->nft.validate) + return nf_tables_validate(net, ctx.table); + + return 0; } /** @@ -6055,6 +6097,17 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb) struct nft_chain *chain; struct nft_table *table; + WARN_ON_ONCE(net->nft.validate); + + /* 0. Validate ruleset, otherwise roll back for error reporting. */ + list_for_each_entry(table, &net->nft.tables, list) { + if (nf_tables_validate(net, table) < 0) { + net->nft.validate = true; + return -EAGAIN; + } + } + net->nft.validate = false; + /* 1. Allocate space for next generation rules_gen_X[] */ list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) { int ret; @@ -6444,19 +6497,18 @@ static int nf_tables_check_loops(const struct nft_ctx *ctx, list_for_each_entry(rule, &chain->rules, list) { nft_rule_for_each_expr(expr, last, rule) { - const struct nft_data *data = NULL; + struct nft_immediate_expr *priv; + const struct nft_data *data; int err; - if (!expr->ops->validate) + if (strcmp(expr->ops->type->name, "immediate")) continue; - err = expr->ops->validate(ctx, expr, &data); - if (err < 0) - return err; - - if (data == NULL) + priv = nft_expr_priv(expr); + if (priv->dreg != NFT_REG_VERDICT) continue; + data = &priv->data; switch (data->verdict.code) { case NFT_JUMP: case NFT_GOTO: @@ -6936,6 +6988,8 @@ static int __net_init nf_tables_init_net(struct net *net) INIT_LIST_HEAD(&net->nft.tables); INIT_LIST_HEAD(&net->nft.commit_list); net->nft.base_seq = 1; + net->nft.validate = false; + return 0; } diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c index aa87ff8beae8..15adf8ca82c3 100644 --- a/net/netfilter/nft_immediate.c +++ b/net/netfilter/nft_immediate.c @@ -17,12 +17,6 @@ #include #include -struct nft_immediate_expr { - struct nft_data data; - enum nft_registers dreg:8; - u8 dlen; -}; - static void nft_immediate_eval(const struct nft_expr *expr, struct nft_regs *regs, const struct nft_pktinfo *pkt) @@ -101,12 +95,27 @@ static int nft_immediate_dump(struct sk_buff *skb, const struct nft_expr *expr) static int nft_immediate_validate(const struct nft_ctx *ctx, const struct nft_expr *expr, - const struct nft_data **data) + const struct nft_data **d) { const struct nft_immediate_expr *priv = nft_expr_priv(expr); + const struct nft_data *data; + int err; - if (priv->dreg == NFT_REG_VERDICT) - *data = &priv->data; + if (priv->dreg != NFT_REG_VERDICT) + return 0; + + data = &priv->data; + + switch (data->verdict.code) { + case NFT_JUMP: + case NFT_GOTO: + err = nft_chain_validate(ctx, data->verdict.chain); + if (err < 0) + return err; + break; + default: + break; + } return 0; } diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c index f52da5e2199f..42e6fadf1417 100644 --- a/net/netfilter/nft_lookup.c +++ b/net/netfilter/nft_lookup.c @@ -149,6 +149,52 @@ static int nft_lookup_dump(struct sk_buff *skb, const struct nft_expr *expr) return -1; } +static int nft_lookup_validate_setelem(const struct nft_ctx *ctx, + struct nft_set *set, + const struct nft_set_iter *iter, + struct nft_set_elem *elem) +{ + const struct nft_set_ext *ext = nft_set_elem_ext(set, elem->priv); + const struct nft_data *data; + + if (nft_set_ext_exists(ext, NFT_SET_EXT_FLAGS) && + *nft_set_ext_flags(ext) & NFT_SET_ELEM_INTERVAL_END) + return 0; + + data = nft_set_ext_data(ext); + switch (data->verdict.code) { + case NFT_JUMP: + case NFT_GOTO: + return nft_chain_validate(ctx, data->verdict.chain); + default: + return 0; + } +} + +static int nft_lookup_validate(const struct nft_ctx *ctx, + const struct nft_expr *expr, + const struct nft_data **d) +{ + const struct nft_lookup *priv = nft_expr_priv(expr); + struct nft_set_iter iter; + + if (!(priv->set->flags & NFT_SET_MAP) || + priv->set->dtype != NFT_DATA_VERDICT) + return 0; + + iter.genmask = nft_genmask_next(ctx->net); + iter.skip = 0; + iter.count = 0; + iter.err = 0; + iter.fn = nft_lookup_validate_setelem; + + priv->set->ops->walk(ctx, priv->set, &iter); + if (iter.err < 0) + return iter.err; + + return 0; +} + static const struct nft_expr_ops nft_lookup_ops = { .type = &nft_lookup_type, .size = NFT_EXPR_SIZE(sizeof(struct nft_lookup)), @@ -156,6 +202,7 @@ static const struct nft_expr_ops nft_lookup_ops = { .init = nft_lookup_init, .destroy = nft_lookup_destroy, .dump = nft_lookup_dump, + .validate = nft_lookup_validate, }; struct nft_expr_type nft_lookup_type __read_mostly = {