On Mon, Sep 21, 2026 at 10:57 AM Ilya Maximets <[email protected]> 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: [email protected] > Reported-by: Axel Mierczuk <[email protected]> > Signed-off-by: Ilya Maximets <[email protected]>
Reviewed-by: Jamal Hadi Salim <[email protected]> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
