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

[Paolo Abeni <pabeni@redhat.com>]

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/

Thanks,

Paolo