diff mbox series

[2/3,nf-next] netfilter: fix error path of the nf_tables_newrule

Message ID 20180429145652.12936-1-ap420073@gmail.com
State Superseded
Delegated to: Pablo Neira
Headers show
Series fix module leak and use-after-free | expand

Commit Message

Taehee Yoo April 29, 2018, 2:56 p.m. UTC 
There is module leak in the error path of the nf_tables_newrule.
In order to solve this, a member nft_expr_type *type is added into
the nft_expr_info. so that, we can make separated error path of the
nft_expr_ops and the nft_expr_type.
So that, the nf_tables_rule_destroy is not used in the error path
of the nf_tables_newrule anymore.

Steps to reproduce:
   $iptables-compat -I OUTPUT -m cpu --cpu 0
   $iptables-compat -F
   $lsmod

   Module                  Size  Used by
   xt_cpu                 16384  1


Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 net/netfilter/nf_tables_api.c | 46 +++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 15 deletions(-)

Comments

Florian Westphal April 29, 2018, 6:30 p.m. UTC | #1 
Taehee Yoo <ap420073@gmail.com> wrote:
> There is module leak in the error path of the nf_tables_newrule.
> In order to solve this, a member nft_expr_type *type is added into
> the nft_expr_info. so that, we can make separated error path of the
> nft_expr_ops and the nft_expr_type.
> So that, the nf_tables_rule_destroy is not used in the error path
> of the nf_tables_newrule anymore.
> 
> Steps to reproduce:
>    $iptables-compat -I OUTPUT -m cpu --cpu 0
>    $iptables-compat -F
>    $lsmod
>    Module                  Size  Used by
>    xt_cpu                 16384  1

Can not reproduce this here:
$ lsmod|grep xt_cpu
$ nft list ruleset
table ip filter {
 [..]
        chain OUTPUT {
                type filter hook output priority 0; policy accept;
                cpu 0 counter packets 27 bytes 3436
}
$ lsmod|grep xt_cpu
xt_cpu                 16384  1
$ nft flush ruleset
$ lsmod|grep xt_cpu
xt_cpu                 16384  0

Linux 4.16.5.

>  	if (expr->ops->destroy)
>  		expr->ops->destroy(ctx, expr);
> -	module_put(expr->ops->type->owner);
> +	if (expr->ops->type->release)
> +		expr->ops->type->release(expr->ops);
> +
> +	module_put(module);

The above indeed looks buggy on 4.16.5 (original, not you patch).

->ops can be free'd already in the destroy path with nft_match_destroy,
So I agree, yes, I think we need to at least fetch module pointer first.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 9134cc4..981f35e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1814,6 +1814,7 @@  int nft_expr_dump(struct sk_buff *skb, unsigned int attr,
 
 struct nft_expr_info {
 	const struct nft_expr_ops	*ops;
+	const struct nft_expr_type	*type;
 	struct nlattr			*tb[NFT_EXPR_MAXATTR + 1];
 };
 
@@ -1853,6 +1854,7 @@  static int nf_tables_expr_parse(const struct nft_ctx *ctx,
 		ops = type->ops;
 
 	info->ops = ops;
+	info->type = type;
 	return 0;
 
 err1:
@@ -1895,9 +1897,14 @@  static int nf_tables_newexpr(const struct nft_ctx *ctx,
 static void nf_tables_expr_destroy(const struct nft_ctx *ctx,
 				   struct nft_expr *expr)
 {
+	struct module *module = expr->ops->type->owner;
+
 	if (expr->ops->destroy)
 		expr->ops->destroy(ctx, expr);
-	module_put(expr->ops->type->owner);
+	if (expr->ops->type->release)
+		expr->ops->type->release(expr->ops);
+
+	module_put(module);
 }
 
 struct nft_expr *nft_expr_init(const struct nft_ctx *ctx,
@@ -1922,9 +1929,11 @@  struct nft_expr *nft_expr_init(const struct nft_ctx *ctx,
 
 	return expr;
 err3:
+	if (info.type->release)
+		info.type->release(info.ops);
 	kfree(expr);
 err2:
-	module_put(info.ops->type->owner);
+	module_put(info.type->owner);
 err1:
 	return ERR_PTR(err);
 }
@@ -2258,7 +2267,7 @@  static int nf_tables_newrule(struct net *net, struct sock *nlsk,
 	struct nft_expr *expr;
 	struct nft_ctx ctx;
 	struct nlattr *tmp;
-	unsigned int size, i, n, ulen = 0, usize = 0;
+	unsigned int size, i, n_type, n_ops, ulen = 0, usize = 0;
 	int err, rem;
 	bool create;
 	u64 handle, pos_handle;
@@ -2307,20 +2316,20 @@  static int nf_tables_newrule(struct net *net, struct sock *nlsk,
 
 	nft_ctx_init(&ctx, net, skb, nlh, family, table, chain, nla);
 
-	n = 0;
+	n_type = 0;
 	size = 0;
 	if (nla[NFTA_RULE_EXPRESSIONS]) {
 		nla_for_each_nested(tmp, nla[NFTA_RULE_EXPRESSIONS], rem) {
 			err = -EINVAL;
 			if (nla_type(tmp) != NFTA_LIST_ELEM)
 				goto err1;
-			if (n == NFT_RULE_MAXEXPRS)
+			if (n_type == NFT_RULE_MAXEXPRS)
 				goto err1;
-			err = nf_tables_expr_parse(&ctx, tmp, &info[n]);
+			err = nf_tables_expr_parse(&ctx, tmp, &info[n_type]);
 			if (err < 0)
 				goto err1;
-			size += info[n].ops->size;
-			n++;
+			size += info[n_type].ops->size;
+			n_type++;
 		}
 	}
 	/* Check for overflow of dlen field */
@@ -2352,11 +2361,10 @@  static int nf_tables_newrule(struct net *net, struct sock *nlsk,
 	}
 
 	expr = nft_expr_first(rule);
-	for (i = 0; i < n; i++) {
-		err = nf_tables_newexpr(&ctx, &info[i], expr);
+	for (n_ops = 0; n_ops < n_type; n_ops++) {
+		err = nf_tables_newexpr(&ctx, &info[n_ops], expr);
 		if (err < 0)
 			goto err2;
-		info[i].ops = NULL;
 		expr = nft_expr_next(expr);
 	}
 
@@ -2397,11 +2405,19 @@  static int nf_tables_newrule(struct net *net, struct sock *nlsk,
 err3:
 	list_del_rcu(&rule->list);
 err2:
-	nf_tables_rule_destroy(&ctx, rule);
+	expr = nft_expr_first(rule);
+	for (i = 0; i < n_ops; i++) {
+		if (expr->ops && expr->ops->destroy)
+			expr->ops->destroy(&ctx, expr);
+		expr = nft_expr_next(expr);
+	}
+	kfree(rule);
+
 err1:
-	for (i = 0; i < n; i++) {
-		if (info[i].ops != NULL)
-			module_put(info[i].ops->type->owner);
+	for (i = 0; i < n_type; i++) {
+		if (info[i].type->release)
+			info[i].type->release(info[i].ops);
+		module_put(info[i].type->owner);
 	}
 	return err;
 }