Re: [PATCH net 11/15] ipvs: validate cport in received sync records

[netdev-bot+sashiko@kernel.org]

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] This patch has Cc: stable@vger.kernel.org, but on its own it does
  not stop synced templates from being matched by ip_vs_conn_in_get().
- [Low] The new backup check in ip_vs_proc_conn() drops every non-
  template record with a nonzero cport and IP_VS_CONN_F_NO_CPORT set.

--

ipvs: validate cport in received sync records

This commit adds checks to ip_vs_proc_conn() that drop received sync
records whose cport and IP_VS_CONN_F_NO_CPORT combination is
inconsistent. Three cases are rejected: templates with a nonzero
cport, ordinary connections with cport 0 and NO_CPORT clear, and
ordinary connections with a nonzero cport and NO_CPORT set.

> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index f11c59eca5536..7e562e745ab5d 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -871,6 +871,25 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
>  	struct ip_vs_dest *dest;
>  	struct ip_vs_conn *cp;
>  
> +	/* Templates require cport 0. Ordinary connections require
> +	 * NO_CPORT exactly when cport is 0.
> +	 */
> +	if (flags & IP_VS_CONN_F_TEMPLATE) {
> +		if (param->cport) {
> +			IP_VS_DBG(2, "BACKUP, template with cport dropped\n");
> +			kfree(param->pe_data);
> +			return;
> +		}

[Severity: Low]

This patch carries Cc: stable, but for templates it only rejects a
nonzero cport. A template with cport 0 and IP_VS_CONN_F_NO_CPORT set
still passes this check.

That combination is rejected only by ip_vs_sync_validate_flags(), which
is added by f96e91748f63 ("ipvs: filter some flags received in the
backup server"):

    if (flags & IP_VS_CONN_F_NO_CPORT) {
        if (flags & IP_VS_CONN_F_TEMPLATE)
            return false;
    }

That commit has a Fixes: tag but no Cc: stable.

Suppose a stable tree picks up only this patch. Can an unauthenticated
sync message then still install a template with TEMPLATE | NO_CPORT
and cport 0? ip_vs_conn_new() would raise no_cport_conns. The cport-zero
fallback in ip_vs_conn_in_get() would then match that template for any
client source port:

    if (atomic_read(&ipvs->no_cport_conns[af_id])) {
        struct ip_vs_conn_param cport_zero_p = *p;

        cport_zero_p.cport = 0;
        cp = __ip_vs_conn_in_get(&cport_zero_p);
    }

This happens because !p->cport ^ !(cp->flags & IP_VS_CONN_F_NO_CPORT)
evaluates to true in __ip_vs_conn_in_get().

Mainline is not affected, because f96e91748f63 is already an ancestor.
In stable, the out-of-bounds part is covered by 74cb39735b6c ("ipvs:
reject invalid states in connection template sync records"). The two
commits share the same Fixes: target, so stable tooling may pick up
f96e91748f63 anyway.

The commit message calls f96e91748f63 a "companion commit". Would it
help to state the backport dependency explicitly, for example:

    Cc: stable@vger.kernel.org # depends on f96e91748f63

> +	} else if (!param->cport && !(flags & IP_VS_CONN_F_NO_CPORT)) {
> +		IP_VS_DBG(2, "BACKUP, conn without cport dropped\n");
> +		kfree(param->pe_data);
> +		return;
> +	} else if (param->cport && (flags & IP_VS_CONN_F_NO_CPORT)) {
> +		IP_VS_DBG(2, "BACKUP, conn with cport and NO_CPORT dropped\n");
> +		kfree(param->pe_data);
> +		return;
> +	}

[Severity: Low]

Can a legitimate in-tree master send a nonzero cport with NO_CPORT
still set for an ordinary connection? The new comment says ordinary
connections require NO_CPORT exactly when cport is 0. However,
ip_vs_conn_fill_cport() updates the two fields in separate passes:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_fill_cport() {
    ...
	if (cp->flags & IP_VS_CONN_F_NO_CPORT && (!cp->cport || by_me)) {
		...
		if (!cp->cport) {
			cp->cport = cport;
			by_me = true;
		}
		if (!dir) {
			atomic_dec(&ipvs->no_cport_conns[af_id]);
			cp->flags &= ~IP_VS_CONN_F_NO_CPORT;
		}
    ...
	spin_unlock_bh(&cp->lock);
	if (dir-- && by_me)
		goto next_dir;
}

The dir=1 pass sets cp->cport and releases cp->lock. Only the dir=0
pass clears NO_CPORT.

On the sending side, ip_vs_sync_conn() reads the flags with
READ_ONCE(cp->flags). It takes cp->lock only when
IP_VS_CONN_F_SEQ_MASK is set. Later it copies s->v4.cport = cp->cport
without the lock. ip_vs_sync_conn_v0() copies s->cport = cp->cport the
same way.

Consider a NO_CPORT FTP data connection under MASQ. One CPU can be in
ip_vs_nat_xmit()->ip_vs_conn_fill_cport() between the two passes while
another CPU syncs the connection. The record would then carry a nonzero
cport with NO_CPORT still set, and this new branch would drop it.

The impact looks small. Before this patch, the backup would have
created a connection that incoming lookups could not match anyway, and
later syncs carry consistent flags.

Should the comment and the debug message be adjusted? This state can
come from an in-tree master, so it does not necessarily mean the peer
misbehaved.

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