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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Sep 29, 2026 at 11:55:11AM +0200, Paolo Abeni wrote:
> On 9/29/26 11:43, Pablo Neira Ayuso wrote:
> > Hi Paolo,
> > 
> > On Tue, Sep 29, 2026 at 10:19:55AM +0200, Paolo Abeni wrote:
> > > On 9/29/26 06:06, Julian Anastasov wrote:
> > > > > > @@ -744,7 +749,7 @@ void ip_vs_sync_conn(struct netns_ipvs *ipvs, struct ip_vs_conn *cp, int pkts)
> > > > > >    		s->v4.vaddr = cp->vaddr.ip;
> > > > > >    		s->v4.daddr = cp->daddr.ip;
> > > > > >    	}
> > > > > > -	if (cp->flags & IP_VS_CONN_F_SEQ_MASK) {
> > > > > > +	if (seq_mask) {
> > > > > >    		*(p++) = IPVS_OPT_SEQ_DATA;
> > > > > >    		*(p++) = sizeof(struct ip_vs_sync_conn_options);
> > > > > >    		hton_seq(&cp->in_seq, (struct ip_vs_seq *)p);
> > > > > 
> > > > > [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.
> > > > 
> > > > 	Agreed, this can be improved to take a snapshot of
> > > > flags and seqs together under lock. I'll request to drop
> > > > this version, it is not urgent to apply it.
> > > 
> > > It looks like a respin of the PR is needed, I'll drop the revision from PW.
> > 
> > Just wrote to Jakub with a summary on the LLM report.
> > 
> > I would take this PR as is if it is still possible.
> > 
> > If you feel strong about to need to respin this PR, that's also fine
> > with me, just let confirm where to go.
>
> Uhm... I must admit that given the pressure we received from Linus on
> shrinking/prevent from increasing the net PR size I think we are better
> off dropping patch 2/11 entirely.
> 
> I'm sorry for the extra work, but the messaging from Linus was pretty
> strong:
> 
> https://lore.kernel.org/netdev/CAHk-=wiSnTE9vBZ=5_v+3EEkdazCCbBM5YABRzRHUAeRdyd4Xw@mail.gmail.com/

Yes, I read that. I tried to move what is less relevant to nf-next (if
you look at my nf-next PR, it's contains not so relevant fixes too).

 include/linux/netdevice.h               |  3 ++
 include/net/netfilter/nf_flow_table.h   |  2 +-
 net/mac80211/iface.c                    |  7 +++++
 net/netfilter/ipset/ip_set_bitmap_gen.h |  2 +-
 net/netfilter/ipvs/ip_vs_conn.c         |  3 ++
 net/netfilter/ipvs/ip_vs_lblc.c         |  4 +++
 net/netfilter/ipvs/ip_vs_lblcr.c        |  3 ++
 net/netfilter/ipvs/ip_vs_sync.c         | 56 ++++++++++++++++++++++++++-------
 net/netfilter/nf_flow_table_core.c      |  7 ++++-
 net/netfilter/nf_flow_table_offload.c   | 14 +++------
 net/netfilter/nf_flow_table_path.c      |  3 ++
 net/netfilter/nf_nat_bpf.c              |  3 ++
 net/netfilter/nf_nat_core.c             |  5 +--
 net/netfilter/nft_flow_offload.c        |  7 ++++-
 net/netfilter/nft_set_rbtree.c          |  2 ++
 net/sched/act_ct.c                      |  2 +-
 16 files changed, 95 insertions(+), 28 deletions(-)

Yes, this patch is larger in the diffstat. I can just move it to
nf-next if you prefer.

I will respin and send v2 today without this.

Thanks.