From 26b2bd70d22457556e2fa01cbf1192cb1a94d619 Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Mon, 21 Sep 2026 16:55:43 +0200 Subject: [PATCH 1/6] net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry 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 Signed-off-by: Ilya Maximets Reviewed-by: Aaron Conole Link: https://patch.msgid.link/20260921145655.3167436-2-i.maximets@ovn.org Signed-off-by: Jakub Kicinski --- 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 bc42dd0e10e6..c39425e54d87 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 0f433688e17b..a733029c28dd 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, From 5e6c14dd42a1c1fe938e573dc6c9098145b2b0c4 Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Mon, 21 Sep 2026 16:55:44 +0200 Subject: [PATCH 2/6] net: openvswitch: conntrack: remove 'add_helper' dead code 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 Reviewed-by: Aaron Conole Link: https://patch.msgid.link/20260921145655.3167436-3-i.maximets@ovn.org Signed-off-by: Jakub Kicinski --- 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 a733029c28dd..c20f096eef40 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); From 1a4151e6be57b098b7a5ebfbde58585e83200cdc Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Mon, 21 Sep 2026 16:55:45 +0200 Subject: [PATCH 3/6] net: openvswitch: conntrack: fix helper UAF due to extensions realloc 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 Signed-off-by: Ilya Maximets Reviewed-by: Aaron Conole Link: https://patch.msgid.link/20260921145655.3167436-4-i.maximets@ovn.org Signed-off-by: Jakub Kicinski --- 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 c20f096eef40..d3326edcabf7 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, From f85009dfcd65e5969526b0db7a49b5413746e630 Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Mon, 21 Sep 2026 16:55:46 +0200 Subject: [PATCH 4/6] net/sched: act_ct: avoid modifying shared unconfirmed ct entry 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 Signed-off-by: Ilya Maximets Reviewed-by: Aaron Conole Reviewed-by: Xin Long Reviewed-by: Jamal Hadi Salim Link: https://patch.msgid.link/20260921145655.3167436-5-i.maximets@ovn.org Signed-off-by: Jakub Kicinski --- 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 55f3521edb4c..e72143d36b11 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 @@ do_nat: 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; } From 00df72e39f306e2f7adb68528a5c92109da6a0a9 Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Mon, 21 Sep 2026 16:55:47 +0200 Subject: [PATCH 5/6] net/sched: act_ct: remove 'add_helper' dead code 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 Reviewed-by: Aaron Conole Reviewed-by: Xin Long Reviewed-by: Jamal Hadi Salim Link: https://patch.msgid.link/20260921145655.3167436-6-i.maximets@ovn.org Signed-off-by: Jakub Kicinski --- 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 e72143d36b11..f62051ec9d57 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 @@ do_nat: 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; From dad19b59da050cb60d3f7023dac2a042a84bf0bd Mon Sep 17 00:00:00 2001 From: Ilya Maximets Date: Mon, 21 Sep 2026 16:55:48 +0200 Subject: [PATCH 6/6] net/sched: act_ct: fix helper UAF due to extensions realloc 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 Signed-off-by: Ilya Maximets Reviewed-by: Xin Long Reviewed-by: Jamal Hadi Salim Reviewed-by: Aaron Conole Link: https://patch.msgid.link/20260921145655.3167436-7-i.maximets@ovn.org Signed-off-by: Jakub Kicinski --- 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 f62051ec9d57..411e3dd92d07 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -1102,6 +1102,18 @@ do_nat: } } + 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) @@ -1109,12 +1121,6 @@ do_nat: } 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); - /* This will take care of sending queued events * even if the connection is already confirmed. */