Re: [PATCH net 02/11] ipvs: fix buffer overflow when sending sync messages

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

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

- [Medium] The commit message for "ipvs: fix buffer overflow when sending
  sync messages" says the SEQ_MASK race "should not be reproducible",…

Pre-existing issues:
- [Medium] After the patch, the flags are a consistent snapshot, but the
  sequence data is still read without locking and without ordering…

--

ipvs: fix buffer overflow when sending sync messages

This commit changes ip_vs_sync_conn_v0() and ip_vs_sync_conn() to read
cp->flags once with READ_ONCE(). The same value is then used to size the
sync entry and to decide whether to emit the sequence options. This
prevents a heap overflow when another CPU sets IP_VS_CONN_F_SEQ_MASK
between the two reads.

Link: https://sashiko.dev/#/patchset/20260903004149.1037028-1-pablo%40netfilter.org
Link: https://sashiko.dev/#/patchset/20260909111338.44357-1-ja%40ssi.bg

Note that IPVS does not set the SEQ_MASK anymore for the
ip_vs_ftp.c helper starting from commit 7f1c40757951
("IPVS: make FTP work with full NAT support") (2.6.36+),
so the problem should not be reproducible.

[Severity: Medium]
Is the "should not be reproducible" part accurate?

The ip_vs_ftp part is true. ip_vs_ftp_out() and ip_vs_ftp_in() always set
*diff = 0, so vs_seq_update() is never reached. However, the SEQ bits
still seem to have another writer, and received sync messages drive it.

In ip_vs_proc_conn(), an existing connection has its flags updated under
cp->lock:

net/netfilter/ipvs/ip_vs_sync.c:ip_vs_proc_conn() {
    ...
		flags &= IP_VS_CONN_F_BACKUP_UPD_MASK;
		flags |= cp->flags & ~IP_VS_CONN_F_BACKUP_UPD_MASK;
		cp->flags = flags;
    ...
}

IP_VS_CONN_F_BACKUP_UPD_MASK includes the SEQ bits:

#define IP_VS_CONN_F_BACKUP_UPD_MASK (IP_VS_CONN_F_INACTIVE | \
				      IP_VS_CONN_F_SEQ_MASK)

ip_vs_proc_sync_conn() masks the wire flags with IP_VS_CONN_F_BACKUP_MASK,
which also keeps the SEQ bits. It does not require an IPVS_OPT_SEQ_DATA
option to be present. So a peer's sync message can set or clear the SEQ
bits on an existing connection.

start_sync_thread() checks the master and backup roles separately and then
does ipvs->sync_state |= state, so one node can run both daemons. On such
a node:

CPU1 (backup kthread)
ip_vs_process_message()
  ip_vs_proc_sync_conn()
    ip_vs_proc_conn()
      cp->flags = flags;    /* SEQ bits toggled */

CPU2 (packet path, or ip_vs_conn_expire())
ip_vs_in()
  ip_vs_sync_conn()
    reads cp->flags to compute len, then again for IPVS_OPT_SEQ_DATA

Before this patch, that double read could size len without the options.
It could then write struct ip_vs_sync_conn_options past buff->end.

Sync traffic is unauthenticated; a later patch in this series calls it
not secure by design. Also, "ipvs: filter some flags received in the
backup server" does not filter the SEQ bits, so this writer is still
live at the end of the series.

Could the commit message drop the "should not be reproducible" claim and
mention the backup update path? The code change itself does look like it
closes the overflow.

[ ... ]

[ ... ]

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The flags
are now a consistent snapshot, but the sequence data is still read
without cp->lock. Nothing orders that read against READ_ONCE(cp->flags).
Can the master send stale or half-updated ip_vs_seq values here?

ip_vs_sync_conn() here only holds ipvs->sync_buff_lock. The same applies
to the memcpy() of cp->sync_conn_opt in ip_vs_sync_conn_v0().

On the backup side, ip_vs_proc_conn() sets the SEQ bits in cp->flags under
cp->lock and drops the lock. Only after that does it copy the sequence
data, with no lock held:

net/netfilter/ipvs/ip_vs_sync.c:ip_vs_proc_conn() {
    ...
		cp->flags = flags;
		spin_unlock_bh(&cp->lock);
    ...
	if (opt) {
		cp->in_seq = opt->in_seq;
		cp->out_seq = opt->out_seq;
	}
    ...
}

On a node running both the master and backup daemons, ip_vs_sync_conn()
could see seq_mask set and then send old or torn init_seq, delta and
previous_delta values. A peer with an app bound to that connection would
then apply the wrong TCP seq/ack adjustment after failover.

vs_seq_update() in ip_vs_app.c is another writer. It updates the sequence
data and flags under cp->lock, but the sync reader never takes that lock.
In-tree it is only reached with diff != 0, which ip_vs_ftp never
produces.

This does not corrupt memory, because the buffer size and the option
emission now both come from the same snapshot.

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