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

[netdev-bot+sashiko@kernel.org]

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