Re: [PATCH net 3/6] net: openvswitch: conntrack: 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] Inaccurate newly added comment in __ovs_ct_lookup()
  (net/openvswitch/conntrack.c:820-826).

Pre-existing issues:
- [Low] Concern (INVALIDATED by later patch in the same series): the
  identical helper/expectation use-after-free was claimed to remain…

--

net: openvswitch: conntrack: fix helper UAF due to extensions realloc

This moves the nf_ct_helper() call for unconfirmed connections out of
__ovs_ct_lookup() and into ovs_ct_commit(), after the mark, labels and
act_ct extensions have been added, so that a later extension realloc
cannot invalidate the expectation list pointer wired up by the helper.

No Link: tags are present in the commit header.

> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c20f096eef40e..d3326edcabf76 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) {

[Severity: Low]
Could this comment be made to match the condition below it?  The text says
the helper is called only if nf_conntrack_in() ran above ("!cached"), but
the guard is:

	if (nf_ct_is_confirmed(ct) && !cached) {

The nf_ct_is_confirmed() term isn't mentioned, which can read as if the
confirmed check were redundant.

The second half of the comment also promises that for unconfirmed
connections the helper "will be called later during commit".  Isn't that
only true for the committing path?  __ovs_ct_lookup() is also reached from
ovs_ct_lookup(), where info->commit is 0 and there is no nf_ct_helper()
call anywhere in that action:

net/openvswitch/conntrack.c:ovs_ct_lookup() {
	...
	err = __ovs_ct_lookup(net, key, info, skb);
	...
}

So for an unconfirmed entry created by nf_conntrack_in() during a plain
ct() lookup action, the helper is simply never run and there is no later
commit to run it.  Behaviour here is the same as before the patch (the old
nf_ct_is_confirmed(ct) ? !cached : info->commit guard skipped it too), so
this is only about the wording of the new comment.

>  			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;

[Severity: Low]
This isn't a bug introduced by this patch, but the same ordering problem
exists in the other in-tree caller of nf_ct_helper(), tcf_ct_act() in
net/sched/act_ct.c, which at this commit runs the helper before the
extension add for unconfirmed entries:

net/sched/act_ct.c:tcf_ct_act() {
	...
	if (nf_ct_is_confirmed(ct) ? (!cached && !skip_add) : commit) {
		err = nf_ct_helper(skb, ct, ctinfo, family);
	...
	if (commit) {
		tcf_ct_act_set_mark(...);
		tcf_ct_act_set_labels(...);
		if (!nf_ct_is_confirmed(ct))
			nf_conn_act_ct_ext_add(skb, ct, ctinfo);
	...
}

nf_conn_act_ct_ext_add() -> nf_ct_ext_add() -> krealloc() can move ct->ext,
leaving exp->lnode.pprev (which points into nf_conn_help inside the old
ext blob, stored by nf_ct_expect_insert() via
hlist_add_head_rcu(&exp->lnode, &master_help->expectations)) dangling.

Note this is already handled by the later patch in the same series,
"net/sched: act_ct: fix helper UAF due to extensions realloc", which moves
the tcf_ct_act() helper call below the nf_conn_act_ct_ext_add() block, so
no extension-adding call remains after the helper in either caller by the
end of the series.  Mentioned here only for completeness.

>  	} else if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
>  		   labels_nonzero(&info->labels.mask)) {
>  		err = ovs_ct_set_labels(ct, key, &info->labels.value,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org