[PATCH nf] netfilter: nf_conncount: fix leaked ct in error paths

[PATCH nf] netfilter: nf_conncount: fix leaked ct in error paths

[Fernando Fernandez Mancera]

There are some situations where ct might be leaked as error paths are
skipping the refcounted check and return immediately. In order to solve
it, call nf_ct_put() as soon as possible or make sure that the check is
always called.

Fixes: be102eb6a0e7 ("netfilter: nf_conncount: rework API to use sk_buff directly")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
Note: this is actually rebased in top of net.git
---
 net/netfilter/nf_conncount.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_conncount.c b/net/netfilter/nf_conncount.c
index f1be4dd5cf85..09b534fb1b4b 100644
--- a/net/netfilter/nf_conncount.c
+++ b/net/netfilter/nf_conncount.c
@@ -181,6 +181,8 @@ static int __nf_conncount_add(struct net *net,
 			nf_ct_put(ct);
 		return -EEXIST;
 	}
+	if (refcounted)
+		nf_ct_put(ct);
 
 	if ((u32)jiffies == list->last_gc)
 		goto add_new_node;
@@ -197,7 +199,7 @@ static int __nf_conncount_add(struct net *net,
 				if (nf_ct_tuple_equal(&conn->tuple, &tuple) &&
 				    nf_ct_zone_id(&conn->zone, conn->zone.dir) ==
 				    nf_ct_zone_id(zone, zone->dir))
-					goto out_put; /* already exists */
+					return 0; /* already exists */
 			} else {
 				collect++;
 			}
@@ -215,7 +217,7 @@ static int __nf_conncount_add(struct net *net,
 			 * Attempt to avoid a re-add in this case.
 			 */
 			nf_ct_put(found_ct);
-			goto out_put;
+			return 0;
 		} else if (already_closed(found_ct)) {
 			/*
 			 * we do not care about connections which are
@@ -246,9 +248,6 @@ static int __nf_conncount_add(struct net *net,
 	list->count++;
 	list->last_gc = (u32)jiffies;
 
-out_put:
-	if (refcounted)
-		nf_ct_put(ct);
 	return 0;
 }
 
@@ -456,11 +455,10 @@ insert_tree(struct net *net,
 
 		rb_link_node_rcu(&rbconn->node, parent, rbnode);
 		rb_insert_color(&rbconn->node, root);
-
-		if (refcounted)
-			nf_ct_put(ct);
 	}
 out_unlock:
+	if (refcounted)
+		nf_ct_put(ct);
 	spin_unlock_bh(&nf_conncount_locks[hash]);
 	return count;
 }
-- 
2.51.1

Re: [PATCH nf] netfilter: nf_conncount: fix leaked ct in error paths

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> There are some situations where ct might be leaked as error paths are
> skipping the refcounted check and return immediately. In order to solve
> it, call nf_ct_put() as soon as possible or make sure that the check is
> always called.
>
> Fixes: be102eb6a0e7 ("netfilter: nf_conncount: rework API to use sk_buff directly")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
> Note: this is actually rebased in top of net.git
> ---
>  net/netfilter/nf_conncount.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/net/netfilter/nf_conncount.c b/net/netfilter/nf_conncount.c
> index f1be4dd5cf85..09b534fb1b4b 100644
> --- a/net/netfilter/nf_conncount.c
> +++ b/net/netfilter/nf_conncount.c
> @@ -181,6 +181,8 @@ static int __nf_conncount_add(struct net *net,
>  			nf_ct_put(ct);
>  		return -EEXIST;
>  	}
> +	if (refcounted)
> +		nf_ct_put(ct);

I don't think you can put it this early.  Afaics the zone pointer may
point to storage within 'ct', so this has to be deferred or handled
like 'tuple' (copy).