[PATCH nf] netfilter: nf_tables: fix memory leak when more than 255 elements expired

[PATCH nf] netfilter: nf_tables: fix memory leak when more than 255 elements expired

[Florian Westphal]

When more than 255 elements expire we're supposed to switch to a new gc
container structure, but nft_trans_gc_space() always returns false in this
case because u8 type is too narrow for the boundary.

While at it, don't deref 'gc' after we've passed it to call_rcu.

Fixes: 5f68718b34a5 ("netfilter: nf_tables: GC transaction API to avoid race with control plane")
Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_tables.h |  2 +-
 net/netfilter/nf_tables_api.c     | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index dd40c75011d2..a338d7d4f69b 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1682,7 +1682,7 @@ struct nft_trans_gc {
 	struct net		*net;
 	struct nft_set		*set;
 	u32			seq;
-	u8			count;
+	u16			count;
 	void			*priv[NFT_TRANS_GC_BATCHCOUNT];
 	struct rcu_head		rcu;
 };
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e429ebba74b3..96d40d62dd5a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9562,12 +9562,15 @@ static int nft_trans_gc_space(struct nft_trans_gc *trans)
 struct nft_trans_gc *nft_trans_gc_queue_async(struct nft_trans_gc *gc,
 					      unsigned int gc_seq, gfp_t gfp)
 {
+	struct nft_set *set;
+
 	if (nft_trans_gc_space(gc))
 		return gc;
 
+	set = gc->set;
 	nft_trans_gc_queue_work(gc);
 
-	return nft_trans_gc_alloc(gc->set, gc_seq, gfp);
+	return nft_trans_gc_alloc(set, gc_seq, gfp);
 }
 
 void nft_trans_gc_queue_async_done(struct nft_trans_gc *trans)
@@ -9582,15 +9585,18 @@ void nft_trans_gc_queue_async_done(struct nft_trans_gc *trans)
 
 struct nft_trans_gc *nft_trans_gc_queue_sync(struct nft_trans_gc *gc, gfp_t gfp)
 {
+	struct nft_set *set;
+
 	if (WARN_ON_ONCE(!lockdep_commit_lock_is_held(gc->net)))
 		return NULL;
 
 	if (nft_trans_gc_space(gc))
 		return gc;
 
+	set = gc->set;
 	call_rcu(&gc->rcu, nft_trans_gc_trans_free);
 
-	return nft_trans_gc_alloc(gc->set, 0, gfp);
+	return nft_trans_gc_alloc(set, 0, gfp);
 }
 
 void nft_trans_gc_queue_sync_done(struct nft_trans_gc *trans)
-- 
2.41.0

Re: [PATCH nf] netfilter: nf_tables: fix memory leak when more than 255 elements expired

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> When more than 255 elements expire we're supposed to switch to a new gc
> container structure, but nft_trans_gc_space() always returns false in this

Grrr.  This should read 'always returns true' or 'never returns false',
but not *THIS*.  I'll fix this up when applying this, probably tomorrow
morning.

Re: [PATCH nf] netfilter: nf_tables: fix memory leak when more than 255 elements expired

[Pablo Neira Ayuso]

On Tue, Sep 19, 2023 at 03:59:38PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > When more than 255 elements expire we're supposed to switch to a new gc
> > container structure, but nft_trans_gc_space() always returns false in this
> 
> Grrr.  This should read 'always returns true' or 'never returns false',
> but not *THIS*.  I'll fix this up when applying this, probably tomorrow
> morning.

You could amend the fix when applying this.

Re: [PATCH nf] netfilter: nf_tables: fix memory leak when more than 255 elements expired

[Pablo Neira Ayuso]

On Tue, Sep 19, 2023 at 04:12:34PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Sep 19, 2023 at 03:59:38PM +0200, Florian Westphal wrote:
> > Florian Westphal <fw@strlen.de> wrote:
> > > When more than 255 elements expire we're supposed to switch to a new gc
> > > container structure, but nft_trans_gc_space() always returns false in this
> > 
> > Grrr.  This should read 'always returns true' or 'never returns false',
> > but not *THIS*.  I'll fix this up when applying this, probably tomorrow
> > morning.
> 
> You could amend the fix when applying this.

I mean, this patch description, as you prefer.