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

[Ilya Maximets <i.maximets@ovn.org>]

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.