[PATCH nf-next] netfilter: conntrack: prevent nf_conntrack_buckets race condition

[PATCH nf-next] netfilter: conntrack: prevent nf_conntrack_buckets race condition

[Fernando Fernandez Mancera]

There is a race condition in the nf_conntrack_buckets sysctl handler.
The implementation uses a single global variable as a temporary buffer
to parse user input. Because sysctl handlers are not locked, a
concurrent read or write can overwrite this shared variable after a
writer has parsed its input but before the actual resize occurs.

While this sysctl is only writable from the initial network namespace,
the race can trigger if a monitoring daemon reads the sysctl while an
administrator or orchestrator is writing to it.

Fix this by removing the unnecessary global state by using a local-stack
table instead and delegating the locking/serialization to
nf_conntrack_hash_resize() function.

Fixes: 3183ab8997a4 ("netfilter: conntrack: allow increasing bucket size via sysctl too")
Reported-by: VEGA <vega@nebusec.ai>
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
Note: this isn't relevant enough to land in nf tree in my opinion, let's
aim for nf-next so we avoid a false sense of urgency
---
 net/netfilter/nf_conntrack_standalone.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index f4f2d82192d5..4b692a184e94 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -533,27 +533,22 @@ EXPORT_SYMBOL_GPL(nf_conntrack_count);
 /* Sysctl support */
 
 #ifdef CONFIG_SYSCTL
-/* size the user *wants to set */
-static unsigned int nf_conntrack_htable_size_user __read_mostly;
-
 static int
 nf_conntrack_hash_sysctl(const struct ctl_table *table, int write,
 			 void *buffer, size_t *lenp, loff_t *ppos)
 {
+	unsigned int htable_size = READ_ONCE(nf_conntrack_htable_size);
+	struct ctl_table tmp = *table;
 	int ret;
 
-	/* module_param hashsize could have changed value */
-	nf_conntrack_htable_size_user = nf_conntrack_htable_size;
+	tmp.data = &htable_size;
 
-	ret = proc_dointvec(table, write, buffer, lenp, ppos);
+	ret = proc_dointvec(&tmp, write, buffer, lenp, ppos);
 	if (ret < 0 || !write)
 		return ret;
 
 	/* update ret, we might not be able to satisfy request */
-	ret = nf_conntrack_hash_resize(nf_conntrack_htable_size_user);
-
-	/* update it to the actual value used by conntrack */
-	nf_conntrack_htable_size_user = nf_conntrack_htable_size;
+	ret = nf_conntrack_hash_resize(htable_size);
 	return ret;
 }
 
@@ -657,7 +652,6 @@ static const struct ctl_table nf_ct_sysctl_table[] = {
 	},
 	[NF_SYSCTL_CT_BUCKETS] = {
 		.procname       = "nf_conntrack_buckets",
-		.data           = &nf_conntrack_htable_size_user,
 		.maxlen         = sizeof(unsigned int),
 		.mode           = 0644,
 		.proc_handler   = nf_conntrack_hash_sysctl,
@@ -1153,8 +1147,6 @@ static int __init nf_conntrack_standalone_init(void)
 		ret = -ENOMEM;
 		goto out_sysctl;
 	}
-
-	nf_conntrack_htable_size_user = nf_conntrack_htable_size;
 #endif
 
 	nf_conntrack_init_end();
-- 
2.55.0