Re: [PATCH net 4/9] netfilter: cttimeout: prevent UAF during module unload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] cttimeout_exit() only drains RCU readers that reach the per-net
  timeout storage through nf_ct_timeout_hook.

--

netfilter: cttimeout: prevent UAF during module unload

This reorders cttimeout_exit() so that nf_ct_timeout_hook is cleared and
synchronize_net() is called before unregister_pernet_subsys() frees the
per-net timeout storage.  That closes the window where nf_ct_set_timeout()
could still reach the freed nfct_timeout_pernet blob through the hook.

> diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
> index 66c2016f6049d..132c02ac7c4ea 100644
> --- a/net/netfilter/nfnetlink_cttimeout.c
> +++ b/net/netfilter/nfnetlink_cttimeout.c
> @@ -652,9 +652,9 @@ static void __exit cttimeout_exit(void)
>  {
>  	nfnetlink_subsys_unregister(&cttimeout_subsys);
>  
> -	unregister_pernet_subsys(&cttimeout_ops);
>  	RCU_INIT_POINTER(nf_ct_timeout_hook, NULL);
>  	synchronize_net();
> +	unregister_pernet_subsys(&cttimeout_ops);
>  }

[Severity: High]
Does this leave the same per-net object reachable from the netlink dump
path during unload?

cttimeout_get_timeout() starts the dump without a module reference:

net/netfilter/nfnetlink_cttimeout.c:cttimeout_get_timeout() {
	if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
		struct netlink_dump_control c = {
			.dump = ctnl_timeout_dump,
		};
		return netlink_dump_start(info->sk, skb, info->nlh, &c);
	}

With no .module set, __netlink_dump_start()'s try_module_get(control->module)
is try_module_get(NULL), which succeeds without taking anything, and
cb->module stays NULL.  nf_tables_api.c and nfnetlink_hook.c pass
.module = THIS_MODULE in their netlink_dump_control for comparison.

ctnl_timeout_dump() reads the per-net blob that unregister_pernet_subsys()
frees, and it is not gated by nf_ct_timeout_hook:

net/netfilter/nfnetlink_cttimeout.c:ctnl_timeout_dump() {
	rcu_read_lock();
	pernet = nfct_timeout_pernet(net);
	list_for_each_entry_rcu(cur, &pernet->nfct_timeout_list, head) {

Is the following sequence possible?

1. IPCTNL_MSG_TIMEOUT_NEW creates one policy; cttimeout_new_timeout() does
   __module_get(THIS_MODULE).
2. A NLM_F_DUMP IPCTNL_MSG_TIMEOUT_GET parks the dump: with one entry
   present ctnl_timeout_dump() returns skb->len > 0, netlink_dump() queues
   the skb and leaves nlk->cb_running set with cb->dump =
   ctnl_timeout_dump.
3. IPCTNL_MSG_TIMEOUT_DELETE removes the policy, and ctnl_timeout_del()
   does module_put(THIS_MODULE), so the module refcount is back to zero
   while the dump is still parked.
4. rmmod runs cttimeout_exit().  The synchronize_rcu() inside
   nfnetlink_subsys_unregister() and the new synchronize_net() only wait
   for RCU read-side sections, and a parked dump is not in one, so
   unregister_pernet_subsys() frees the nfct_timeout_pernet allocation and
   the module text goes away.
5. The next recvmsg() on that netlink socket resumes the dump:

net/netlink/af_netlink.c:netlink_recvmsg() {
	if (READ_ONCE(nlk->cb_running) &&
	    atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2) {
		ret = netlink_dump(sk, false);

which is an indirect call through cb->dump into freed module text, and
then a read of the freed pernet->nfct_timeout_list.

Should the dump control also carry .module = THIS_MODULE so the parked
dump pins the module across the unload, given the commit is aimed at the
same freed nfct_timeout_pernet object during module unload?

Note that a dump over an empty policy list does not park: ctnl_timeout_dump()
returns 0 there and netlink_dump() finishes the dump immediately, so the
create/park/delete order above is what makes it reproducible.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907171732.1407739-2-pablo%40netfilter.org