[PATCH net 0/2] ovpn: fix TCP teardown UAF races

[PATCH net 0/2] ovpn: fix TCP teardown UAF races

[David Carlier]

Two distinct UAFs in the TCP transport teardown path, both inherited
  from 11851cbd60ea ("ovpn: implement TCP transport"). They share a race
  class with the already-merged 94560267d6c4 ("ovpn: tcp - don't deref
  NULL sk_socket member after tcp_close()"), which fixed the detach-side
  of the keepalive-vs-userspace-close race window; the two patches in
  this series cover the other two victim sites in the same window.

  Patch 1 fixes a UAF in ovpn_tcp_close(). The function loads the
  ovpn_socket via rcu_dereference_sk_user_data(), caches sock->peer in a
  local, drops rcu_read_lock, and then passes sock->peer (rather than
  the cached local) to ovpn_peer_del(). Unlike ovpn_tcp_sendmsg() which
  uses the same pattern but is protected by lock_sock, ovpn_tcp_close()
  runs without the socket lock - inet_release() does not lock_sock
  before calling sk_prot->close. A concurrent ovpn_socket_release() can
  therefore complete kref_put -> detach -> synchronize_rcu -> kfree(sock)
  in the window after rcu_read_unlock() but before the dangling
  sock->peer dereference. The cached peer local already exists, is
  held by ovpn_peer_hold() taken under RCU, and is the correct argument.

  Patch 2 fixes the CMD_NEW_PEER error path in ovpn_nl_peer_new_doit(),
  which calls ovpn_peer_release() directly rather than ovpn_peer_put(),
  bypassing the kref. The accompanying "peer was not yet hashed, thus
  not used in any context" comment is correct for UDP - whose
  ovpn_socket union uses the .ovpn arm and is unreachable from a peer
  pointer - but wrong for TCP. After ovpn_tcp_socket_attach() publishes
  ovpn_sock via rcu_assign_sk_user_data(), the peer is reachable via
  sk_user_data -> ovpn_sock->peer; userspace recvmsg/sendmsg/close/poll
  and the strparser-driven ovpn_tcp_rcv() path can bump its refcount.
  ovpn_tcp_socket_wait_finish() drains strparser and tx work but does
  not synchronize with userspace syscall callers. Use ovpn_peer_put()
  so the kref correctly defers destruction until the last reference
  is dropped.

  Reachability is narrower for patch 2 than patch 1: it requires a
  userspace operation on the TCP fd to be in flight while the netlink
  CMD_NEW_PEER handler hits an error in ovpn_nl_peer_modify() or
  ovpn_peer_add(). A well-behaved openvpn daemon issues CMD_NEW_PEER
  before passing the fd to a recv loop, but the kernel cannot enforce
  that ordering, and the fix is one line.

  Both Fixes: 11851cbd60ea ("ovpn: implement TCP transport"). Not Cc'd
  to stable - borderline practical reachability and reproducer is
  non-trivial; maintainer judgement on backport.


David Carlier (2):
  ovpn: tcp - use cached peer pointer in ovpn_tcp_close()
  ovpn: respect peer refcount in CMD_NEW_PEER error path

 drivers/net/ovpn/netlink.c | 8 +++++---
 drivers/net/ovpn/tcp.c     | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.53.0

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

[David Carlier]

ovpn_tcp_close() loads the ovpn_socket via rcu_dereference_sk_user_data()
under rcu_read_lock(), takes a reference on sock->peer, caches the peer
pointer in a local, and drops the read lock. It then passes sock->peer
(rather than the cached local) to ovpn_peer_del(), re-dereferencing the
ovpn_socket after the RCU read section has ended.

Unlike ovpn_tcp_sendmsg(), which uses the same "load under RCU, use
after unlock" pattern but is protected by lock_sock() held across the
function, ovpn_tcp_close() runs without the socket lock: inet_release()
invokes sk_prot->close() without taking lock_sock first.

ovpn_socket_release() can therefore complete its kref_put -> detach ->
synchronize_rcu -> kfree(sock) sequence concurrently, in the window
after ovpn_tcp_close() drops rcu_read_lock() but before it dereferences
sock->peer. The synchronize_rcu() in ovpn_socket_release() protects
readers that use the dereferenced pointer inside the RCU read section,
not those that escape the pointer to a local and use it afterwards.

A reproducer follows the pattern of commit 94560267d6c4 ("ovpn: tcp -
don't deref NULL sk_socket member after tcp_close()"): trigger a peer
removal (keepalive expiration or netlink OVPN_CMD_DEL_PEER) at the same
moment userspace closes the TCP fd. That commit fixed the detach-side
of the same race window; this one fixes the close-side at a different
victim.

Use the already-loaded peer local, which is held by the
ovpn_peer_hold() taken under RCU and is the correct argument anyway.
The remaining lines in the function already use peer; switching this
call makes the function consistent and removes the dangling sock
dereference.

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 drivers/net/ovpn/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 65054cc84be5..ed4782de141a 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -588,7 +588,7 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
 	peer = sock->peer;
 	rcu_read_unlock();
 
-	ovpn_peer_del(sock->peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
+	ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
 	peer->tcp.sk_cb.prot->close(sk, timeout);
 	ovpn_peer_put(peer);
 }
-- 
2.53.0

[PATCH net 2/2] ovpn: respect peer refcount in CMD_NEW_PEER error path

[David Carlier]

ovpn_nl_peer_new_doit()'s error path calls ovpn_peer_release() directly
rather than ovpn_peer_put(), bypassing the kref. The accompanying
comment ("peer was not yet hashed, thus it is not used in any context")
holds for UDP but not for TCP.

For UDP, the ovpn_socket union uses the .ovpn arm and never points back
at a peer; UDP encap_recv looks up peers via the not-yet-populated
hashtables, so the new peer is unreachable until ovpn_peer_add()
publishes it.

For TCP, ovpn_socket_new() sets ovpn_sock->peer and
ovpn_tcp_socket_attach() publishes ovpn_sock via rcu_assign_sk_user_data().
From that moment until ovpn_socket_release() detaches in the error path,
the TCP fd is fully wired: userspace recvmsg / sendmsg / close / poll
on the fd, as well as the strparser-driven ovpn_tcp_rcv() path, can
reach the peer through sk_user_data -> ovpn_sock->peer and bump its
refcount via ovpn_peer_hold().

ovpn_tcp_socket_wait_finish() (called inside ovpn_socket_release())
drains strparser and the tx work, but does not synchronize with
userspace syscall callers that already hold a peer reference. If
ovpn_nl_peer_modify() or ovpn_peer_add() returns an error while such
a caller is in flight - notably an ovpn_tcp_recvmsg() blocked in
__skb_recv_datagram() on peer->tcp.user_queue - the direct
ovpn_peer_release() destroys the peer while the caller still holds
the reference, and the eventual ovpn_peer_put() from that caller
operates on freed memory.

Replace the direct destructor call with ovpn_peer_put() so the kref
correctly defers destruction until the last reference is dropped.
In the common case where no concurrent user is present, behaviour is
unchanged: the kref hits zero immediately and ovpn_peer_release_kref()
runs the same destructor.

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 drivers/net/ovpn/netlink.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index 291e2e5bb450..4c66c1ec497e 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -462,10 +462,12 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 sock_release:
 	ovpn_socket_release(peer);
 peer_release:
-	/* release right away because peer was not yet hashed, thus it is not
-	 * used in any context
+	/* For UDP, the peer is unreachable until added to the hashtables, so
+	 * dropping the initial reference is enough. For TCP, the peer may be
+	 * concurrently reachable via sk_user_data->peer until
+	 * ovpn_socket_release() detaches; rely on the refcount.
 	 */
-	ovpn_peer_release(peer);
+	ovpn_peer_put(peer);
 
 	return ret;
 }
-- 
2.53.0

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

[Eric Dumazet]

On Mon, May 11, 2026 at 9:20 PM David Carlier <devnexen@gmail.com> wrote:
>
> ovpn_tcp_close() loads the ovpn_socket via rcu_dereference_sk_user_data()
> under rcu_read_lock(), takes a reference on sock->peer, caches the peer
> pointer in a local, and drops the read lock. It then passes sock->peer
> (rather than the cached local) to ovpn_peer_del(), re-dereferencing the
> ovpn_socket after the RCU read section has ended.
>
> Unlike ovpn_tcp_sendmsg(), which uses the same "load under RCU, use
> after unlock" pattern but is protected by lock_sock() held across the
> function, ovpn_tcp_close() runs without the socket lock: inet_release()
> invokes sk_prot->close() without taking lock_sock first.
>
> ovpn_socket_release() can therefore complete its kref_put -> detach ->
> synchronize_rcu -> kfree(sock) sequence concurrently, in the window
> after ovpn_tcp_close() drops rcu_read_lock() but before it dereferences
> sock->peer. The synchronize_rcu() in ovpn_socket_release() protects
> readers that use the dereferenced pointer inside the RCU read section,
> not those that escape the pointer to a local and use it afterwards.
>
> A reproducer follows the pattern of commit 94560267d6c4 ("ovpn: tcp -
> don't deref NULL sk_socket member after tcp_close()"): trigger a peer
> removal (keepalive expiration or netlink OVPN_CMD_DEL_PEER) at the same
> moment userspace closes the TCP fd. That commit fixed the detach-side
> of the same race window; this one fixes the close-side at a different
> victim.
>
> Use the already-loaded peer local, which is held by the
> ovpn_peer_hold() taken under RCU and is the correct argument anyway.
> The remaining lines in the function already use peer; switching this
> call makes the function consistent and removes the dangling sock
> dereference.
>
> Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
>  drivers/net/ovpn/tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
> index 65054cc84be5..ed4782de141a 100644
> --- a/drivers/net/ovpn/tcp.c
> +++ b/drivers/net/ovpn/tcp.c
> @@ -588,7 +588,7 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
>         peer = sock->peer;
>         rcu_read_unlock();
>
> -       ovpn_peer_del(sock->peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
> +       ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
>         peer->tcp.sk_cb.prot->close(sk, timeout);
>         ovpn_peer_put(peer);
>  }

I do not see how rcu_read_lock() can protect sock->peer changes.

I think we need to be more careful, in the prior code which reads
sock->peer 3 times.

If RCU is used, we better use it properly.

rcu_read_lock();
sock = rcu_dereference_sk_user_data(sk);
if (!sock || !sock->peer || !ovpn_peer_hold(sock->peer)) {
        rcu_read_unlock();
        return;
}
peer = sock->peer;
rcu_read_unlock();

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

[David CARLIER]

Hi Eric,

On Tue, 12 May 2026 at 05:29, Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, May 11, 2026 at 9:20 PM David Carlier <devnexen@gmail.com> wrote:
> >
> > ovpn_tcp_close() loads the ovpn_socket via rcu_dereference_sk_user_data()
> > under rcu_read_lock(), takes a reference on sock->peer, caches the peer
> > pointer in a local, and drops the read lock. It then passes sock->peer
> > (rather than the cached local) to ovpn_peer_del(), re-dereferencing the
> > ovpn_socket after the RCU read section has ended.
> >
> > Unlike ovpn_tcp_sendmsg(), which uses the same "load under RCU, use
> > after unlock" pattern but is protected by lock_sock() held across the
> > function, ovpn_tcp_close() runs without the socket lock: inet_release()
> > invokes sk_prot->close() without taking lock_sock first.
> >
> > ovpn_socket_release() can therefore complete its kref_put -> detach ->
> > synchronize_rcu -> kfree(sock) sequence concurrently, in the window
> > after ovpn_tcp_close() drops rcu_read_lock() but before it dereferences
> > sock->peer. The synchronize_rcu() in ovpn_socket_release() protects
> > readers that use the dereferenced pointer inside the RCU read section,
> > not those that escape the pointer to a local and use it afterwards.
> >
> > A reproducer follows the pattern of commit 94560267d6c4 ("ovpn: tcp -
> > don't deref NULL sk_socket member after tcp_close()"): trigger a peer
> > removal (keepalive expiration or netlink OVPN_CMD_DEL_PEER) at the same
> > moment userspace closes the TCP fd. That commit fixed the detach-side
> > of the same race window; this one fixes the close-side at a different
> > victim.
> >
> > Use the already-loaded peer local, which is held by the
> > ovpn_peer_hold() taken under RCU and is the correct argument anyway.
> > The remaining lines in the function already use peer; switching this
> > call makes the function consistent and removes the dangling sock
> > dereference.
> >
> > Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
> > Assisted-by: Claude:claude-opus-4-7
> > Signed-off-by: David Carlier <devnexen@gmail.com>
> > ---
> >  drivers/net/ovpn/tcp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
> > index 65054cc84be5..ed4782de141a 100644
> > --- a/drivers/net/ovpn/tcp.c
> > +++ b/drivers/net/ovpn/tcp.c
> > @@ -588,7 +588,7 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
> >         peer = sock->peer;
> >         rcu_read_unlock();
> >
> > -       ovpn_peer_del(sock->peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
> > +       ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
> >         peer->tcp.sk_cb.prot->close(sk, timeout);
> >         ovpn_peer_put(peer);
> >  }
>
> I do not see how rcu_read_lock() can protect sock->peer changes.
>
> I think we need to be more careful, in the prior code which reads
> sock->peer 3 times.
>
> If RCU is used, we better use it properly.
>
> rcu_read_lock();
> sock = rcu_dereference_sk_user_data(sk);
> if (!sock || !sock->peer || !ovpn_peer_hold(sock->peer)) {
>         rcu_read_unlock();
>         return;
> }
> peer = sock->peer;
> rcu_read_unlock();

You're right. sock->peer is only assigned once, in ovpn_socket_new()
  under lock_sock() before the ovpn_socket is even published via
  rcu_assign_sk_user_data(), and never touched again - which is why the
  three reads happen to be stable. But that's the kind of invariant the
  next person has to go dig up, so the pattern is doing nobody any
  favours.

  v2 tomorrow with sock->peer read once into the local up front, and
  all the subsequent uses going through that.

  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.

  Thanks,
  David

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

[Antonio Quartulli]

Hi,

On 12/05/2026 06:56, David CARLIER wrote:
> Hi Eric,
> 
> On Tue, 12 May 2026 at 05:29, Eric Dumazet <edumazet@google.com> wrote:
>>
>> On Mon, May 11, 2026 at 9:20 PM David Carlier <devnexen@gmail.com> wrote:
>>>
>>> ovpn_tcp_close() loads the ovpn_socket via rcu_dereference_sk_user_data()
>>> under rcu_read_lock(), takes a reference on sock->peer, caches the peer
>>> pointer in a local, and drops the read lock. It then passes sock->peer
>>> (rather than the cached local) to ovpn_peer_del(), re-dereferencing the
>>> ovpn_socket after the RCU read section has ended.
>>>
>>> Unlike ovpn_tcp_sendmsg(), which uses the same "load under RCU, use
>>> after unlock" pattern but is protected by lock_sock() held across the
>>> function, ovpn_tcp_close() runs without the socket lock: inet_release()
>>> invokes sk_prot->close() without taking lock_sock first.
>>>
>>> ovpn_socket_release() can therefore complete its kref_put -> detach ->
>>> synchronize_rcu -> kfree(sock) sequence concurrently, in the window
>>> after ovpn_tcp_close() drops rcu_read_lock() but before it dereferences
>>> sock->peer. The synchronize_rcu() in ovpn_socket_release() protects
>>> readers that use the dereferenced pointer inside the RCU read section,
>>> not those that escape the pointer to a local and use it afterwards.
>>>
>>> A reproducer follows the pattern of commit 94560267d6c4 ("ovpn: tcp -
>>> don't deref NULL sk_socket member after tcp_close()"): trigger a peer
>>> removal (keepalive expiration or netlink OVPN_CMD_DEL_PEER) at the same
>>> moment userspace closes the TCP fd. That commit fixed the detach-side
>>> of the same race window; this one fixes the close-side at a different
>>> victim.
>>>
>>> Use the already-loaded peer local, which is held by the
>>> ovpn_peer_hold() taken under RCU and is the correct argument anyway.
>>> The remaining lines in the function already use peer; switching this
>>> call makes the function consistent and removes the dangling sock
>>> dereference.
>>>
>>> Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
>>> Assisted-by: Claude:claude-opus-4-7
>>> Signed-off-by: David Carlier <devnexen@gmail.com>
>>> ---
>>>   drivers/net/ovpn/tcp.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
>>> index 65054cc84be5..ed4782de141a 100644
>>> --- a/drivers/net/ovpn/tcp.c
>>> +++ b/drivers/net/ovpn/tcp.c
>>> @@ -588,7 +588,7 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
>>>          peer = sock->peer;
>>>          rcu_read_unlock();
>>>
>>> -       ovpn_peer_del(sock->peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
>>> +       ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
>>>          peer->tcp.sk_cb.prot->close(sk, timeout);
>>>          ovpn_peer_put(peer);
>>>   }
>>
>> I do not see how rcu_read_lock() can protect sock->peer changes.
>>
>> I think we need to be more careful, in the prior code which reads
>> sock->peer 3 times.
>>
>> If RCU is used, we better use it properly.
>>
>> rcu_read_lock();
>> sock = rcu_dereference_sk_user_data(sk);
>> if (!sock || !sock->peer || !ovpn_peer_hold(sock->peer)) {
>>          rcu_read_unlock();
>>          return;
>> }
>> peer = sock->peer;
>> rcu_read_unlock();
> 
> You're right. sock->peer is only assigned once, in ovpn_socket_new()
>    under lock_sock() before the ovpn_socket is even published via
>    rcu_assign_sk_user_data(), and never touched again - which is why the
>    three reads happen to be stable. But that's the kind of invariant the
>    next person has to go dig up, so the pattern is doing nobody any
>    favours.
> 
>    v2 tomorrow with sock->peer read once into the local up front, and
>    all the subsequent uses going through that.
> 
>    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.

I have to double check all the mentioned spots, however this sounds more 
like a larger refactoring for net-next that we should be carefully 
review, rather than a bugfix.


Regards,


-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net 2/2] ovpn: respect peer refcount in CMD_NEW_PEER error path

[Antonio Quartulli]

Hi,

On 12/05/2026 06:19, David Carlier wrote:
> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> index 291e2e5bb450..4c66c1ec497e 100644
> --- a/drivers/net/ovpn/netlink.c
> +++ b/drivers/net/ovpn/netlink.c
> @@ -462,10 +462,12 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>   sock_release:
>   	ovpn_socket_release(peer);
>   peer_release:
> -	/* release right away because peer was not yet hashed, thus it is not
> -	 * used in any context
> +	/* For UDP, the peer is unreachable until added to the hashtables, so
> +	 * dropping the initial reference is enough. For TCP, the peer may be
> +	 * concurrently reachable via sk_user_data->peer until
> +	 * ovpn_socket_release() detaches; rely on the refcount.
>   	 */
> -	ovpn_peer_release(peer);
> +	ovpn_peer_put(peer);

Thanks for this patch!

Unless the CI Complains, I'm pulling this in my tree and staging it for 
the next PR to net.

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

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

[Antonio Quartulli]

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.

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.

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

[Sabrina Dubroca]

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? Then just wait
until David sends the whole change at once?

And since the "rcu_dereference_sk_user_data + !sock || !sock->peer ||
!ovpn_peer_hold(sock->peer)" pattern is used multiple times, it would
be better to turn it into a helper (ovpn_tcp_get_peer()?). That way
the caller can't be tempted to play with sock->peer.

(also, please wait at least the usual 24 hours before applying)

-- 
Sabrina

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

[Antonio Quartulli]

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?

 >  Then just wait until David sends the whole change at once?

Yap.

> 
> And since the "rcu_dereference_sk_user_data + !sock || !sock->peer ||
> !ovpn_peer_hold(sock->peer)" pattern is used multiple times, it would
> be better to turn it into a helper (ovpn_tcp_get_peer()?). That way
> the caller can't be tempted to play with sock->peer.
> 
> (also, please wait at least the usual 24 hours before applying)

ACK


Cheers,


-- 
Antonio Quartulli
OpenVPN Inc.

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

[Sabrina Dubroca]

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

Re: [PATCH net 2/2] ovpn: respect peer refcount in CMD_NEW_PEER error path

[Sabrina Dubroca]

2026-05-12, 05:19:13 +0100, David Carlier wrote:
> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> index 291e2e5bb450..4c66c1ec497e 100644
> --- a/drivers/net/ovpn/netlink.c
> +++ b/drivers/net/ovpn/netlink.c
> @@ -462,10 +462,12 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>  sock_release:
>  	ovpn_socket_release(peer);
>  peer_release:
> -	/* release right away because peer was not yet hashed, thus it is not
> -	 * used in any context
> +	/* For UDP, the peer is unreachable until added to the hashtables, so
> +	 * dropping the initial reference is enough. For TCP, the peer may be
> +	 * concurrently reachable via sk_user_data->peer until
> +	 * ovpn_socket_release() detaches; rely on the refcount.
>  	 */
> -	ovpn_peer_release(peer);
> +	ovpn_peer_put(peer);
>  
>  	return ret;
>  }

nit: after this change, ovpn_peer_release() is only used within
peer.c, and can become static.

I think it'd be nicer to do that now with a v2, but if Antonio wants
to pick this patch up directly, the fix LGTM:

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

-- 
Sabrina

Re: [PATCH net 2/2] ovpn: respect peer refcount in CMD_NEW_PEER error path

[Antonio Quartulli]

On 12/05/2026 17:13, Sabrina Dubroca wrote:
> 2026-05-12, 05:19:13 +0100, David Carlier wrote:
>> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
>> index 291e2e5bb450..4c66c1ec497e 100644
>> --- a/drivers/net/ovpn/netlink.c
>> +++ b/drivers/net/ovpn/netlink.c
>> @@ -462,10 +462,12 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>>   sock_release:
>>   	ovpn_socket_release(peer);
>>   peer_release:
>> -	/* release right away because peer was not yet hashed, thus it is not
>> -	 * used in any context
>> +	/* For UDP, the peer is unreachable until added to the hashtables, so
>> +	 * dropping the initial reference is enough. For TCP, the peer may be
>> +	 * concurrently reachable via sk_user_data->peer until
>> +	 * ovpn_socket_release() detaches; rely on the refcount.
>>   	 */
>> -	ovpn_peer_release(peer);
>> +	ovpn_peer_put(peer);
>>   
>>   	return ret;
>>   }
> 
> nit: after this change, ovpn_peer_release() is only used within
> peer.c, and can become static.
> 
> I think it'd be nicer to do that now with a v2, but if Antonio wants
> to pick this patch up directly, the fix LGTM:
> 
> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> 

I think it makes sense to make it static now.

@David: how about sending v2 with this change included?

Thanks a lot!
Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net v2 0/2] ovpn: fix TCP teardown UAF races

[David Carlier]

v1: https://lore.kernel.org/netdev/20260512042036.19870-1-devnexen@gmail.com/

Two distinct UAFs in the TCP transport teardown path, both inherited
from 11851cbd60ea ("ovpn: implement TCP transport"). They share a race
class with the already-merged 94560267d6c4 ("ovpn: tcp - don't deref
NULL sk_socket member after tcp_close()"), which fixed the detach-side
of the keepalive-vs-userspace-close race window; the two patches in
this series cover the other two victim sites in the same window.

Patch 1 fixes a UAF in ovpn_tcp_close(). The function loads the
ovpn_socket via rcu_dereference_sk_user_data(), caches sock->peer in a
local, drops rcu_read_lock, and then passes sock->peer (rather than
the cached local) to ovpn_peer_del(). Unlike ovpn_tcp_sendmsg() which
uses the same pattern but is protected by lock_sock, ovpn_tcp_close()
runs without the socket lock - inet_release() does not lock_sock
before calling sk_prot->close. A concurrent ovpn_socket_release() can
therefore complete kref_put -> detach -> synchronize_rcu -> kfree(sock)
in the window after rcu_read_unlock() but before the dangling
sock->peer dereference.

Patch 2 fixes the CMD_NEW_PEER error path in ovpn_nl_peer_new_doit(),
which calls ovpn_peer_release() directly rather than ovpn_peer_put(),
bypassing the kref. The accompanying "peer was not yet hashed, thus
not used in any context" comment is correct for UDP but wrong for TCP:
after ovpn_tcp_socket_attach() publishes ovpn_sock via
rcu_assign_sk_user_data(), the peer is reachable via sk_user_data ->
ovpn_sock->peer; userspace recvmsg/sendmsg/close/poll and the
strparser-driven ovpn_tcp_rcv() path can bump its refcount.

Both Fixes: 11851cbd60ea ("ovpn: implement TCP transport"). Not Cc'd
to stable - borderline practical reachability and reproducer is
non-trivial; maintainer judgement on backport.

Changes since v1:
- Patch 1: tighten the entry block to read sock->peer exactly once
  into the cached peer local; route the hold check, ovpn_peer_del()
  and prot->close() invocations through that local (Eric Dumazet).
  The same multi-read pattern in ovpn_tcp_recvmsg(), ovpn_tcp_sendmsg(),
  ovpn_tcp_data_ready() and ovpn_tcp_write_space() will be handled by
  a dedicated helper in a follow-up net-next series (Sabrina Dubroca,
  Antonio Quartulli).
- Patch 2: make ovpn_peer_release() static and drop its declaration
  from peer.h, since the netlink callsite was the last external user
  (Sabrina Dubroca, Antonio Quartulli).
- Both patches: add Reviewed-by from Sabrina Dubroca.

David Carlier (2):
  ovpn: tcp - use cached peer pointer in ovpn_tcp_close()
  ovpn: respect peer refcount in CMD_NEW_PEER error path

 drivers/net/ovpn/netlink.c | 8 +++++---
 drivers/net/ovpn/peer.c    | 2 +-
 drivers/net/ovpn/peer.h    | 1 -
 drivers/net/ovpn/tcp.c     | 9 +++++++--
 4 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.53.0

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

[David Carlier]

ovpn_tcp_close() loads the ovpn_socket via rcu_dereference_sk_user_data()
under rcu_read_lock(), takes a reference on sock->peer, caches the peer
pointer in a local, and drops the read lock. It then passes sock->peer
(rather than the cached local) to ovpn_peer_del(), re-dereferencing the
ovpn_socket after the RCU read section has ended.

Unlike ovpn_tcp_sendmsg(), which uses the same "load under RCU, use
after unlock" pattern but is protected by lock_sock() held across the
function, ovpn_tcp_close() runs without the socket lock: inet_release()
invokes sk_prot->close() without taking lock_sock first.

ovpn_socket_release() can therefore complete its kref_put -> detach ->
synchronize_rcu -> kfree(sock) sequence concurrently, in the window
after ovpn_tcp_close() drops rcu_read_lock() but before it dereferences
sock->peer. The synchronize_rcu() in ovpn_socket_release() protects
readers that use the dereferenced pointer inside the RCU read section,
not those that escape the pointer to a local and use it afterwards.

A reproducer follows the pattern of commit 94560267d6c4 ("ovpn: tcp -
don't deref NULL sk_socket member after tcp_close()"): trigger a peer
removal (keepalive expiration or netlink OVPN_CMD_DEL_PEER) at the same
moment userspace closes the TCP fd. That commit fixed the detach-side
of the same race window; this one fixes the close-side at a different
victim.

Tighten the entry block to read sock->peer exactly once into the cached
peer local, and route all subsequent uses (the hold check, the
ovpn_peer_del() call, and the prot->close() invocation) through that
local. sock->peer is only ever written once in ovpn_socket_new() under
lock_sock(), before rcu_assign_sk_user_data() publishes the ovpn_socket,
and is never reassigned afterwards - but the previous multi-read pattern
made that invariant implicit rather than explicit. The same multi-read
shape exists in ovpn_tcp_recvmsg(), ovpn_tcp_sendmsg(),
ovpn_tcp_data_ready() and ovpn_tcp_write_space(); those will be cleaned
up via a dedicated helper in a follow-up net-next series.

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: David Carlier <devnexen@gmail.com>
---

v2:
- Tighten the entry block to read sock->peer exactly once into the
  cached peer local; route the hold check, ovpn_peer_del() call and
  prot->close() invocation through that local (Eric Dumazet)
- Add Reviewed-by from Sabrina Dubroca
- The same multi-read sock->peer pattern in ovpn_tcp_recvmsg(),
  ovpn_tcp_sendmsg(), ovpn_tcp_data_ready() and ovpn_tcp_write_space()
  will be handled by a dedicated helper in a follow-up net-next series
  (Sabrina Dubroca, Antonio Quartulli)

 drivers/net/ovpn/tcp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 65054cc84be5..82809b016f0a 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -581,14 +581,19 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
 
 	rcu_read_lock();
 	sock = rcu_dereference_sk_user_data(sk);
-	if (!sock || !sock->peer || !ovpn_peer_hold(sock->peer)) {
+	if (!sock) {
 		rcu_read_unlock();
 		return;
 	}
+
 	peer = sock->peer;
+	if (!peer || !ovpn_peer_hold(peer)) {
+		rcu_read_unlock();
+		return;
+	}
 	rcu_read_unlock();
 
-	ovpn_peer_del(sock->peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
+	ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT);
 	peer->tcp.sk_cb.prot->close(sk, timeout);
 	ovpn_peer_put(peer);
 }
-- 
2.53.0

[PATCH v2 2/2] ovpn: respect peer refcount in CMD_NEW_PEER error path

[David Carlier]

ovpn_nl_peer_new_doit()'s error path calls ovpn_peer_release() directly
rather than ovpn_peer_put(), bypassing the kref. The accompanying
comment ("peer was not yet hashed, thus it is not used in any context")
holds for UDP but not for TCP.

For UDP, the ovpn_socket union uses the .ovpn arm and never points back
at a peer; UDP encap_recv looks up peers via the not-yet-populated
hashtables, so the new peer is unreachable until ovpn_peer_add()
publishes it.

For TCP, ovpn_socket_new() sets ovpn_sock->peer and
ovpn_tcp_socket_attach() publishes ovpn_sock via rcu_assign_sk_user_data().
From that moment until ovpn_socket_release() detaches in the error path,
the TCP fd is fully wired: userspace recvmsg / sendmsg / close / poll
on the fd, as well as the strparser-driven ovpn_tcp_rcv() path, can
reach the peer through sk_user_data -> ovpn_sock->peer and bump its
refcount via ovpn_peer_hold().

ovpn_tcp_socket_wait_finish() (called inside ovpn_socket_release())
drains strparser and the tx work, but does not synchronize with
userspace syscall callers that already hold a peer reference. If
ovpn_nl_peer_modify() or ovpn_peer_add() returns an error while such
a caller is in flight - notably an ovpn_tcp_recvmsg() blocked in
__skb_recv_datagram() on peer->tcp.user_queue - the direct
ovpn_peer_release() destroys the peer while the caller still holds
the reference, and the eventual ovpn_peer_put() from that caller
operates on freed memory.

Replace the direct destructor call with ovpn_peer_put() so the kref
correctly defers destruction until the last reference is dropped.
In the common case where no concurrent user is present, behaviour is
unchanged: the kref hits zero immediately and ovpn_peer_release_kref()
runs the same destructor.

With this conversion ovpn_peer_release() has no callers outside peer.c
- ovpn_peer_release_kref() in the same translation unit is the only
remaining user - so make it static and drop its declaration from
peer.h.

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: David Carlier <devnexen@gmail.com>
---

v2:
- Make ovpn_peer_release() static and drop its declaration from
  peer.h, since the netlink callsite was the last external user
  (Sabrina Dubroca, Antonio Quartulli)
- Add Reviewed-by from Sabrina Dubroca

 drivers/net/ovpn/netlink.c | 8 +++++---
 drivers/net/ovpn/peer.c    | 2 +-
 drivers/net/ovpn/peer.h    | 1 -
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index 291e2e5bb450..4c66c1ec497e 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -462,10 +462,12 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 sock_release:
 	ovpn_socket_release(peer);
 peer_release:
-	/* release right away because peer was not yet hashed, thus it is not
-	 * used in any context
+	/* For UDP, the peer is unreachable until added to the hashtables, so
+	 * dropping the initial reference is enough. For TCP, the peer may be
+	 * concurrently reachable via sk_user_data->peer until
+	 * ovpn_socket_release() detaches; rely on the refcount.
 	 */
-	ovpn_peer_release(peer);
+	ovpn_peer_put(peer);
 
 	return ret;
 }
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index c02dfab51a6e..fb10d1fea940 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -354,7 +354,7 @@ static void ovpn_peer_release_rcu(struct rcu_head *head)
  * ovpn_peer_release - release peer private members
  * @peer: the peer to release
  */
-void ovpn_peer_release(struct ovpn_peer *peer)
+static void ovpn_peer_release(struct ovpn_peer *peer)
 {
 	ovpn_crypto_state_release(&peer->crypto);
 	spin_lock_bh(&peer->lock);
diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index 328401570cba..86c8cffada6d 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h
@@ -127,7 +127,6 @@ static inline bool ovpn_peer_hold(struct ovpn_peer *peer)
 	return kref_get_unless_zero(&peer->refcount);
 }
 
-void ovpn_peer_release(struct ovpn_peer *peer);
 void ovpn_peer_release_kref(struct kref *kref);
 
 /**
-- 
2.53.0