[net,07/11] ipvs: filter some flags received in the backup server

From: Julian Anastasov <ja@ssi.bg>

While the IPVS SYNC protocol is not secure by design
we can still protect the backup server from messages that
can wreak havoc.

This commit addresses problems from received connection flags
or their combinations. We now drop messages as follows:

1. the NO_CPORT+TEMPLATE combination allows lookups for normal
connections to hit template which can break in many ways.
While the master does not sync connections with NO_CPORT flag,
i.e. before they are established, we still accept NO_CPORT
without TEMPLATE.

2. ONE_PACKET: it is not sent by master, so we do not
expect it in backup. Before now it was ignored by
IP_VS_CONN_F_BACKUP_MASK for protocol v1 while protocol
v0 created connections that are not hashed and dropped
immediately. Better to apply the IP_VS_CONN_F_BACKUP_MASK
also to the flags from v0 messages for consistency with v1.

Fixes: 87375ab47cd0 ("[IPVS]: ip_vs_ftp breaks connections using persistence")
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipvs/ip_vs_sync.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

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 patch does not stop forged sync messages from creating
  templates that normal-connection lookups can hit.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  ip_vs_sync_validate_flags() still accepts NO_CPORT on non-template…

--

ipvs: filter some flags received in the backup server

This commit adds ip_vs_sync_validate_flags() so the backup drops sync
entries that have both NO_CPORT and TEMPLATE set, or that have ONE_PACKET
set. It also applies IP_VS_CONN_F_BACKUP_MASK to v0 message flags, to
match the existing v1 handling.

> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index dfa8487ec0c20..1a30817fbbafe 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -954,6 +954,21 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
>  	ip_vs_conn_put(cp);
>  }
>  
> +/* Check for incompatible flags */
> +static bool ip_vs_sync_validate_flags(u32 flags)
> +{
> +	/* We do not expect NO_CPORT, especially to allow lookups
> +	 * to hit templates
> +	 */
> +	if (flags & IP_VS_CONN_F_NO_CPORT) {
> +		if (flags & IP_VS_CONN_F_TEMPLATE)
> +			return false;
> +	}

[Severity: High]
Does this fully stop sync messages from creating templates that
normal-connection lookups can hit?

Take a v0 or v1 entry with TEMPLATE set, NO_CPORT clear and a non-zero
cport. It passes this check and survives IP_VS_CONN_F_BACKUP_MASK. Then
ip_vs_proc_conn()->ip_vs_conn_new() creates it with the cport from the
message: ip_vs_conn_fill_param_sync() for v1, s->cport for v0.

With no PE data, the template is hashed on the same (proto, caddr, cport,
vaddr, vport) key a normal connection uses. __ip_vs_conn_in_get() does
not check IP_VS_CONN_F_TEMPLATE:

net/netfilter/ipvs/ip_vs_conn.c:__ip_vs_conn_in_get() {
    ...
				if (p->cport == cp->cport &&
				    p->vport == cp->vport && cp->af == p->af &&
				    ...
				    (!p->cport ^
				     (!(cp->flags & IP_VS_CONN_F_NO_CPORT))) &&
				    p->protocol == cp->protocol) {
    ...
}

When p->cport == cp->cport != 0 and NO_CPORT is clear, the XOR term is 1,
so the lookup returns the template.

A later non-template sync for that tuple then gets the template back from
ip_vs_conn_in_get() in ip_vs_proc_conn(). It either expires the template,
or it toggles dest->activeconns on the template's dest and overwrites
cp->flags, state, timeout and seq:

net/netfilter/ipvs/ip_vs_sync.c:ip_vs_proc_conn() {
    ...
		if ((cp->flags ^ flags) & IP_VS_CONN_F_INACTIVE &&
		    !(flags & IP_VS_CONN_F_TEMPLATE) && dest) {
			if (flags & IP_VS_CONN_F_INACTIVE)
				atomic_dec(&dest->activeconns);
			else
				atomic_inc(&dest->activeconns);
		}
    ...
}

For templates, ip_vs_unbind_dest() only decrements persistconns, so the
activeconns change is never undone. After failover, real client packets
for that tuple would also be handled by the template in the ip_vs_in()
path.

The master always builds templates with cport 0. Both
ip_vs_conn_fill_param_persist() callers in ip_vs_core.c pass a literal 0.

Should the backup also reject TEMPLATE entries with a non-zero cport when
there is no PE data?

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. NO_CPORT
without TEMPLATE is still accepted here, and nothing checks that the
synced cport is 0. Is that intended?

Other code assumes NO_CPORT means cport == 0. ip_vs_ftp creates NO_CPORT
connections with cport 0, and ip_vs_conn_fill_cport() uses !cp->cport to
mean the cport has not been filled in yet:

	if (cp->flags & IP_VS_CONN_F_NO_CPORT && (!cp->cport || by_me)) {

ip_vs_conn_new() creates a NO_CPORT entry with cport X != 0 and
increments ipvs->no_cport_conns[af]. ip_vs_conn_in_get() can never find
that entry. The exact lookup fails the XOR term in __ip_vs_conn_in_get(),
and the cport_zero_p retry fails the cport comparison.

So each refresh in ip_vs_proc_conn() would create another duplicate entry,
and each one lasts until its timeout. no_cport_conns also stays non-zero,
which forces the second lookup on every in-direction miss. The entry can
still be matched in the reply direction, because ip_vs_conn_out_get()
does not check NO_CPORT.

Would it make sense to also require cport == 0 when NO_CPORT is accepted
on a non-template entry?

> +	if (flags & IP_VS_CONN_F_ONE_PACKET)
> +		return false;
> +	return true;
> +}
> +

[ ... ]

Hello,

On Mon, 28 Sep 2026, netdev-bot+sashiko@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

	This patch is fine, see below. It is another patch
by another author that adds the missing checks.

> 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 patch does not stop forged sync messages from creating
>   templates that normal-connection lookups can hit.
> 
> Pre-existing issues:
> - [Low] This problem wasn't introduced by this patch, but
>   ip_vs_sync_validate_flags() still accepts NO_CPORT on non-template…
> 
> --
> 
> ipvs: filter some flags received in the backup server
> 
> This commit adds ip_vs_sync_validate_flags() so the backup drops sync
> entries that have both NO_CPORT and TEMPLATE set, or that have ONE_PACKET
> set. It also applies IP_VS_CONN_F_BACKUP_MASK to v0 message flags, to
> match the existing v1 handling.
> 
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > index dfa8487ec0c20..1a30817fbbafe 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -954,6 +954,21 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
> >  	ip_vs_conn_put(cp);
> >  }
> >  
> > +/* Check for incompatible flags */
> > +static bool ip_vs_sync_validate_flags(u32 flags)
> > +{
> > +	/* We do not expect NO_CPORT, especially to allow lookups
> > +	 * to hit templates
> > +	 */
> > +	if (flags & IP_VS_CONN_F_NO_CPORT) {
> > +		if (flags & IP_VS_CONN_F_TEMPLATE)
> > +			return false;
> > +	}
> 
> [Severity: High]
> Does this fully stop sync messages from creating templates that
> normal-connection lookups can hit?
> 
> Take a v0 or v1 entry with TEMPLATE set, NO_CPORT clear and a non-zero
> cport. It passes this check and survives IP_VS_CONN_F_BACKUP_MASK. Then
> ip_vs_proc_conn()->ip_vs_conn_new() creates it with the cport from the
> message: ip_vs_conn_fill_param_sync() for v1, s->cport for v0.
> 
> With no PE data, the template is hashed on the same (proto, caddr, cport,
> vaddr, vport) key a normal connection uses. __ip_vs_conn_in_get() does
> not check IP_VS_CONN_F_TEMPLATE:
> 
> net/netfilter/ipvs/ip_vs_conn.c:__ip_vs_conn_in_get() {
>     ...
> 				if (p->cport == cp->cport &&
> 				    p->vport == cp->vport && cp->af == p->af &&
> 				    ...
> 				    (!p->cport ^
> 				     (!(cp->flags & IP_VS_CONN_F_NO_CPORT))) &&
> 				    p->protocol == cp->protocol) {
>     ...
> }
> 
> When p->cport == cp->cport != 0 and NO_CPORT is clear, the XOR term is 1,
> so the lookup returns the template.
> 
> A later non-template sync for that tuple then gets the template back from
> ip_vs_conn_in_get() in ip_vs_proc_conn(). It either expires the template,
> or it toggles dest->activeconns on the template's dest and overwrites
> cp->flags, state, timeout and seq:
> 
> net/netfilter/ipvs/ip_vs_sync.c:ip_vs_proc_conn() {
>     ...
> 		if ((cp->flags ^ flags) & IP_VS_CONN_F_INACTIVE &&
> 		    !(flags & IP_VS_CONN_F_TEMPLATE) && dest) {
> 			if (flags & IP_VS_CONN_F_INACTIVE)
> 				atomic_dec(&dest->activeconns);
> 			else
> 				atomic_inc(&dest->activeconns);
> 		}
>     ...
> }
> 
> For templates, ip_vs_unbind_dest() only decrements persistconns, so the
> activeconns change is never undone. After failover, real client packets
> for that tuple would also be handled by the template in the ip_vs_in()
> path.
> 
> The master always builds templates with cport 0. Both
> ip_vs_conn_fill_param_persist() callers in ip_vs_core.c pass a literal 0.
> 
> Should the backup also reject TEMPLATE entries with a non-zero cport when
> there is no PE data?

	Above is solved with a pending patch
"ipvs: validate cport in received sync records".

> 
> [Severity: Low]
> This is a pre-existing issue, not one introduced by this patch. NO_CPORT
> without TEMPLATE is still accepted here, and nothing checks that the
> synced cport is 0. Is that intended?
> 
> Other code assumes NO_CPORT means cport == 0. ip_vs_ftp creates NO_CPORT
> connections with cport 0, and ip_vs_conn_fill_cport() uses !cp->cport to
> mean the cport has not been filled in yet:
> 
> 	if (cp->flags & IP_VS_CONN_F_NO_CPORT && (!cp->cport || by_me)) {
> 
> ip_vs_conn_new() creates a NO_CPORT entry with cport X != 0 and
> increments ipvs->no_cport_conns[af]. ip_vs_conn_in_get() can never find
> that entry. The exact lookup fails the XOR term in __ip_vs_conn_in_get(),
> and the cport_zero_p retry fails the cport comparison.
> 
> So each refresh in ip_vs_proc_conn() would create another duplicate entry,
> and each one lasts until its timeout. no_cport_conns also stays non-zero,
> which forces the second lookup on every in-direction miss. The entry can
> still be matched in the reply direction, because ip_vs_conn_out_get()
> does not check NO_CPORT.
> 
> Would it make sense to also require cport == 0 when NO_CPORT is accepted
> on a non-template entry?

	Above is solved with a pending patch
"ipvs: validate cport in received sync records".

> 
> > +	if (flags & IP_VS_CONN_F_ONE_PACKET)
> > +		return false;
> > +	return true;
> > +}
> > +
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927220816.268206-2-pablo%40netfilter.org
> 

Regards

--
Julian Anastasov <ja@ssi.bg>