Re: [PATCH net 1/2] ovpn: tcp - use cached peer pointer in ovpn_tcp_close()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-05-12, 16:17:39 +0200, Antonio Quartulli wrote:
> On 12/05/2026 16:11, Sabrina Dubroca wrote:
> > 2026-05-12, 15:55:39 +0200, Antonio Quartulli wrote:
> > > Hi,
> > > 
> > > On 12/05/2026 06:56, David CARLIER wrote:
> > > >     Same multi-read pattern shows up in ovpn_tcp_recvmsg(),
> > > >     ovpn_tcp_sendmsg(), ovpn_tcp_data_ready() and ovpn_tcp_write_space()
> > > >     - happy to roll those into v2 as well, or punt to a follow-up,
> > > >     whichever you'd prefer.
> > > 
> > > @Eric, if you have no objection, I'd pick this patch up in my tree and let
> > > David follow with a new patch for net-next.
> > 
> > But this patch is not fixing any problem either, right?
> 
> Mh, because the sock outlives the peer, so there is no risk in accessing
> sock->peer in this case, right?

I guess I got distracted by some of the discussion. I thought this was
only about "peer and sock->peer may differ", and not "sock may be gone
so sock->peer is not valid".

sock->peer can't change behind our backs, because, as David said:

    sock->peer is only assigned once, in ovpn_socket_new()


But the sock doesn't outlive the peer. ovpn_socket_release() does
ovpn_peer_put(), and frees the ovpn_socket immediately via
kfree(sock). So if:

   ovpn_tcp_close() starts, finds sk_user_data set and a peer, does peer_hold
   the peer gets deleted in parallel, ovpn_socket_release() frees the ovpn_socket
   ovpn_tcp_close() resumes and does ovpn_peer_del(sock->peer)

we can indeed hit a UAF on sock.


So this patch is needed as-is, sorry for my confusion earlier:

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>


The refactoring of all those peer accesses can be done in -next.

-- 
Sabrina