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