Re: [PATCH net 07/15] ipvs: ftp: add seqadj extension before linking the expectation

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The fix is incomplete.

--

ipvs: ftp: add seqadj extension before linking the expectation

This patch adds the seqadj extension to an unconfirmed conntrack in
ip_vs_ftp_out() before the payload is mangled and before the expectation
is linked. The goal is to keep ip_vs_update_conntrack() from reallocating
ct->ext while the helper's expectation list still points into it.

> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
> index 44de99f4d187f..81c3a2fd27b39 100644
> --- a/net/netfilter/ipvs/ip_vs_ftp.c
> +++ b/net/netfilter/ipvs/ip_vs_ftp.c

[ ... ]

> @@ -384,19 +385,25 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp,
>
>  	ct = nf_ct_get(skb, &ctinfo);
>  	if (ct && !nf_ct_is_template(ct)) {
> -		bool mangled;
> +		bool mangled = false;
>
>  		/* If mangling fails this function will return 0
>  		 * which will cause the packet to be dropped.
>  		 * Mangling can only fail under memory pressure,
>  		 * hopefully it will succeed on the retransmitted
>  		 * packet.
> +		 *
> +		 * Add the seqadj extension before the expectation is
> +		 * linked into the helper extension: adding it later
> +		 * reallocates ct->ext under the expectation list.
>  		 */
> -		mangled = nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
> -						   ipvsh->len,
> -						   start - data,
> -						   end - start,
> -						   buf, buf_len);
> +		if (nf_ct_is_confirmed(ct) || nfct_seqadj(ct) ||
> +		    nfct_seqadj_ext_add(ct))
> +			mangled = nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
> +							   ipvsh->len,
> +							   start - data,
> +							   end - start,
> +							   buf, buf_len);
>  		if (mangled) {
>  			ip_vs_nfct_expect_related(skb, ct, n_cp,
>  						  ipvsh->protocol, 0, 0);

[Severity: High]
Does this fully close the use-after-free? The expectation is still linked
into nfct_help(ct)->expectations while ct is unconfirmed, and that list
head lives inside ct->ext. Pre-adding seqadj only stops one later
extension add from moving ct->ext.

nf_ct_expect_insert() does:

    hlist_add_head_rcu(&exp->lnode, &master_help->expectations);

so exp->lnode.pprev now points into the current ct->ext block.

Later hooks on the same packet can still grow ct->ext. One example is a
MASQUERADE rule in POSTROUTING, which runs for this IP_CT_NEW conntrack
after the IPVS out hook:

  nf_nat_inet_fn()
    nf_nat_masquerade_ipv4()
      nf_ct_nat_ext_add()
        nf_ct_ext_add(ct, NF_CT_EXT_NAT, GFP_ATOMIC)
          krealloc(ct->ext, alloc, gfp)

Here is one layout where this moves the block. With
CONFIG_NF_CONNTRACK_TIMESTAMP=y and an ecache extension present
(nf_conntrack_events=1), the extensions are header 16, ecache 32, help
56 and seqadj 24. That adds up to exactly 128 bytes, which is the
NF_CT_EXT_PREALLOC object size. Adding NAT gives a newlen of 132-136.
__do_krealloc() then goes to alloc_new and the old block is freed.
nf_conntrack_acct=1 gives the same arithmetic.

After that, ext->help.expectations.first in the new block points to
exp->lnode. But exp->lnode.pprev still points into the freed block. When
the expectation or conntrack is removed, nf_ct_unlink_expect_report()
calls:

    hlist_del_rcu(&exp->lnode);

This writes through the stale pprev, which looks like the same KASAN
report quoted in the commit message. In this layout the seqadj add would
not have moved the block even before the patch, so the patch does not
help in this case.

Other extension adds on an unconfirmed ct are also reachable later in
the same traversal:

  nft_ct_set_eval() with NFT_CT_EVENTMASK -> nf_ct_ecache_ext_add()
  nft_ct_timeout_obj_eval() -> nf_ct_timeout_ext_add()

The commit message says this is reachable with CAP_NET_ADMIN in a user
namespace. That context can configure MASQUERADE, nft ct rules, the acct
sysctl and ctnetlink listeners.

Would it be better to not link the expectation while the conntrack's ext
can still grow? Two options are deferring the link until confirmation,
or making ct->ext stable first. Pre-adding one more extension does not
seem to cover these other paths.

None of the later patches in the series appear to touch ip_vs_ftp.c,
ip_vs_nfct.c, nf_conntrack_extend.c, nf_nat_core.c, nf_nat_masquerade.c
or nf_conntrack_expect.c.

>  			if (skb->ip_summed == CHECKSUM_COMPLETE)
>  				skb->ip_summed = CHECKSUM_UNNECESSARY;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org