| Message ID | 20261001180224.1018290-1-4ncienth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] netfilter: conntrack: avoid recursive master destruction | expand |
Daehyeon Ko <4ncienth@gmail.com> wrote: > Conntrack entries created through ctnetlink can reference another > confirmed entry as their master. There is no limit on the resulting > chain depth. I don't think this should be allowed. I.e. (totally untested): diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -2359,6 +2359,12 @@ 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; + } + __set_bit(IPS_EXPECTED_BIT, &ct->status); ct->master = master_ct;
Thanks for taking a look.
I think this check is reasonable for CTA_TUPLE_MASTER, but it does not seem
to make master chains impossible by itself. There is another master writer
in init_conntrack(): expectation fulfillment assigns exp->master to the new
conntrack.
In particular, ctnetlink_glue_attach_expect() can attach an expectation
from an NFQUEUE verdict and set assign_helper. The expected child then has
both a master and a helper, so it can serve as the master of another
userspace-helper expectation. That path does not pass through the proposed
check and has no cumulative depth bound.
This helper propagation was intentionally restored by dcb0f9aefdd6
("netfilter: nf_conntrack_expect: restore helper propagation via
expectation") for SIP, H.323 and userspace helpers. Restricting all nested
masters would therefore need a wider helper/expectation compatibility
change.
For that reason I think nf_ct_destroy() still needs to be stack-safe. The
creation-time check could be added separately if nested conntrackd restore
should also be rejected. Does that match your intended invariant?
Thanks,
Daehyeon
On Fri, Oct 02, 2026 at 03:02:24AM +0900, Daehyeon Ko wrote: > Conntrack entries created through ctnetlink can reference another > confirmed entry as their master. There is no limit on the resulting > chain depth. Then, just limit ctnetlink because this makes no sense. > When the last external reference to such a chain is dropped, > nf_ct_destroy() puts the master reference. If that is the master's last > reference, nf_ct_put() invokes nf_ct_destroy() recursively. A sufficiently > long chain therefore exhausts the task stack. > > Release the master reference directly. When it was the final reference, > continue destroying it in the current invocation. This keeps the existing > refcount and lifetime rules while bounding stack use. > > Fixes: 5faa1f4cb5a1 ("[NETFILTER]: nf_conntrack_netlink: add support to related connections") > Cc: stable@vger.kernel.org > Assisted-by: LLM > Signed-off-by: Daehyeon Ko <4ncienth@gmail.com> > --- > Tested on net e23a64eb244356ee47c0620f0722d51bd88db522 and exact > v6.12.105. A source reproducer and userns launcher are available privately > on request and are intentionally omitted from this public posting. > > The trigger needs CONFIG_USER_NS, CONFIG_NET_NS, CONFIG_NF_CONNTRACK and > CONFIG_NF_CT_NETLINK. Host UID 65534 used only namespace-local > CAP_NET_ADMIN. The essential vulnerable trace is: > > BUG: TASK stack guard page was hit at ffffc90001197ff8 > CPU: 1 UID: 65534 PID: 178 Comm: conntrack-maste > nf_ct_destroy+0x1ac/0x5f0 (repeated) > > Fixed current and LTS 6,000-entry runs ended with nf_conntrack_count=0 and > no crash marker. The netdev allyesconfig and allmodconfig W=1 full builds > were not run. > > net/netfilter/nf_conntrack_core.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c > index d0d9e5ea84a09..0ce6141b3dfd7 100644 > --- a/net/netfilter/nf_conntrack_core.c > +++ b/net/netfilter/nf_conntrack_core.c > @@ -592,6 +592,10 @@ static void warn_on_keymap_list_leak(const struct net *net) > void nf_ct_destroy(struct nf_conntrack *nfct) > { > struct nf_conn *ct = (struct nf_conn *)nfct; > + struct nf_conn *master; > + bool destroy_master; > + > +again: > > WARN_ON(refcount_read(&nfct->use) != 0); > > @@ -610,10 +614,17 @@ void nf_ct_destroy(struct nf_conntrack *nfct) > */ > nf_ct_remove_expectations(ct); > > - if (ct->master) > - nf_ct_put(ct->master); > + master = ct->master; > + destroy_master = master && > + refcount_dec_and_test(&master->ct_general.use); > > nf_conntrack_free(ct); > + > + if (destroy_master) { > + ct = master; > + nfct = &ct->ct_general; > + goto again; > + } > } > EXPORT_SYMBOL(nf_ct_destroy); > > -- > 2.55.0
On Thu, Oct 01, 2026 at 09:19:15PM +0200, Florian Westphal wrote: > Daehyeon Ko <4ncienth@gmail.com> wrote: > > Conntrack entries created through ctnetlink can reference another > > confirmed entry as their master. There is no limit on the resulting > > chain depth. > > I don't think this should be allowed. I.e. (totally untested): > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c > --- a/net/netfilter/nf_conntrack_netlink.c > +++ b/net/netfilter/nf_conntrack_netlink.c > @@ -2359,6 +2359,12 @@ 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; > + } Agreed. Thanks. > + > __set_bit(IPS_EXPECTED_BIT, &ct->status); > ct->master = master_ct;
On Fri, Oct 02, 2026 at 02:39:41PM +0900, Daehyeon Ko wrote: > Thanks for taking a look. > > I think this check is reasonable for CTA_TUPLE_MASTER, but it does not seem > to make master chains impossible by itself. There is another master writer > in init_conntrack(): expectation fulfillment assigns exp->master to the new > conntrack. > > In particular, ctnetlink_glue_attach_expect() can attach an expectation > from an NFQUEUE verdict and set assign_helper. The expected child then has > both a master and a helper, so it can serve as the master of another > userspace-helper expectation. That path does not pass through the proposed > check and has no cumulative depth bound. Then, just restrict glue_attach_expect() to reject this too. > This helper propagation was intentionally restored by dcb0f9aefdd6 > ("netfilter: nf_conntrack_expect: restore helper propagation via > expectation") for SIP, H.323 and userspace helpers. Restricting all nested > masters would therefore need a wider helper/expectation compatibility > change. > > For that reason I think nf_ct_destroy() still needs to be stack-safe. The > creation-time check could be added separately if nested conntrackd restore > should also be rejected. Does that match your intended invariant? > > Thanks, > Daehyeon
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index d0d9e5ea84a09..0ce6141b3dfd7 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -592,6 +592,10 @@ static void warn_on_keymap_list_leak(const struct net *net) void nf_ct_destroy(struct nf_conntrack *nfct) { struct nf_conn *ct = (struct nf_conn *)nfct; + struct nf_conn *master; + bool destroy_master; + +again: WARN_ON(refcount_read(&nfct->use) != 0); @@ -610,10 +614,17 @@ void nf_ct_destroy(struct nf_conntrack *nfct) */ nf_ct_remove_expectations(ct); - if (ct->master) - nf_ct_put(ct->master); + master = ct->master; + destroy_master = master && + refcount_dec_and_test(&master->ct_general.use); nf_conntrack_free(ct); + + if (destroy_master) { + ct = master; + nfct = &ct->ct_general; + goto again; + } } EXPORT_SYMBOL(nf_ct_destroy);
Conntrack entries created through ctnetlink can reference another confirmed entry as their master. There is no limit on the resulting chain depth. When the last external reference to such a chain is dropped, nf_ct_destroy() puts the master reference. If that is the master's last reference, nf_ct_put() invokes nf_ct_destroy() recursively. A sufficiently long chain therefore exhausts the task stack. Release the master reference directly. When it was the final reference, continue destroying it in the current invocation. This keeps the existing refcount and lifetime rules while bounding stack use. Fixes: 5faa1f4cb5a1 ("[NETFILTER]: nf_conntrack_netlink: add support to related connections") Cc: stable@vger.kernel.org Assisted-by: LLM Signed-off-by: Daehyeon Ko <4ncienth@gmail.com> --- Tested on net e23a64eb244356ee47c0620f0722d51bd88db522 and exact v6.12.105. A source reproducer and userns launcher are available privately on request and are intentionally omitted from this public posting. The trigger needs CONFIG_USER_NS, CONFIG_NET_NS, CONFIG_NF_CONNTRACK and CONFIG_NF_CT_NETLINK. Host UID 65534 used only namespace-local CAP_NET_ADMIN. The essential vulnerable trace is: BUG: TASK stack guard page was hit at ffffc90001197ff8 CPU: 1 UID: 65534 PID: 178 Comm: conntrack-maste nf_ct_destroy+0x1ac/0x5f0 (repeated) Fixed current and LTS 6,000-entry runs ended with nf_conntrack_count=0 and no crash marker. The netdev allyesconfig and allmodconfig W=1 full builds were not run. net/netfilter/nf_conntrack_core.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)