[PATCH net 0/6] ovs, net/sched: fixes for UAF after conntrack extension realloc

[PATCH net 0/6] ovs, net/sched: fixes for UAF after conntrack extension realloc

[Ilya Maximets]

One clean up change plus two fixes for the UAF on helper extension
realloc x 2.  First half for OVS and the second half for the similar
code in act_ct.  This should cover all the known cases of this problem
in these two modules.


Ilya Maximets (6):
  net: openvswitch: conntrack: avoid modifying shared unconfirmed ct
    entry
  net: openvswitch: conntrack: remove 'add_helper' dead code
  net: openvswitch: conntrack: fix helper UAF due to extensions realloc
  net/sched: act_ct: avoid modifying shared unconfirmed ct entry
  net/sched: act_ct: remove 'add_helper' dead code
  net/sched: act_ct: fix helper UAF due to extensions realloc

 include/net/netfilter/nf_conntrack.h |  5 ++++
 net/openvswitch/conntrack.c          | 37 +++++++++++++++++++-------
 net/sched/act_ct.c                   | 39 +++++++++++++++++++++-------
 3 files changed, 61 insertions(+), 20 deletions(-)

-- 
2.55.0

[PATCH net 1/6] net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry

[Ilya Maximets]

In a case where skb with an unconfirmed ct entry gets cloned, we may
end up committing both but with different sets of extensions.

The series of events:

 1. The first clone wants to commit and runs the helpers wiring up
    the extension pointer into the expectation list.
 2. Then it looses the confirmation keeping the entry unconfirmed.
 3. Second clone now wants to commit labels and adds the new extension
    for that breaking the pointer in the expectation list causing
    UAF on the destruction path later.

While this is possible to trigger, there should be no practical
network pipeline where committing both clones without modifications
into the same zone is needed.  So, let's just reset the entry in case
for some reason we got an skb with a shared one during commit.  This
doesn't affect any known use cases, but avoids any potential problems
with sharing and modification of the unconfirmed ct entry.

The fixes tag points to the introduction of helpers, since that's the
main UAF trigger for the sharing.

Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
Cc: stable@vger.kernel.org
Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 include/net/netfilter/nf_conntrack.h |  5 +++++
 net/openvswitch/conntrack.c          | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index bc42dd0e10e65..c39425e54d87d 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -185,6 +185,11 @@ static inline void nf_ct_put(struct nf_conn *ct)
 		nf_ct_destroy(&ct->ct_general);
 }
 
+static inline bool nf_ct_shared(const struct nf_conn *ct)
+{
+	return refcount_read(&ct->ct_general.use) > 1;
+}
+
 /* load module; enable/disable conntrack in this namespace */
 int nf_ct_netns_get(struct net *net, u8 nfproto);
 void nf_ct_netns_put(struct net *net, u8 nfproto);
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 0f433688e17b9..a733029c28dd0 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -734,6 +734,18 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct;
 
+	/* If the ct entry is not confirmed and shared with some other skb,
+	 * e.g., a cloned one, we can't just modify it with the commit as we
+	 * must not modify the extension set.  Reset.
+	 */
+	if (cached && info->commit) {
+		ct = nf_ct_get(skb, &ctinfo);
+		if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
+			nf_reset_ct(skb);
+			cached = false;
+		}
+	}
+
 	if (!cached) {
 		struct nf_hook_state state = {
 			.hook = NF_INET_PRE_ROUTING,
-- 
2.55.0

[PATCH net 2/6] net: openvswitch: conntrack: remove 'add_helper' dead code

[Ilya Maximets]

This variable can only become 'true' when the connection is not
confirmed, but it is only checked when it is confirmed.  So, it can be
treated as being always false and just removed.

Fixes: 3c1860543fcc ("openvswitch: add nf_ct_is_confirmed check before assigning the helper")
Cc: stable@vger.kernel.org
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/openvswitch/conntrack.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index a733029c28dd0..c20f096eef40e 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -778,8 +778,6 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (ct) {
-		bool add_helper = false;
-
 		/* Packets starting a new connection must be NATted before the
 		 * helper, so that the helper knows about the NAT.  We enforce
 		 * this by delaying both NAT and helper calls for unconfirmed
@@ -811,7 +809,6 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 							    GFP_ATOMIC);
 			if (err)
 				return err;
-			add_helper = true;
 
 			/* helper installed, add seqadj if NAT is required */
 			if (info->nat && !nfct_seqadj(ct)) {
@@ -821,13 +818,10 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 		}
 
 		/* Call the helper only if:
-		 * - nf_conntrack_in() was executed above ("!cached") or a
-		 *   helper was just attached ("add_helper") for a confirmed
-		 *   connection, or
+		 * - nf_conntrack_in() was executed above ("!cached"), or
 		 * - When committing an unconfirmed connection.
 		 */
-		if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
-					      info->commit)) {
+		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {
 			int err = nf_ct_helper(skb, ct, ctinfo, info->family);
 
 			err = verdict_to_errno(err);
-- 
2.55.0

[PATCH net 3/6] net: openvswitch: conntrack: fix helper UAF due to extensions realloc

[Ilya Maximets]

While calling the helpers, a raw pointer to the extensions area is
wired into expectations list:

  -> nf_ct_helper()
   -> helper->help()
    -> nf_ct_expect_related_report()
     -> nf_ct_expect_insert()
      -> hlist_add_head_rcu(&exp->lnode, &master_help->expectations)

In case the connection is not confirmed yet, more extensions can be
added afterwards with *_ext_add() calls reallocating the extension
space and leaving the now invalid pointer in the expectations list
that is later accessed while removing the expectation.

Make sure that helpers are called at the end after all the other
extensions are already added.

Note that the helper rejection now leaves the mark and labels set,
but that's not different from how the NAT was handled before or how
the mark and the labels were handled on confirmation failure.  And
there are no atomicity guarantees provided by the API anyway.

Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
Cc: stable@vger.kernel.org
Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/openvswitch/conntrack.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index c20f096eef40e..d3326edcabf76 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -817,11 +817,14 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 			}
 		}
 
-		/* Call the helper only if:
-		 * - nf_conntrack_in() was executed above ("!cached"), or
-		 * - When committing an unconfirmed connection.
+		/* Call the helper only if nf_conntrack_in() was executed
+		 * above ("!cached").
+		 *
+		 * For unconfirmed connections it will be called later during
+		 * commit as we need to have all the other extensions allocated
+		 * before the call.
 		 */
-		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {
+		if (nf_ct_is_confirmed(ct) && !cached) {
 			int err = nf_ct_helper(skb, ct, ctinfo, info->family);
 
 			err = verdict_to_errno(err);
@@ -1025,6 +1028,14 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
 			return err;
 
 		nf_conn_act_ct_ext_add(skb, ct, ctinfo);
+
+		/* Call the helpers now.  We couldn't do this before as
+		 * all the extensions must be allocated before the call.
+		 */
+		err = nf_ct_helper(skb, ct, ctinfo, info->family);
+		err = verdict_to_errno(err);
+		if (err)
+			return err;
 	} else if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
 		   labels_nonzero(&info->labels.mask)) {
 		err = ovs_ct_set_labels(ct, key, &info->labels.value,
-- 
2.55.0

[PATCH net 4/6] net/sched: act_ct: avoid modifying shared unconfirmed ct entry

[Ilya Maximets]

In a case where skb with an unconfirmed ct entry gets cloned, we may
end up processing both again but with different sets of extensions.

The series of events:

 1. The first clone wants to commit and runs the helpers wiring up
    the extension pointer into the expectation list.
 2. Then it looses the confirmation keeping the entry unconfirmed.
 3. Second clone now wants to commit labels or run NAT and adds the
    new extension for that breaking the pointer in the expectation
    list causing UAF on the destruction path later.

While this is possible to trigger, there should be no practical
network pipeline where we need to process both clones without
modifications in the same zone.  So, let's just reset the entry in
case for some reason we got an skb with a shared one.  This doesn't
affect any known use cases, but avoids any potential problems with
sharing and modification of the unconfirmed ct entry.

Unlike openvswitch module, act_ct allows for NAT without commit.
Changing that would be a uAPI break.  So, act_ct needs to reset on NAT
regardless of the commit flag to avoid reallocation of the extension
space.  This, however, doesn't really change the picture for sensible
networking cases as there should be no need to run the same packet
twice (before and after the clone) through conntrack without packet
header or zone changes and without commit.

The fixes tag points to the introduction of helpers, since that's the
main UAF trigger for the sharing.

Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
Cc: stable@vger.kernel.org
Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/sched/act_ct.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 55f3521edb4c9..e72143d36b119 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -979,11 +979,11 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 				 struct tcf_result *res)
 {
 	struct net *net = dev_net(skb->dev);
+	bool cached, commit, clear, nat;
 	enum ip_conntrack_info ctinfo;
 	struct tcf_ct *c = to_ct(a);
 	struct nf_conn *tmpl = NULL;
 	struct nf_hook_state state;
-	bool cached, commit, clear;
 	int nh_ofs, err, retval;
 	struct tcf_ct_params *p;
 	bool add_helper = false;
@@ -998,6 +998,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 	retval = p->action;
 	commit = p->ct_action & TCA_CT_ACT_COMMIT;
 	clear = p->ct_action & TCA_CT_ACT_CLEAR;
+	nat = p->ct_action & TCA_CT_ACT_NAT;
 	tmpl = p->tmpl;
 
 	tcf_lastuse_update(&c->tcf_tm);
@@ -1046,6 +1047,19 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 	 * different zone.
 	 */
 	cached = tcf_ct_skb_nfct_cached(net, skb, p);
+
+	/* If the ct entry is not confirmed and shared with some other skb,
+	 * e.g., a cloned one, we can't just modify it with a commit or nat
+	 * as we must not modify the extension set.  Reset.
+	 */
+	if (cached && (commit || nat)) {
+		ct = nf_ct_get(skb, &ctinfo);
+		if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
+			nf_reset_ct(skb);
+			cached = false;
+		}
+	}
+
 	if (!cached) {
 		if (tcf_ct_flow_table_lookup(p, skb, family)) {
 			skip_add = true;
@@ -1083,7 +1097,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 		if (err)
 			goto drop;
 		add_helper = true;
-		if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
+		if (nat && !nfct_seqadj(ct)) {
 			if (!nfct_seqadj_ext_add(ct))
 				goto drop;
 		}
-- 
2.55.0

[PATCH net 5/6] net/sched: act_ct: remove 'add_helper' dead code

[Ilya Maximets]

This variable can only become 'true' when the connection is not
confirmed, but it is only checked when it is confirmed.  So, it can be
treated as being always false and just removed.

Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
Cc: stable@vger.kernel.org
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/sched/act_ct.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index e72143d36b119..f62051ec9d57d 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -986,7 +986,6 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 	struct nf_hook_state state;
 	int nh_ofs, err, retval;
 	struct tcf_ct_params *p;
-	bool add_helper = false;
 	bool skb_is_ours = false;
 	bool skip_add = false;
 	bool defrag = false;
@@ -1096,14 +1095,14 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 		err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
 		if (err)
 			goto drop;
-		add_helper = true;
+
 		if (nat && !nfct_seqadj(ct)) {
 			if (!nfct_seqadj_ext_add(ct))
 				goto drop;
 		}
 	}
 
-	if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
+	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
 		err = nf_ct_helper(skb, ct, ctinfo, family);
 		if (err != NF_ACCEPT)
 			goto nf_error;
-- 
2.55.0

[PATCH net 6/6] net/sched: act_ct: fix helper UAF due to extensions realloc

[Ilya Maximets]

While calling the helpers, a raw pointer to the extensions area is
wired into expectations list:

  -> nf_ct_helper()
   -> helper->help()
    -> nf_ct_expect_related_report()
     -> nf_ct_expect_insert()
      -> hlist_add_head_rcu(&exp->lnode, &master_help->expectations)

In case the connection is not confirmed yet, more extensions can be
added afterwards with *_ext_add() calls reallocating the extension
space and leaving the now invalid pointer in the expectations list
that is later accessed while removing the expectation.

Make sure that helpers are called at the end after all the other
extensions are already added.

Note that the helper rejection now leaves the mark and labels set,
but that's not different from how the NAT was handled before or how
the mark and the labels were handled on confirmation failure.  And
there are no atomicity guarantees provided by the API anyway.

Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
Cc: stable@vger.kernel.org
Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/sched/act_ct.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index f62051ec9d57d..411e3dd92d072 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1102,19 +1102,25 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
 		}
 	}
 
-	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
-		err = nf_ct_helper(skb, ct, ctinfo, family);
-		if (err != NF_ACCEPT)
-			goto nf_error;
-	}
-
 	if (commit) {
 		tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
 		tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
 
 		if (!nf_ct_is_confirmed(ct))
 			nf_conn_act_ct_ext_add(skb, ct, ctinfo);
+	}
 
+	/* Run helpers for the connection if nf_conntrack_in() was executed
+	 * or if we're about to commit.  This has to be done after all the
+	 * extensions are already added.
+	 */
+	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
+		err = nf_ct_helper(skb, ct, ctinfo, family);
+		if (err != NF_ACCEPT)
+			goto nf_error;
+	}
+
+	if (commit) {
 		/* This will take care of sending queued events
 		 * even if the connection is already confirmed.
 		 */
-- 
2.55.0

Re: [PATCH net 1/6] net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry

[netdev-bot+sashiko]

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

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

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).

- [Medium] New behaviour change introduced by the guard in
  __ovs_ct_lookup() (net/openvswitch/conntrack.c:741-747): when two skbs…
- [Low] nf_ct_shared() is added as an undocumented generic predicate in
  include/net/netfilter/nf_conntrack.h:188-191 returning…

Pre-existing issues:
- [Low] Peer call site net/sched/act_ct.c (tcf_ct_act()) performs the
  same 'cached, unconfirmed ct + commit modifies the extension set'…

--

net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry

This adds a new nf_ct_shared() helper to the conntrack header and uses it in
__ovs_ct_lookup() to reset the cached conntrack entry when an skb arrives at
a committing ct action with an unconfirmed entry that is shared with another
skb.  The goal is to avoid two clones committing the same unconfirmed entry
with different extension sets, which can lead to a use-after-free.

A few questions below.

> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index bc42dd0e10e65..c39425e54d87d 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -185,6 +185,11 @@ static inline void nf_ct_put(struct nf_conn *ct)
>  		nf_ct_destroy(&ct->ct_general);
>  }
>  
> +static inline bool nf_ct_shared(const struct nf_conn *ct)
> +{
> +	return refcount_read(&ct->ct_general.use) > 1;
> +}
> +

[Severity: Low]
Would it be worth documenting that this predicate is only meaningful for
unconfirmed, non-template entries?

The reference accounting comment on struct nf_conn in this same header says
the count is "1 for hash table, 1 per skb, plus 1 for any connection(s) we
are `master' for".  With that, every confirmed entry owned by a single skb
already has use == 2, since __nf_conntrack_confirm() does:

	/* The caller holds a reference to this object */
	refcount_set(&ct->ct_general.use, 2);

The same is true for a template in use: nf_ct_tmpl_alloc() sets use = 1 and
nf_ct_set(skb, tmpl, ...) adds another.  So nf_ct_shared() returns true for
an ordinary single-owner confirmed entry and for an in-use template, while
the name reads as "more than one skb owns this ct".

Both consumers at the end of the series check !nf_ct_is_confirmed(ct) first,
so there is no misbehaviour today, but a future caller without that check
would get a surprising answer.

There is also already an open-coded twin of this test in
nf_ct_drop_unconfirmed() in net/netfilter/nfnetlink_queue.c, which states the
precondition ("conntrack assumes exclusive skb->_nfct ownership for
unconfirmed entries") and applies it only after:

	if (!ct || nf_ct_is_confirmed(ct))
		return false;

Should that one be converted to the new helper so there is a single
definition of "shared"?

[Severity: Low]
This isn't a bug introduced by this patch, but the helper lands in the
shared netfilter header while the equivalent call site in net/sched/act_ct.c
is left alone here.

tcf_ct_act() runs the same sequence on a cached unconfirmed entry, adding
extensions and running helpers:

	err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
	...
	if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
		if (!nfct_seqadj_ext_add(ct))
			goto drop;

tcf_ct_skb_nfct_cached() has no exclusivity test either.

For what it is worth, the follow-up patch in this series, "net/sched:
act_ct: avoid modifying shared unconfirmed ct entry", adds the same guard to
tcf_ct_act() and extends it to the NAT-without-commit case, so the gap is
closed by the end of the series.  Mentioning that ordering in this changelog
might help backporters.

> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 0f433688e17b9..a733029c28dd0 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -734,6 +734,18 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>  	enum ip_conntrack_info ctinfo;
>  	struct nf_conn *ct;
>  
> +	/* If the ct entry is not confirmed and shared with some other skb,
> +	 * e.g., a cloned one, we can't just modify it with the commit as we
> +	 * must not modify the extension set.  Reset.
> +	 */
> +	if (cached && info->commit) {
> +		ct = nf_ct_get(skb, &ctinfo);
> +		if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
> +			nf_reset_ct(skb);
> +			cached = false;
> +		}
> +	}
> +

[Severity: Medium]
Since the test is a point-in-time refcount snapshot, does it exempt the last
remaining holder?

With two skbs sharing one unconfirmed entry and both executing ct(commit) in
the same zone:

  - the first one to reach the check sees use == 2, so nf_reset_ct() drops
    its reference (2 -> 1), nf_conntrack_in() runs again and it commits a
    brand new entry with the same tuple.

  - the second holder now sees use == 1, so nf_ct_shared() is false, it
    keeps the stale unconfirmed entry and commits that.

__nf_conntrack_confirm() then finds the equal tuple already in
nf_conntrack_hash and falls into nf_ct_resolve_clash(), where:

	l4proto = nf_ct_l4proto_find(nf_ct_protonum(ct));
	if (!l4proto->allow_clash)
		goto drop;

For TCP, SCTP and DCCP that returns NF_DROP, and verdict_to_errno(NF_DROP)
== -EINVAL propagates through ovs_ct_commit() and ovs_ct_execute(), so that
packet is dropped with OVS_DROP_CONNTRACK.  Before the patch both clones
were accepted, as the second nf_conntrack_confirm() was a no-op on the
already confirmed shared entry.

For UDP and ICMP the clash resolution succeeds, but __nf_ct_resolve_clash()
discards the losing entry, so the mark and labels the second clone just
wrote are silently lost.

The changelog says the reset "doesn't affect any known use cases".  Should
the drop and the mark/label loss be spelled out there, given both are
visible consequences for a pipeline that commits two clones into the same
zone?

The clone precondition itself comes from __nf_copy() in
include/linux/skbuff.h, which unconditionally takes a ct reference:

	dst->_nfct = src->_nfct;
	nf_conntrack_get(skb_nfct(src));

and skb_nfct_cached() returns true for the surviving clone since it only
checks netns, zone, helper, timeout and force.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org

Re: [PATCH net 2/6] net: openvswitch: conntrack: remove 'add_helper' dead code

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: after this patch the comment above the helper call reads
  '- nf_conntrack_in() was executed above ("!cached"), or - When…

--

net: openvswitch: conntrack: remove 'add_helper' dead code

This drops the add_helper variable in __ovs_ct_lookup(), since it is only
ever set for unconfirmed connections while it is only read on the
confirmed branch of the helper-call condition.  The helper-call condition
is simplified to nf_ct_is_confirmed(ct) ? !cached : info->commit.

> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index a733029c28dd0..c20f096eef40e 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c

[ ... ]

> @@ -821,13 +818,10 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>  		}
>  
>  		/* Call the helper only if:
> -		 * - nf_conntrack_in() was executed above ("!cached") or a
> -		 *   helper was just attached ("add_helper") for a confirmed
> -		 *   connection, or
> +		 * - nf_conntrack_in() was executed above ("!cached"), or
>  		 * - When committing an unconfirmed connection.
>  		 */
> -		if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
> -					      info->commit)) {
> +		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {

[Severity: Low]
This isn't a bug, but does the reworded comment still match the condition?

The first bullet now reads as an unqualified statement:

	/* Call the helper only if:
	 * - nf_conntrack_in() was executed above ("!cached"), or
	 * - When committing an unconfirmed connection.
	 */

while the code only honours "!cached" on the confirmed side of the
ternary:

	if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {

For a non-committing ct() action on a new packet in __ovs_ct_lookup(),
cached is false, the entry returned by nf_ct_get() is not yet confirmed
and info->commit is 0, so nf_ct_helper() is not called even though the
first bullet says it should be.

Worth noting that the following patch in the series, "net: openvswitch:
conntrack: fix helper UAF due to extensions realloc", rewrites both the
comment and the condition to:

	/* Call the helper only if nf_conntrack_in() was executed
	 * above ("!cached").
	 *
	 * For unconfirmed connections it will be called later during
	 * commit as we need to have all the other extensions allocated
	 * before the call.
	 */
	if (nf_ct_is_confirmed(ct) && !cached) {

so the mismatch only exists in this intermediate commit.  Could the
deferral note be pulled into this patch to keep each commit
self-consistent, given this one is tagged for stable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org

Re: [PATCH net 3/6] net: openvswitch: conntrack: fix helper UAF due to extensions realloc

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Inaccurate newly added comment in __ovs_ct_lookup()
  (net/openvswitch/conntrack.c:820-826).

Pre-existing issues:
- [Low] Concern (INVALIDATED by later patch in the same series): the
  identical helper/expectation use-after-free was claimed to remain…

--

net: openvswitch: conntrack: fix helper UAF due to extensions realloc

This moves the nf_ct_helper() call for unconfirmed connections out of
__ovs_ct_lookup() and into ovs_ct_commit(), after the mark, labels and
act_ct extensions have been added, so that a later extension realloc
cannot invalidate the expectation list pointer wired up by the helper.

No Link: tags are present in the commit header.

> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c20f096eef40e..d3326edcabf76 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -817,11 +817,14 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>  			}
>  		}
>  
> -		/* Call the helper only if:
> -		 * - nf_conntrack_in() was executed above ("!cached"), or
> -		 * - When committing an unconfirmed connection.
> +		/* Call the helper only if nf_conntrack_in() was executed
> +		 * above ("!cached").
> +		 *
> +		 * For unconfirmed connections it will be called later during
> +		 * commit as we need to have all the other extensions allocated
> +		 * before the call.
>  		 */
> -		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {
> +		if (nf_ct_is_confirmed(ct) && !cached) {

[Severity: Low]
Could this comment be made to match the condition below it?  The text says
the helper is called only if nf_conntrack_in() ran above ("!cached"), but
the guard is:

	if (nf_ct_is_confirmed(ct) && !cached) {

The nf_ct_is_confirmed() term isn't mentioned, which can read as if the
confirmed check were redundant.

The second half of the comment also promises that for unconfirmed
connections the helper "will be called later during commit".  Isn't that
only true for the committing path?  __ovs_ct_lookup() is also reached from
ovs_ct_lookup(), where info->commit is 0 and there is no nf_ct_helper()
call anywhere in that action:

net/openvswitch/conntrack.c:ovs_ct_lookup() {
	...
	err = __ovs_ct_lookup(net, key, info, skb);
	...
}

So for an unconfirmed entry created by nf_conntrack_in() during a plain
ct() lookup action, the helper is simply never run and there is no later
commit to run it.  Behaviour here is the same as before the patch (the old
nf_ct_is_confirmed(ct) ? !cached : info->commit guard skipped it too), so
this is only about the wording of the new comment.

>  			int err = nf_ct_helper(skb, ct, ctinfo, info->family);
>  
>  			err = verdict_to_errno(err);
> @@ -1025,6 +1028,14 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>  			return err;
>  
>  		nf_conn_act_ct_ext_add(skb, ct, ctinfo);
> +
> +		/* Call the helpers now.  We couldn't do this before as
> +		 * all the extensions must be allocated before the call.
> +		 */
> +		err = nf_ct_helper(skb, ct, ctinfo, info->family);
> +		err = verdict_to_errno(err);
> +		if (err)
> +			return err;

[Severity: Low]
This isn't a bug introduced by this patch, but the same ordering problem
exists in the other in-tree caller of nf_ct_helper(), tcf_ct_act() in
net/sched/act_ct.c, which at this commit runs the helper before the
extension add for unconfirmed entries:

net/sched/act_ct.c:tcf_ct_act() {
	...
	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
		err = nf_ct_helper(skb, ct, ctinfo, family);
	...
	if (commit) {
		tcf_ct_act_set_mark(...);
		tcf_ct_act_set_labels(...);
		if (!nf_ct_is_confirmed(ct))
			nf_conn_act_ct_ext_add(skb, ct, ctinfo);
	...
}

nf_conn_act_ct_ext_add() -> nf_ct_ext_add() -> krealloc() can move ct->ext,
leaving exp->lnode.pprev (which points into nf_conn_help inside the old
ext blob, stored by nf_ct_expect_insert() via
hlist_add_head_rcu(&exp->lnode, &master_help->expectations)) dangling.

Note this is already handled by the later patch in the same series,
"net/sched: act_ct: fix helper UAF due to extensions realloc", which moves
the tcf_ct_act() helper call below the nf_conn_act_ct_ext_add() block, so
no extension-adding call remains after the helper in either caller by the
end of the series.  Mentioned here only for completeness.

>  	} else if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
>  		   labels_nonzero(&info->labels.mask)) {
>  		err = ovs_ct_set_labels(ct, key, &info->labels.value,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org

Re: [PATCH net 4/6] net/sched: act_ct: avoid modifying shared unconfirmed ct entry

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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).

- [Medium] tcf_ct_act() now discards an unconfirmed conntrack entry
  purely because its refcount is >1, without checking whether the…

Pre-existing issues:
- [Medium] At this commit, tcf_ct_act() still calls nf_ct_helper()
  (net/sched/act_ct.c:1106) before the commit block that grows the…

--

net/sched: act_ct: avoid modifying shared unconfirmed ct entry

tcf_ct_act() now resets the skb conntrack association when the cached entry
is unconfirmed and its refcount shows it is shared with another skb, so that
a commit or nat pass cannot grow the extension area of an entry another
clone still points into.  The nat flag is also hoisted into a local
variable.

> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 55f3521edb4c9..e72143d36b119 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c

[ ... ]

> @@ -998,6 +998,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	retval = p->action;
>  	commit = p->ct_action & TCA_CT_ACT_COMMIT;
>  	clear = p->ct_action & TCA_CT_ACT_CLEAR;
> +	nat = p->ct_action & TCA_CT_ACT_NAT;
>  	tmpl = p->tmpl;
>  
>  	tcf_lastuse_update(&c->tcf_tm);
> @@ -1046,6 +1047,19 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  	 * different zone.
>  	 */
>  	cached = tcf_ct_skb_nfct_cached(net, skb, p);
> +
> +	/* If the ct entry is not confirmed and shared with some other skb,
> +	 * e.g., a cloned one, we can't just modify it with a commit or nat
> +	 * as we must not modify the extension set.  Reset.
> +	 */
> +	if (cached && (commit || nat)) {
> +		ct = nf_ct_get(skb, &ctinfo);
> +		if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
> +			nf_reset_ct(skb);
> +			cached = false;
> +		}
> +	}
> +

[Severity: Medium]
Can this drop NAT state that was already applied to the packet?

The condition keys off nf_ct_shared(ct), i.e. refcount > 1, and not off
whether the sharing skb will ever be re-processed by conntrack.  act_ct
permits nat without commit, and in that case
tcf_ct_act_nat()->nf_ct_nat() (net/netfilter/nf_nat_ovs.c) already
installs the binding and translates the headers on the unconfirmed entry:

net/netfilter/nf_nat_ovs.c:nf_ct_nat() {
	...
	/* Add NAT extension if not confirmed yet. */
	if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
		return NF_DROP;   /* Can't NAT. */
	...
	err = nf_ct_nat_execute(skb, ct, ctinfo, action, range, maniptype);
}

If the skb is then cloned by a path that keeps the nfct reference, and a
later filter runs "action ct zone N commit" on the original, the new block
sees cached && commit with an unconfirmed, shared entry, calls
nf_reset_ct(skb) and sets cached = false.  nf_conntrack_in() then re-tracks
the already translated packet, so the entry that gets committed carries the
post-NAT tuple as its ORIGINAL tuple and no matching binding, and reply
traffic is no longer reverse translated.  Before this change the cached,
NAT'ed unconfirmed entry was simply committed.

One note on the trigger: act_mirred and AF_PACKET taps do not produce this
sharing, since both clear the association on the clone:

net/sched/act_mirred.c:tcf_mirred_to_dev() {
	/* All mirred/redirected skbs should clear previous ct info */
	nf_reset_ct(skb_to_send);
}

net/packet/af_packet.c:packet_rcv() {
	/* drop conntrack reference */
	nf_reset_ct(skb);
}

Bridge flood/multicast delivery does keep it, though -
net/bridge/br_forward.c:deliver_clone() calls skb_clone() with no
nf_reset_ct(), so __nf_copy() raises ct_general.use on the clone.

Would it be worth mentioning in the changelog that the reset also fires
when the second pass does commit?  The changelog currently only argues
about the "without commit" case:

    This, however, doesn't really change the picture for sensible
    networking cases as there should be no need to run the same packet
    twice (before and after the clone) through conntrack without packet
    header or zone changes and without commit.

[Severity: Medium]
This is a pre-existing ordering problem and is not introduced by this
patch, but it is worth flagging for backports since the new guard does not
cover it.

At this commit tcf_ct_act() still runs the helper before the commit block
grows the extension area of an unconfirmed entry:

net/sched/act_ct.c:tcf_ct_act() {
	...
	if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
		err = nf_ct_helper(skb, ct, ctinfo, family);
		if (err != NF_ACCEPT)
			goto nf_error;
	}

	if (commit) {
		tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
		tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);

		if (!nf_ct_is_confirmed(ct))
			nf_conn_act_ct_ext_add(skb, ct, ctinfo);
	...
}

A helper that inserts an expectation links exp->lnode into a list head that
lives inside ct->ext:

net/netfilter/nf_conntrack_expect.c:nf_ct_expect_insert() {
	hlist_add_head_rcu(&exp->lnode, &master_help->expectations);
}

and the following nf_ct_ext_add(NF_CT_EXT_ACT_CT) relocates that memory:

net/netfilter/nf_conntrack_extend.c:nf_ct_ext_add() {
	new = krealloc(old, alloc, gfp);
}

leaving exp->lnode.pprev pointing into the freed block, which is written
through when the expectation is unlinked via nf_ct_remove_expectations().

The new reset only fires for cached entries with refcount > 1, so an entry
that was just tracked (cached == false), or the replacement entry created
after the reset, still follows the helper-then-realloc sequence with a
single skb reference.

The later patch in this series ("net/sched: act_ct: fix helper UAF due to
extensions realloc") moves nf_ct_helper() after the
nf_conn_act_ct_ext_add() block with the comment "This has to be done after
all the extensions are already added.", so the ordering is resolved within
the series.  Given the stable tag here, should the two patches be marked so
that backporters take the reordering change together with this one?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org

Re: [PATCH net 6/6] net/sched: act_ct: fix helper UAF due to extensions realloc

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Comment vs. code mismatch introduced by this patch in
  tcf_ct_act() (net/sched/act_ct.c:1113-1117).

--

net/sched: act_ct: fix helper UAF due to extensions realloc

This moves the nf_ct_helper() call in tcf_ct_act() to after the mark,
labels and act_ct extension updates, so that no further extension
reallocation can happen once a helper has wired a raw pointer to the
extension area into the expectations list.  The executable condition
guarding the helper call is unchanged, only relocated.

> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index f62051ec9d57d..411e3dd92d072 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1102,19 +1102,25 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>  		}
>  	}
>  
> -	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
> -		err = nf_ct_helper(skb, ct, ctinfo, family);
> -		if (err != NF_ACCEPT)
> -			goto nf_error;
> -	}
> -
>  	if (commit) {
>  		tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
>  		tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
>  
>  		if (!nf_ct_is_confirmed(ct))
>  			nf_conn_act_ct_ext_add(skb, ct, ctinfo);
> +	}
>  
> +	/* Run helpers for the connection if nf_conntrack_in() was executed
> +	 * or if we're about to commit.  This has to be done after all the
> +	 * extensions are already added.
> +	 */
> +	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {

[Severity: Low]
Does the new comment match the condition it annotates?  The prose reads as
an unconditional "or", but the ternary below it skips the helper in both of
those cases in some configurations.

First packet through a non-committing ct action: tcf_ct_skb_nfct_cached()
misses so cached is false, nf_conntrack_in() runs and creates an
unconfirmed ct, and then:

	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {

takes the commit branch, which is false here, so the helper is not called
even though nf_conntrack_in() was executed.

Recirculation case: a previous ct action already confirmed the entry, and a
later committing ct action in the same zone hits the cached path, so
nf_ct_is_confirmed(ct) is true and !cached is false.  The helper is not
called even though we are about to commit.

The peer call site of the same nf_ct_helper() API words it in a way that
matches the code, in __ovs_ct_lookup() in net/openvswitch/conntrack.c:

	/* Call the helper only if nf_conntrack_in() was executed
	 * above ("!cached").
	 *
	 * For unconfirmed connections it will be called later during
	 * commit as we need to have all the other extensions allocated
	 * before the call.
	 */
	if (nf_ct_is_confirmed(ct) && !cached) {

Since this patch is about the ordering contract between nf_ct_helper() and
the extension allocations, could the comment be reworded along the same
lines, so the two in-tree descriptions of the same contract agree?

> +		err = nf_ct_helper(skb, ct, ctinfo, family);
> +		if (err != NF_ACCEPT)
> +			goto nf_error;
> +	}
> +
> +	if (commit) {
>  		/* This will take care of sending queued events
>  		 * even if the connection is already confirmed.
>  		 */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org

Re: [PATCH net 1/6] net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> In a case where skb with an unconfirmed ct entry gets cloned, we may
> end up committing both but with different sets of extensions.
>
> The series of events:
>
>  1. The first clone wants to commit and runs the helpers wiring up
>     the extension pointer into the expectation list.
>  2. Then it looses the confirmation keeping the entry unconfirmed.
>  3. Second clone now wants to commit labels and adds the new extension
>     for that breaking the pointer in the expectation list causing
>     UAF on the destruction path later.
>
> While this is possible to trigger, there should be no practical
> network pipeline where committing both clones without modifications
> into the same zone is needed.  So, let's just reset the entry in case
> for some reason we got an skb with a shared one during commit.  This
> doesn't affect any known use cases, but avoids any potential problems
> with sharing and modification of the unconfirmed ct entry.
>
> The fixes tag points to the introduction of helpers, since that's the
> main UAF trigger for the sharing.
>
> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
> Cc: stable@vger.kernel.org
> Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net 2/6] net: openvswitch: conntrack: remove 'add_helper' dead code

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> This variable can only become 'true' when the connection is not
> confirmed, but it is only checked when it is confirmed.  So, it can be
> treated as being always false and just removed.
>
> Fixes: 3c1860543fcc ("openvswitch: add nf_ct_is_confirmed check before
> assigning the helper")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
> @@ -821,13 +818,10 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>  		}
>  
>  		/* Call the helper only if:
> -		 * - nf_conntrack_in() was executed above ("!cached") or a
> -		 *   helper was just attached ("add_helper") for a confirmed
> -		 *   connection, or
> +		 * - nf_conntrack_in() was executed above ("!cached"), or
>  		 * - When committing an unconfirmed connection.
>  		 */
> -		if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
> -					      info->commit)) {
> +		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {

This is already a good reason to apply - the existing construct wasn't
particularly friendly.  And the follow up in 3/6 is perfect.

>  			int err = nf_ct_helper(skb, ct, ctinfo, info->family);
>  
>  			err = verdict_to_errno(err);

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net 3/6] net: openvswitch: conntrack: fix helper UAF due to extensions realloc

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> While calling the helpers, a raw pointer to the extensions area is
> wired into expectations list:
>
>   -> nf_ct_helper()
>    -> helper->help()
>     -> nf_ct_expect_related_report()
>      -> nf_ct_expect_insert()
>       -> hlist_add_head_rcu(&exp->lnode, &master_help->expectations)
>
> In case the connection is not confirmed yet, more extensions can be
> added afterwards with *_ext_add() calls reallocating the extension
> space and leaving the now invalid pointer in the expectations list
> that is later accessed while removing the expectation.
>
> Make sure that helpers are called at the end after all the other
> extensions are already added.
>
> Note that the helper rejection now leaves the mark and labels set,
> but that's not different from how the NAT was handled before or how
> the mark and the labels were handled on confirmation failure.  And
> there are no atomicity guarantees provided by the API anyway.
>
> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action")
> Cc: stable@vger.kernel.org
> Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net 1/6] net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry

[Ilya Maximets]

On 9/22/26 4:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1 (1)
> 
> 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).
> 
> - [Medium] New behaviour change introduced by the guard in
>   __ovs_ct_lookup() (net/openvswitch/conntrack.c:741-747): when two skbs…
> - [Low] nf_ct_shared() is added as an undocumented generic predicate in
>   include/net/netfilter/nf_conntrack.h:188-191 returning…
> 
> Pre-existing issues:
> - [Low] Peer call site net/sched/act_ct.c (tcf_ct_act()) performs the
>   same 'cached, unconfirmed ct + commit modifies the extension set'…
> 
> --
> 
> net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry
> 
> This adds a new nf_ct_shared() helper to the conntrack header and uses it in
> __ovs_ct_lookup() to reset the cached conntrack entry when an skb arrives at
> a committing ct action with an unconfirmed entry that is shared with another
> skb.  The goal is to avoid two clones committing the same unconfirmed entry
> with different extension sets, which can lead to a use-after-free.
> 
> A few questions below.
> 
>> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
>> index bc42dd0e10e65..c39425e54d87d 100644
>> --- a/include/net/netfilter/nf_conntrack.h
>> +++ b/include/net/netfilter/nf_conntrack.h
>> @@ -185,6 +185,11 @@ static inline void nf_ct_put(struct nf_conn *ct)
>>  		nf_ct_destroy(&ct->ct_general);
>>  }
>>  
>> +static inline bool nf_ct_shared(const struct nf_conn *ct)
>> +{
>> +	return refcount_read(&ct->ct_general.use) > 1;
>> +}
>> +
> 
> [Severity: Low]
> Would it be worth documenting that this predicate is only meaningful for
> unconfirmed, non-template entries?
> 
> The reference accounting comment on struct nf_conn in this same header says
> the count is "1 for hash table, 1 per skb, plus 1 for any connection(s) we
> are `master' for".  With that, every confirmed entry owned by a single skb
> already has use == 2, since __nf_conntrack_confirm() does:
> 
> 	/* The caller holds a reference to this object */
> 	refcount_set(&ct->ct_general.use, 2);
> 
> The same is true for a template in use: nf_ct_tmpl_alloc() sets use = 1 and
> nf_ct_set(skb, tmpl, ...) adds another.  So nf_ct_shared() returns true for
> an ordinary single-owner confirmed entry and for an in-use template, while
> the name reads as "more than one skb owns this ct".

The comments in the callers state the cloned skb as an example, not as a
sole reason for the entry being shared.  "shared" just means there is more
than one reference here.  I can add a comment to the function itself if
someone thinks this is not clear enough, but otherwise I'd keep as is.

> Both consumers at the end of the series check !nf_ct_is_confirmed(ct) first,
> so there is no misbehaviour today, but a future caller without that check
> would get a surprising answer.
> 
> There is also already an open-coded twin of this test in
> nf_ct_drop_unconfirmed() in net/netfilter/nfnetlink_queue.c, which states the
> precondition ("conntrack assumes exclusive skb->_nfct ownership for
> unconfirmed entries") and applies it only after:
> 
> 	if (!ct || nf_ct_is_confirmed(ct))
> 		return false;
> 
> Should that one be converted to the new helper so there is a single
> definition of "shared"?

nfnetlink_queue needs some work on its own and can be cleaned up separately.

> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but the helper lands in the
> shared netfilter header while the equivalent call site in net/sched/act_ct.c
> is left alone here.
> 
> tcf_ct_act() runs the same sequence on a cached unconfirmed entry, adding
> extensions and running helpers:
> 
> 	err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
> 	...
> 	if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
> 		if (!nfct_seqadj_ext_add(ct))
> 			goto drop;
> 
> tcf_ct_skb_nfct_cached() has no exclusivity test either.
> 
> For what it is worth, the follow-up patch in this series, "net/sched:
> act_ct: avoid modifying shared unconfirmed ct entry", adds the same guard to
> tcf_ct_act() and extends it to the NAT-without-commit case, so the gap is
> closed by the end of the series.  Mentioning that ordering in this changelog
> might help backporters.

Preexisting, fixed later in the set.

> 
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index 0f433688e17b9..a733029c28dd0 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -734,6 +734,18 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>>  	enum ip_conntrack_info ctinfo;
>>  	struct nf_conn *ct;
>>  
>> +	/* If the ct entry is not confirmed and shared with some other skb,
>> +	 * e.g., a cloned one, we can't just modify it with the commit as we
>> +	 * must not modify the extension set.  Reset.
>> +	 */
>> +	if (cached && info->commit) {
>> +		ct = nf_ct_get(skb, &ctinfo);
>> +		if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
>> +			nf_reset_ct(skb);
>> +			cached = false;
>> +		}
>> +	}
>> +
> 
> [Severity: Medium]
> Since the test is a point-in-time refcount snapshot, does it exempt the last
> remaining holder?
> 
> With two skbs sharing one unconfirmed entry and both executing ct(commit) in
> the same zone:
> 
>   - the first one to reach the check sees use == 2, so nf_reset_ct() drops
>     its reference (2 -> 1), nf_conntrack_in() runs again and it commits a
>     brand new entry with the same tuple.
> 
>   - the second holder now sees use == 1, so nf_ct_shared() is false, it
>     keeps the stale unconfirmed entry and commits that.
> 
> __nf_conntrack_confirm() then finds the equal tuple already in
> nf_conntrack_hash and falls into nf_ct_resolve_clash(), where:
> 
> 	l4proto = nf_ct_l4proto_find(nf_ct_protonum(ct));
> 	if (!l4proto->allow_clash)
> 		goto drop;
> 
> For TCP, SCTP and DCCP that returns NF_DROP, and verdict_to_errno(NF_DROP)
> == -EINVAL propagates through ovs_ct_commit() and ovs_ct_execute(), so that
> packet is dropped with OVS_DROP_CONNTRACK.  Before the patch both clones
> were accepted, as the second nf_conntrack_confirm() was a no-op on the
> already confirmed shared entry.
> 
> For UDP and ICMP the clash resolution succeeds, but __nf_ct_resolve_clash()
> discards the losing entry, so the mark and labels the second clone just
> wrote are silently lost.
> 
> The changelog says the reset "doesn't affect any known use cases".  Should
> the drop and the mark/label loss be spelled out there, given both are
> visible consequences for a pipeline that commits two clones into the same
> zone?
> 
> The clone precondition itself comes from __nf_copy() in
> include/linux/skbuff.h, which unconditionally takes a ct reference:
> 
> 	dst->_nfct = src->_nfct;
> 	nf_conntrack_get(skb_nfct(src));
> 
> and skb_nfct_cached() returns true for the surviving clone since it only
> checks netns, zone, helper, timeout and force.
> 

All of that can happen even without cloning.  With or without the changes
applied.  The entry can be created separately and then clash with the one
in the skb causing a drop.  This is expected.  The "use case" is also not
meaningful in practice.

Best regards, Ilya Maximets.

Re: [PATCH net 4/6] net/sched: act_ct: avoid modifying shared unconfirmed ct entry

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> In a case where skb with an unconfirmed ct entry gets cloned, we may
> end up processing both again but with different sets of extensions.
>
> The series of events:
>
>  1. The first clone wants to commit and runs the helpers wiring up
>     the extension pointer into the expectation list.
>  2. Then it looses the confirmation keeping the entry unconfirmed.
>  3. Second clone now wants to commit labels or run NAT and adds the
>     new extension for that breaking the pointer in the expectation
>     list causing UAF on the destruction path later.
>
> While this is possible to trigger, there should be no practical
> network pipeline where we need to process both clones without
> modifications in the same zone.  So, let's just reset the entry in
> case for some reason we got an skb with a shared one.  This doesn't
> affect any known use cases, but avoids any potential problems with
> sharing and modification of the unconfirmed ct entry.
>
> Unlike openvswitch module, act_ct allows for NAT without commit.
> Changing that would be a uAPI break.  So, act_ct needs to reset on NAT
> regardless of the commit flag to avoid reallocation of the extension
> space.  This, however, doesn't really change the picture for sensible
> networking cases as there should be no need to run the same packet
> twice (before and after the clone) through conntrack without packet
> header or zone changes and without commit.
>
> The fixes tag points to the introduction of helpers, since that's the
> main UAF trigger for the sharing.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net 5/6] net/sched: act_ct: remove 'add_helper' dead code

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> This variable can only become 'true' when the connection is not
> confirmed, but it is only checked when it is confirmed.  So, it can be
> treated as being always false and just removed.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net 2/6] net: openvswitch: conntrack: remove 'add_helper' dead code

[Ilya Maximets]

On 9/22/26 4:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Concern: after this patch the comment above the helper call reads
>   '- nf_conntrack_in() was executed above ("!cached"), or - When…
> 
> --
> 
> net: openvswitch: conntrack: remove 'add_helper' dead code
> 
> This drops the add_helper variable in __ovs_ct_lookup(), since it is only
> ever set for unconfirmed connections while it is only read on the
> confirmed branch of the helper-call condition.  The helper-call condition
> is simplified to nf_ct_is_confirmed(ct) ? !cached : info->commit.
> 
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index a733029c28dd0..c20f096eef40e 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
> 
> [ ... ]
> 
>> @@ -821,13 +818,10 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>>  		}
>>  
>>  		/* Call the helper only if:
>> -		 * - nf_conntrack_in() was executed above ("!cached") or a
>> -		 *   helper was just attached ("add_helper") for a confirmed
>> -		 *   connection, or
>> +		 * - nf_conntrack_in() was executed above ("!cached"), or
>>  		 * - When committing an unconfirmed connection.
>>  		 */
>> -		if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
>> -					      info->commit)) {
>> +		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {
> 
> [Severity: Low]
> This isn't a bug, but does the reworded comment still match the condition?
> 
> The first bullet now reads as an unqualified statement:
> 
> 	/* Call the helper only if:
> 	 * - nf_conntrack_in() was executed above ("!cached"), or
> 	 * - When committing an unconfirmed connection.
> 	 */
> 
> while the code only honours "!cached" on the confirmed side of the
> ternary:
> 
> 	if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {
> 
> For a non-committing ct() action on a new packet in __ovs_ct_lookup(),
> cached is false, the entry returned by nf_ct_get() is not yet confirmed
> and info->commit is 0, so nf_ct_helper() is not called even though the
> first bullet says it should be.
> 
> Worth noting that the following patch in the series, "net: openvswitch:
> conntrack: fix helper UAF due to extensions realloc", rewrites both the
> comment and the condition to:
> 
> 	/* Call the helper only if nf_conntrack_in() was executed
> 	 * above ("!cached").
> 	 *
> 	 * For unconfirmed connections it will be called later during
> 	 * commit as we need to have all the other extensions allocated
> 	 * before the call.
> 	 */
> 	if (nf_ct_is_confirmed(ct) && !cached) {
> 
> so the mismatch only exists in this intermediate commit.  Could the
> deferral note be pulled into this patch to keep each commit
> self-consistent, given this one is tagged for stable?
Comments do not need to spell out verbatim what the code is doing,
it is pointless for the most part.  The current comment, IMO, is
conveying the intention clear enough.  It is also re-written in the
very next patch.

Best regards, Ilya Maximets.

Re: [PATCH net 3/6] net: openvswitch: conntrack: fix helper UAF due to extensions realloc

[Ilya Maximets]

On 9/22/26 4:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Inaccurate newly added comment in __ovs_ct_lookup()
>   (net/openvswitch/conntrack.c:820-826).

Same thing as for the previous patch.  Comments do not need to
spell out everything that happens in the code.  The intention of
the comment and the code seems clear enough to me.  So, I'd keep
it as-is, unless someone thinks it needs more clarification.

> 
> Pre-existing issues:
> - [Low] Concern (INVALIDATED by later patch in the same series): the
>   identical helper/expectation use-after-free was claimed to remain…

Preexisting and fixed later in the set.

Best regards, Ilya Maximets.

Re: [PATCH net 4/6] net/sched: act_ct: avoid modifying shared unconfirmed ct entry

[Ilya Maximets]

On 9/22/26 4:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (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).
> 
> - [Medium] tcf_ct_act() now discards an unconfirmed conntrack entry
>   purely because its refcount is >1, without checking whether the…
> 
> Pre-existing issues:
> - [Medium] At this commit, tcf_ct_act() still calls nf_ct_helper()
>   (net/sched/act_ct.c:1106) before the commit block that grows the…
> 
> --
> 
> net/sched: act_ct: avoid modifying shared unconfirmed ct entry
> 
> tcf_ct_act() now resets the skb conntrack association when the cached entry
> is unconfirmed and its refcount shows it is shared with another skb, so that
> a commit or nat pass cannot grow the extension area of an entry another
> clone still points into.  The nat flag is also hoisted into a local
> variable.
> 
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index 55f3521edb4c9..e72143d36b119 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
> 
> [ ... ]
> 
>> @@ -998,6 +998,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>>  	retval = p->action;
>>  	commit = p->ct_action & TCA_CT_ACT_COMMIT;
>>  	clear = p->ct_action & TCA_CT_ACT_CLEAR;
>> +	nat = p->ct_action & TCA_CT_ACT_NAT;
>>  	tmpl = p->tmpl;
>>  
>>  	tcf_lastuse_update(&c->tcf_tm);
>> @@ -1046,6 +1047,19 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>>  	 * different zone.
>>  	 */
>>  	cached = tcf_ct_skb_nfct_cached(net, skb, p);
>> +
>> +	/* If the ct entry is not confirmed and shared with some other skb,
>> +	 * e.g., a cloned one, we can't just modify it with a commit or nat
>> +	 * as we must not modify the extension set.  Reset.
>> +	 */
>> +	if (cached && (commit || nat)) {
>> +		ct = nf_ct_get(skb, &ctinfo);
>> +		if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
>> +			nf_reset_ct(skb);
>> +			cached = false;
>> +		}
>> +	}
>> +
> 
> [Severity: Medium]
> Can this drop NAT state that was already applied to the packet?
> 
> The condition keys off nf_ct_shared(ct), i.e. refcount > 1, and not off
> whether the sharing skb will ever be re-processed by conntrack.  act_ct
> permits nat without commit, and in that case
> tcf_ct_act_nat()->nf_ct_nat() (net/netfilter/nf_nat_ovs.c) already
> installs the binding and translates the headers on the unconfirmed entry:
> 
> net/netfilter/nf_nat_ovs.c:nf_ct_nat() {
> 	...
> 	/* Add NAT extension if not confirmed yet. */
> 	if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
> 		return NF_DROP;   /* Can't NAT. */
> 	...
> 	err = nf_ct_nat_execute(skb, ct, ctinfo, action, range, maniptype);
> }
> 
> If the skb is then cloned by a path that keeps the nfct reference, and a
> later filter runs "action ct zone N commit" on the original, the new block
> sees cached && commit with an unconfirmed, shared entry, calls
> nf_reset_ct(skb) and sets cached = false.  nf_conntrack_in() then re-tracks
> the already translated packet, so the entry that gets committed carries the
> post-NAT tuple as its ORIGINAL tuple and no matching binding, and reply
> traffic is no longer reverse translated.  Before this change the cached,
> NAT'ed unconfirmed entry was simply committed.

It's true that some information will be lost on reset, but it is expected.
The described sequence of events should also not happen in a practical
networking pipeline.  Alternative is to forbid nat without commit, which
would be a significant uAPI break.

> 
> One note on the trigger: act_mirred and AF_PACKET taps do not produce this
> sharing, since both clear the association on the clone:
> 
> net/sched/act_mirred.c:tcf_mirred_to_dev() {
> 	/* All mirred/redirected skbs should clear previous ct info */
> 	nf_reset_ct(skb_to_send);
> }
> 
> net/packet/af_packet.c:packet_rcv() {
> 	/* drop conntrack reference */
> 	nf_reset_ct(skb);
> }
> 
> Bridge flood/multicast delivery does keep it, though -
> net/bridge/br_forward.c:deliver_clone() calls skb_clone() with no
> nf_reset_ct(), so __nf_copy() raises ct_general.use on the clone.
> 
> Would it be worth mentioning in the changelog that the reset also fires
> when the second pass does commit?  The changelog currently only argues
> about the "without commit" case:
> 
>     This, however, doesn't really change the picture for sensible
>     networking cases as there should be no need to run the same packet
>     twice (before and after the clone) through conntrack without packet
>     header or zone changes and without commit.

Flood cases will be addressed separately, there is no need to spell out
every single thing.

> 
> [Severity: Medium]
> This is a pre-existing ordering problem and is not introduced by this
> patch, but it is worth flagging for backports since the new guard does not
> cover it.
> 
> At this commit tcf_ct_act() still runs the helper before the commit block
> grows the extension area of an unconfirmed entry:
> 
> net/sched/act_ct.c:tcf_ct_act() {
> 	...
> 	if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
> 		err = nf_ct_helper(skb, ct, ctinfo, family);
> 		if (err != NF_ACCEPT)
> 			goto nf_error;
> 	}
> 
> 	if (commit) {
> 		tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
> 		tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
> 
> 		if (!nf_ct_is_confirmed(ct))
> 			nf_conn_act_ct_ext_add(skb, ct, ctinfo);
> 	...
> }
> 
> A helper that inserts an expectation links exp->lnode into a list head that
> lives inside ct->ext:
> 
> net/netfilter/nf_conntrack_expect.c:nf_ct_expect_insert() {
> 	hlist_add_head_rcu(&exp->lnode, &master_help->expectations);
> }
> 
> and the following nf_ct_ext_add(NF_CT_EXT_ACT_CT) relocates that memory:
> 
> net/netfilter/nf_conntrack_extend.c:nf_ct_ext_add() {
> 	new = krealloc(old, alloc, gfp);
> }
> 
> leaving exp->lnode.pprev pointing into the freed block, which is written
> through when the expectation is unlinked via nf_ct_remove_expectations().
> 
> The new reset only fires for cached entries with refcount > 1, so an entry
> that was just tracked (cached == false), or the replacement entry created
> after the reset, still follows the helper-then-realloc sequence with a
> single skb reference.
> 
> The later patch in this series ("net/sched: act_ct: fix helper UAF due to
> extensions realloc") moves nf_ct_helper() after the
> nf_conn_act_ct_ext_add() block with the comment "This has to be done after
> all the extensions are already added.", so the ordering is resolved within
> the series.  Given the stable tag here, should the two patches be marked so
> that backporters take the reordering change together with this one?
Preexisting.  Fixed later in the set.

Best regards, Ilya Maximets.

Re: [PATCH net 6/6] net/sched: act_ct: fix helper UAF due to extensions realloc

[Ilya Maximets]

On 9/22/26 4:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Comment vs. code mismatch introduced by this patch in
>   tcf_ct_act() (net/sched/act_ct.c:1113-1117).

Same thing as for the previous patches.  Comments do not need to
spell out everything that happens in the code.

Best regards, Ilya Maximets.

Re: [PATCH net 4/6] net/sched: act_ct: avoid modifying shared unconfirmed ct entry

[Xin Long]

On Mon, Sep 21, 2026 at 10:57 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> In a case where skb with an unconfirmed ct entry gets cloned, we may
> end up processing both again but with different sets of extensions.
>
> The series of events:
>
>  1. The first clone wants to commit and runs the helpers wiring up
>     the extension pointer into the expectation list.
>  2. Then it looses the confirmation keeping the entry unconfirmed.
>  3. Second clone now wants to commit labels or run NAT and adds the
>     new extension for that breaking the pointer in the expectation
>     list causing UAF on the destruction path later.
>
> While this is possible to trigger, there should be no practical
> network pipeline where we need to process both clones without
> modifications in the same zone.  So, let's just reset the entry in
> case for some reason we got an skb with a shared one.  This doesn't
> affect any known use cases, but avoids any potential problems with
> sharing and modification of the unconfirmed ct entry.
>
> Unlike openvswitch module, act_ct allows for NAT without commit.
> Changing that would be a uAPI break.  So, act_ct needs to reset on NAT
> regardless of the commit flag to avoid reallocation of the extension
> space.  This, however, doesn't really change the picture for sensible
> networking cases as there should be no need to run the same packet
> twice (before and after the clone) through conntrack without packet
> header or zone changes and without commit.
>
> The fixes tag points to the introduction of helpers, since that's the
> main UAF trigger for the sharing.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Reviewed-by: Xin Long <lucien.xin@gmail.com>

Re: [PATCH net 5/6] net/sched: act_ct: remove 'add_helper' dead code

[Xin Long]

On Mon, Sep 21, 2026 at 10:57 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> This variable can only become 'true' when the connection is not
> confirmed, but it is only checked when it is confirmed.  So, it can be
> treated as being always false and just removed.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  net/sched/act_ct.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index e72143d36b119..f62051ec9d57d 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -986,7 +986,6 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>         struct nf_hook_state state;
>         int nh_ofs, err, retval;
>         struct tcf_ct_params *p;
> -       bool add_helper = false;
>         bool skb_is_ours = false;
>         bool skip_add = false;
>         bool defrag = false;
> @@ -1096,14 +1095,14 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>                 err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
>                 if (err)
>                         goto drop;
> -               add_helper = true;
> +
>                 if (nat && !nfct_seqadj(ct)) {
>                         if (!nfct_seqadj_ext_add(ct))
>                                 goto drop;
>                 }
>         }
>
> -       if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
> +       if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
>                 err = nf_ct_helper(skb, ct, ctinfo, family);
>                 if (err != NF_ACCEPT)
>                         goto nf_error;
> --
> 2.55.0
>

Reviewed-by: Xin Long <lucien.xin@gmail.com>

Re: [PATCH net 6/6] net/sched: act_ct: fix helper UAF due to extensions realloc

[Xin Long]

On Mon, Sep 21, 2026 at 10:57 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> While calling the helpers, a raw pointer to the extensions area is
> wired into expectations list:
>
>   -> nf_ct_helper()
>    -> helper->help()
>     -> nf_ct_expect_related_report()
>      -> nf_ct_expect_insert()
>       -> hlist_add_head_rcu(&exp->lnode, &master_help->expectations)
>
> In case the connection is not confirmed yet, more extensions can be
> added afterwards with *_ext_add() calls reallocating the extension
> space and leaving the now invalid pointer in the expectations list
> that is later accessed while removing the expectation.
>
> Make sure that helpers are called at the end after all the other
> extensions are already added.
>
> Note that the helper rejection now leaves the mark and labels set,
> but that's not different from how the NAT was handled before or how
> the mark and the labels were handled on confirmation failure.  And
> there are no atomicity guarantees provided by the API anyway.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  net/sched/act_ct.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index f62051ec9d57d..411e3dd92d072 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1102,19 +1102,25 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>                 }
>         }
>
> -       if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
> -               err = nf_ct_helper(skb, ct, ctinfo, family);
> -               if (err != NF_ACCEPT)
> -                       goto nf_error;
> -       }
> -
>         if (commit) {
>                 tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
>                 tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
>
>                 if (!nf_ct_is_confirmed(ct))
>                         nf_conn_act_ct_ext_add(skb, ct, ctinfo);
> +       }
>
> +       /* Run helpers for the connection if nf_conntrack_in() was executed
> +        * or if we're about to commit.  This has to be done after all the
> +        * extensions are already added.
> +        */
> +       if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
> +               err = nf_ct_helper(skb, ct, ctinfo, family);
> +               if (err != NF_ACCEPT)
> +                       goto nf_error;
> +       }
> +
> +       if (commit) {
>                 /* This will take care of sending queued events
>                  * even if the connection is already confirmed.
>                  */
> --
> 2.55.0
>

Reviewed-by: Xin Long <lucien.xin@gmail.com>

Re: [PATCH net 4/6] net/sched: act_ct: avoid modifying shared unconfirmed ct entry

[Jamal Hadi Salim]

On Mon, Sep 21, 2026 at 10:57 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> In a case where skb with an unconfirmed ct entry gets cloned, we may
> end up processing both again but with different sets of extensions.
>
> The series of events:
>
>  1. The first clone wants to commit and runs the helpers wiring up
>     the extension pointer into the expectation list.
>  2. Then it looses the confirmation keeping the entry unconfirmed.
>  3. Second clone now wants to commit labels or run NAT and adds the
>     new extension for that breaking the pointer in the expectation
>     list causing UAF on the destruction path later.
>
> While this is possible to trigger, there should be no practical
> network pipeline where we need to process both clones without
> modifications in the same zone.  So, let's just reset the entry in
> case for some reason we got an skb with a shared one.  This doesn't
> affect any known use cases, but avoids any potential problems with
> sharing and modification of the unconfirmed ct entry.
>
> Unlike openvswitch module, act_ct allows for NAT without commit.
> Changing that would be a uAPI break.  So, act_ct needs to reset on NAT
> regardless of the commit flag to avoid reallocation of the extension
> space.  This, however, doesn't really change the picture for sensible
> networking cases as there should be no need to run the same packet
> twice (before and after the clone) through conntrack without packet
> header or zone changes and without commit.
>
> The fixes tag points to the introduction of helpers, since that's the
> main UAF trigger for the sharing.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

> ---
>  net/sched/act_ct.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 55f3521edb4c9..e72143d36b119 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -979,11 +979,11 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>                                  struct tcf_result *res)
>  {
>         struct net *net = dev_net(skb->dev);
> +       bool cached, commit, clear, nat;
>         enum ip_conntrack_info ctinfo;
>         struct tcf_ct *c = to_ct(a);
>         struct nf_conn *tmpl = NULL;
>         struct nf_hook_state state;
> -       bool cached, commit, clear;
>         int nh_ofs, err, retval;
>         struct tcf_ct_params *p;
>         bool add_helper = false;
> @@ -998,6 +998,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>         retval = p->action;
>         commit = p->ct_action & TCA_CT_ACT_COMMIT;
>         clear = p->ct_action & TCA_CT_ACT_CLEAR;
> +       nat = p->ct_action & TCA_CT_ACT_NAT;
>         tmpl = p->tmpl;
>
>         tcf_lastuse_update(&c->tcf_tm);
> @@ -1046,6 +1047,19 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>          * different zone.
>          */
>         cached = tcf_ct_skb_nfct_cached(net, skb, p);
> +
> +       /* If the ct entry is not confirmed and shared with some other skb,
> +        * e.g., a cloned one, we can't just modify it with a commit or nat
> +        * as we must not modify the extension set.  Reset.
> +        */
> +       if (cached && (commit || nat)) {
> +               ct = nf_ct_get(skb, &ctinfo);
> +               if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
> +                       nf_reset_ct(skb);
> +                       cached = false;
> +               }
> +       }
> +
>         if (!cached) {
>                 if (tcf_ct_flow_table_lookup(p, skb, family)) {
>                         skip_add = true;
> @@ -1083,7 +1097,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>                 if (err)
>                         goto drop;
>                 add_helper = true;
> -               if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
> +               if (nat && !nfct_seqadj(ct)) {
>                         if (!nfct_seqadj_ext_add(ct))
>                                 goto drop;
>                 }
> --
> 2.55.0
>

Re: [PATCH net 5/6] net/sched: act_ct: remove 'add_helper' dead code

[Jamal Hadi Salim]

On Mon, Sep 21, 2026 at 10:57 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> This variable can only become 'true' when the connection is not
> confirmed, but it is only checked when it is confirmed.  So, it can be
> treated as being always false and just removed.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
> ---
>  net/sched/act_ct.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index e72143d36b119..f62051ec9d57d 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -986,7 +986,6 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>         struct nf_hook_state state;
>         int nh_ofs, err, retval;
>         struct tcf_ct_params *p;
> -       bool add_helper = false;
>         bool skb_is_ours = false;
>         bool skip_add = false;
>         bool defrag = false;
> @@ -1096,14 +1095,14 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>                 err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
>                 if (err)
>                         goto drop;
> -               add_helper = true;
> +
>                 if (nat && !nfct_seqadj(ct)) {
>                         if (!nfct_seqadj_ext_add(ct))
>                                 goto drop;
>                 }
>         }
>
> -       if (nf_ct_is_confirmed(ct) ? ((!cached && !skip_add) || add_helper) : commit) {
> +       if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
>                 err = nf_ct_helper(skb, ct, ctinfo, family);
>                 if (err != NF_ACCEPT)
>                         goto nf_error;
> --
> 2.55.0
>

Re: [PATCH net 6/6] net/sched: act_ct: fix helper UAF due to extensions realloc

[Jamal Hadi Salim]

On Mon, Sep 21, 2026 at 10:57 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> While calling the helpers, a raw pointer to the extensions area is
> wired into expectations list:
>
>   -> nf_ct_helper()
>    -> helper->help()
>     -> nf_ct_expect_related_report()
>      -> nf_ct_expect_insert()
>       -> hlist_add_head_rcu(&exp->lnode, &master_help->expectations)
>
> In case the connection is not confirmed yet, more extensions can be
> added afterwards with *_ext_add() calls reallocating the extension
> space and leaving the now invalid pointer in the expectations list
> that is later accessed while removing the expectation.
>
> Make sure that helpers are called at the end after all the other
> extensions are already added.
>
> Note that the helper rejection now leaves the mark and labels set,
> but that's not different from how the NAT was handled before or how
> the mark and the labels were handled on confirmation failure.  And
> there are no atomicity guarantees provided by the API anyway.
>
> Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> Cc: stable@vger.kernel.org
> Reported-by: Axel Mierczuk <axel.mierczuk@1password.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

> ---
>  net/sched/act_ct.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index f62051ec9d57d..411e3dd92d072 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1102,19 +1102,25 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>                 }
>         }
>
> -       if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
> -               err = nf_ct_helper(skb, ct, ctinfo, family);
> -               if (err != NF_ACCEPT)
> -                       goto nf_error;
> -       }
> -
>         if (commit) {
>                 tcf_ct_act_set_mark(ct, p->mark, p->mark_mask);
>                 tcf_ct_act_set_labels(ct, p->labels, p->labels_mask);
>
>                 if (!nf_ct_is_confirmed(ct))
>                         nf_conn_act_ct_ext_add(skb, ct, ctinfo);
> +       }
>
> +       /* Run helpers for the connection if nf_conntrack_in() was executed
> +        * or if we're about to commit.  This has to be done after all the
> +        * extensions are already added.
> +        */
> +       if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
> +               err = nf_ct_helper(skb, ct, ctinfo, family);
> +               if (err != NF_ACCEPT)
> +                       goto nf_error;
> +       }
> +
> +       if (commit) {
>                 /* This will take care of sending queued events
>                  * even if the connection is already confirmed.
>                  */
> --
> 2.55.0
>