Re: [PATCH v2 net] netfilter: conntrack: reject nested ctnetlink master chains

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix does not stop userspace from building an unbounded
  ct->master chain.
- [High] The commit keeps "kernel helper expectations unchanged", but the
  H.323 Q.931 helper nests itself without limit.
- [Medium] The new check in ctnetlink_create_conntrack()
  (nf_conntrack_netlink.c:2362) makes IPCTNL_MSG_CT_NEW fail with…

--

netfilter: conntrack: reject nested ctnetlink master chains

The patch rejects ctnetlink conntrack creation when the CTA_TUPLE_MASTER
conntrack already has a master. It also rejects NFQUEUE-attached
expectations (NFQA_EXP) for conntracks that have a master. The aim is to
stop userspace from building unbounded master chains, whose recursive
release in nf_ct_destroy() exhausts the kernel stack.

> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 4e5d7c7014368..c68e79d1ac87b 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2359,6 +2359,11 @@ ctnetlink_create_conntrack(struct net *net,
>  			goto err2;
>  		}
>  		master_ct = nf_ct_tuplehash_to_ctrack(master_h);
> +		if (master_ct->master) {
> +			nf_ct_put(master_ct);
> +			err = -EOPNOTSUPP;
> +			goto err2;
> +		}

[Severity: Medium]
Does this check reject state that the kernel itself creates? In H.323, a
RAS expectation creates the Q.931 conntrack. That Q.931 conntrack then
sets up the H.245 expectation:

net/netfilter/nf_conntrack_h323_main.c:expect_h245() {
    ...
	rcu_assign_pointer(exp->assign_helper, nf_conntrack_helper_h245_ptr);
    ...
}

So the resulting H.245 and RTP conntracks have a master that itself has
a master.

A state-sync daemon such as conntrackd may recreate those flows with
IPCTNL_MSG_CT_NEW and a CTA_TUPLE_MASTER that points at the related Q.931
conntrack. That request used to be accepted and now gets -EOPNOTSUPP, so
these flows would not be replicated after a failover.

Since this is tagged for stable, could the commit message mention this
change in what CT_NEW accepts?

>  		__set_bit(IPS_EXPECTED_BIT, &ct->status);
>  		ct->master = master_ct;
>  	}
> @@ -2864,6 +2869,9 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
>  	struct nf_conntrack_expect *exp;
>  	int err;
>  
> +	if (ct->master)
> +		return -EOPNOTSUPP;
> +

[Severity: High]
Can nf_tables still reach the same stack exhaustion? The commit message
says "Stop the repeatable userspace paths", but the patch only covers
CTA_TUPLE_MASTER and NFQA_EXP.

The first path uses an nft ct expectation object alone.
nft_ct_expect_obj_eval() only skips conntracks that are confirmed,
templates, or that already have a help extension:

net/netfilter/nft_ct.c:nft_ct_expect_obj_eval() {
    ...
	ct = nf_ct_get(pkt->skb, &ctinfo);
	if (!ct || nf_ct_is_confirmed(ct) || nf_ct_is_template(ct)) {
		regs->verdict.code = NFT_BREAK;
		return;
	}

	help = nfct_help(ct);
	if (help) {
    ...
}

A RELATED child created from an expectation with a NULL assign_helper has
no help extension, so it also gets the ct_expect_help helper.

ct_expect_help() then calls nf_ct_expect_related() with the child as
exp->master, and it never checks ct->master. Over loopback, each new flow
from a fresh source port consumes the previous expectation and arms the
next one, so every flow adds a level.

The second path combines an nft ct helper with ctnetlink.
IPCTNL_MSG_EXP_NEW only stops the chain because ctnetlink_alloc_expect()
requires help->helper.

nft_ct_helper_obj_eval() assigns a helper to any unconfirmed conntrack,
related ones included. After that, ctnetlink_create_expect() succeeds
with the child as master.

A kernel helper attached this way, such as FTP, could also extend the
chain from traffic alone.

In every case init_conntrack() sets ct->master = exp->master with no
depth limit. Freeing the leaf then recurses once per level:

nf_ct_put()
  nf_ct_destroy()
    nf_ct_put(ct->master)
      nf_ct_destroy()
        ...

Both nft paths need only CAP_NET_ADMIN in a user/net namespace, the same
privilege ctnetlink needs.

Would a central bound in nf_ct_expect_related() or init_conntrack() be
needed to close this, or an iterative release of the master chain in
nf_ct_destroy()? Gating each entry point separately leaves these paths
open.

[Severity: High]
The commit message says "Keep regular ctnetlink and kernel helper
expectations unchanged". Can the H.323 Q.931 helper build an unbounded
chain on its own?

For a Facility message with reason callForwarded, process_facility()
calls expect_callforwarding(). That function gives the expected child the
Q.931 helper again:

net/netfilter/nf_conntrack_h323_main.c:expect_callforwarding() {
    ...
	rcu_assign_pointer(exp->assign_helper, nf_conntrack_helper_q931_ptr[0]);
    ...
}

Neither function checks IPS_EXPECTED or ct->master. Each new expected
Q.931 connection can therefore send another Facility(callForwarded) and
create the next level.

The default callforward_filter does not seem to prevent this.
callforward_do_filter() only filters when the nexthop and device are the
same, and two distinct local addresses have different nexthops.

This works over loopback from an unprivileged user namespace once the
helper is attached. It also works for remote peers wherever the Q.931
helper is enabled. Either way, the same recursion in nf_ct_destroy()
exhausts the stack.

SIP REGISTER does not seem to have this problem, because
process_register_request() returns early when IPS_EXPECTED is set.

>  	err = nla_parse_nested_deprecated(cda, CTA_EXPECT_MAX, attr,
>  					  exp_nla_policy, NULL);
>  	if (err < 0)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002165601.1754467-1-4ncienth%40gmail.com