[PATCH net] net: ovpn: refuse TCP socket layering with an active ULP

[PATCH net] net: ovpn: refuse TCP socket layering with an active ULP

[Michael Bommarito]

ovpn's TCP transport replaces sk_prot, sk_data_ready, sk_write_space
and sk_socket->ops by direct field writes when a peer is attached, and
restores them by direct field writes when the peer is detached. That
layering scheme is not compatible with a TCP ULP (e.g. kTLS) being
installed on the same socket: ULP setup also captures the current
sk_prot as its lower layer and replaces sk_prot with its own. The two
schemes are not aware of each other and can be combined in either
order on the same fd.

In the order "ovpn attach, then setsockopt(TCP_ULP=tls), then
OVPN_CMD_PEER_DEL, then close", ovpn_tcp_socket_detach() restores the
saved pre-ovpn sk_prot directly. That overwrites the ULP-installed
sk_prot with the base TCP proto, so the ULP's close hook
(tls_sk_proto_close() in the kTLS case) is never invoked when
userspace closes the fd. The ULP's TX delayed work is never cancelled
and still carries the raw struct sock *, the per-ULP context is never
freed, and TlsCurrTxSw / TlsCurrRxSw are not decremented. After the
TCP socket is freed by the normal RCU path, the still-scheduled TX
delayed work fires and dereferences the freed sock:

  BUG: KASAN: slab-use-after-free in tx_work_handler+0x3a/0x146
  Workqueue: events tx_work_handler
  Read of size 8 at addr ... by task kworker/...
  Allocated by task ...:
    sk_alloc
    inet_create
    __sock_create
  Freed by task 0:
    __sk_destruct
    rcu_core

The same trigger run as a loop also produces an unbounded leak of the
per-iteration ULP context: TlsCurrTxSw and TlsCurrRxSw grow
monotonically and SUnreclaim grows in proportion, because
tls_sk_proto_close() is the only path that frees them and it is
skipped.

Fix this in three related places, all in ovpn, since ovpn is the
layer that bypasses the in-tree ULP close contract by restoring
sk_prot directly:

  - ovpn_tcp_socket_attach(): if the socket already carries a TCP ULP
    (inet_csk_has_ulp() true), return -EBUSY. ovpn does not own the
    sk_prot it would be saving, and would silently displace the ULP's
    callback chain.

  - ovpn_tcp_socket_detach(): if a ULP was layered on top of ovpn
    between attach and detach, do NOT restore the saved sk_prot /
    sk_data_ready / sk_write_space / sk_socket->ops. The ULP recorded
    ovpn_tcp_prot as its lower layer; the correct contract on detach
    is to leave the ULP's callbacks in place so that close() reaches
    the ULP's close hook and the ULP's own teardown path can run.

  - ovpn_tcp_close(): after detach has cleared sk_user_data, close
    dispatches that still land on ovpn_tcp_prot (the ULP close hook
    chains via the captured ctx->sk_proto, which equals
    &ovpn_tcp_prot; the detach-vs-close race window can also leave
    sk_prot pointing at ovpn_tcp_prot momentarily) used to early-
    return without invoking the base TCP close, leaving the TCP
    socket to be freed without a graceful FIN handshake. Chain to
    tcp_prot.close / tcpv6_prot.close in that case so the TCP layer
    still completes its normal shutdown.

OVPN_CMD_PEER_NEW and OVPN_CMD_PEER_DEL are gated by
GENL_ADMIN_PERM, and the ovpn netdev itself is created via
rtnetlink RTM_NEWLINK which also requires CAP_NET_ADMIN; both
gates resolve to CAP_NET_ADMIN in init_user_ns.  A uid 65534
process in a new user+net namespace cannot reach this code path.
The trigger therefore requires an attacker with init-ns
CAP_NET_ADMIN (host network-management service, privileged
container, or rootful container deliberately granted host
CAP_NET_ADMIN).

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Cc: stable@vger.kernel.org
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Assisted-by: Claude:claude-opus-4-7
---
 drivers/net/ovpn/tcp.c | 57 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 65054cc84be55..469f968010b74 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -9,6 +9,7 @@
 #include <linux/skbuff.h>
 #include <net/hotdata.h>
 #include <net/inet_common.h>
+#include <net/inet_connection_sock.h>
 #include <net/ipv6.h>
 #include <net/tcp.h>
 #include <net/transp_v6.h>
@@ -210,14 +211,29 @@ void ovpn_tcp_socket_detach(struct ovpn_socket *ovpn_sock)
 {
 	struct ovpn_peer *peer = ovpn_sock->peer;
 	struct sock *sk = ovpn_sock->sk;
+	bool has_ulp;
 
 	strp_stop(&peer->tcp.strp);
 	skb_queue_purge(&peer->tcp.user_queue);
 
-	/* restore CBs that were saved in ovpn_sock_set_tcp_cb() */
-	sk->sk_data_ready = peer->tcp.sk_cb.sk_data_ready;
-	sk->sk_write_space = peer->tcp.sk_cb.sk_write_space;
-	sk->sk_prot = peer->tcp.sk_cb.prot;
+	/* If a TCP ULP (e.g. kTLS) was installed on top of our protocol
+	 * callbacks after ovpn_tcp_socket_attach(), the ULP recorded
+	 * ovpn_tcp_prot as its lower layer and replaced sk_prot with its
+	 * own. Directly restoring the pre-attach sk_prot here would
+	 * overwrite the ULP's sk_prot with the base TCP proto, bypassing
+	 * the ULP close path entirely on subsequent close() (which would
+	 * leak the ULP context and leave any deferred ULP work pointing
+	 * at the freed sock). Leave the callbacks alone in that case;
+	 * the ULP's close path is responsible for both its own teardown
+	 * and for chaining into the saved lower-layer close.
+	 */
+	has_ulp = inet_csk_has_ulp(sk);
+	if (!has_ulp) {
+		/* restore CBs that were saved in ovpn_sock_set_tcp_cb() */
+		sk->sk_data_ready = peer->tcp.sk_cb.sk_data_ready;
+		sk->sk_write_space = peer->tcp.sk_cb.sk_write_space;
+		sk->sk_prot = peer->tcp.sk_cb.prot;
+	}
 
 	/* tcp_close() may race this function and could set
 	 * sk->sk_socket to NULL. It does so by invoking
@@ -228,7 +244,7 @@ void ovpn_tcp_socket_detach(struct ovpn_socket *ovpn_sock)
 	 * sk_socket to disappear under our feet
 	 */
 	write_lock_bh(&sk->sk_callback_lock);
-	if (sk->sk_socket)
+	if (sk->sk_socket && !has_ulp)
 		sk->sk_socket->ops = peer->tcp.sk_cb.ops;
 	write_unlock_bh(&sk->sk_callback_lock);
 
@@ -518,6 +534,17 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
 	/* make sure no pre-existing encapsulation handler exists */
 	if (ovpn_sock->sk->sk_user_data)
 		return -EBUSY;
+
+	/* refuse to layer on top of an already-attached TCP ULP (e.g.
+	 * kTLS). ovpn replaces sk_prot/sk_data_ready/sk_write_space by
+	 * direct field writes; layering it under an existing ULP would
+	 * confuse the ULP's saved-proto chain. The inverse case (a ULP
+	 * layered on top of ovpn after attach) is handled in
+	 * ovpn_tcp_socket_detach().
+	 */
+	if (inet_csk_has_ulp(ovpn_sock->sk))
+		return -EBUSY;
+
 	rcu_assign_sk_user_data(ovpn_sock->sk, ovpn_sock);
 
 	/* only a fully connected socket is expected. Connection should be
@@ -583,6 +610,26 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
 	sock = rcu_dereference_sk_user_data(sk);
 	if (!sock || !sock->peer || !ovpn_peer_hold(sock->peer)) {
 		rcu_read_unlock();
+		/* No (or no-longer-attached) ovpn state on this socket.
+		 * That happens when ovpn_tcp_socket_detach() already ran
+		 * and cleared sk_user_data, but the caller chain still
+		 * dispatches ->close() on ovpn_tcp_prot. The two cases
+		 * that exhibit this are:
+		 *   - a TCP ULP layered on top of ovpn whose close hook
+		 *     chains via the captured ctx->sk_proto (which is
+		 *     &ovpn_tcp_prot) after detach has run;
+		 *   - the close-vs-detach race window where sk_user_data
+		 *     has been cleared but sk_prot has not yet been
+		 *     restored.
+		 * In both cases the TCP layer still owes the peer a
+		 * graceful close, so chain to the base TCP close.
+		 */
+#if IS_ENABLED(CONFIG_IPV6)
+		if (sk->sk_family == AF_INET6)
+			tcpv6_prot.close(sk, timeout);
+		else
+#endif
+			tcp_prot.close(sk, timeout);
 		return;
 	}
 	peer = sock->peer;

base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.53.0

Re: [PATCH net] net: ovpn: refuse TCP socket layering with an active ULP

[Sabrina Dubroca]

2026-05-17, 11:56:45 -0400, Michael Bommarito wrote:
> ovpn's TCP transport replaces sk_prot, sk_data_ready, sk_write_space
> and sk_socket->ops by direct field writes when a peer is attached, and
> restores them by direct field writes when the peer is detached. That
> layering scheme is not compatible with a TCP ULP (e.g. kTLS) being
> installed on the same socket: ULP setup also captures the current
> sk_prot as its lower layer and replaces sk_prot with its own. The two
> schemes are not aware of each other and can be combined in either
> order on the same fd.

It looks like we should make ovpn-tcp a ULP (without requesting
userspace to do the setsockopt, since we messed that up a year ago. I
thought I had suggested that at some point, but maybe not). That's
what it is anyway, and layering ovpn on top of (or underneath) ktls or
espintcp makes no sense. It would save us the messy restore hacks in
this patch.

mptcp calls tcp_set_ulp() from the setup, maybe ovpn can also do
that. detach() may be an issue.


[...]
> @@ -583,6 +610,26 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
>  	sock = rcu_dereference_sk_user_data(sk);
>  	if (!sock || !sock->peer || !ovpn_peer_hold(sock->peer)) {
>  		rcu_read_unlock();
> +		/* No (or no-longer-attached) ovpn state on this socket.
> +		 * That happens when ovpn_tcp_socket_detach() already ran
> +		 * and cleared sk_user_data, but the caller chain still
> +		 * dispatches ->close() on ovpn_tcp_prot. The two cases
> +		 * that exhibit this are:
> +		 *   - a TCP ULP layered on top of ovpn whose close hook
> +		 *     chains via the captured ctx->sk_proto (which is
> +		 *     &ovpn_tcp_prot) after detach has run;
> +		 *   - the close-vs-detach race window where sk_user_data
> +		 *     has been cleared but sk_prot has not yet been
> +		 *     restored.
> +		 * In both cases the TCP layer still owes the peer a
> +		 * graceful close, so chain to the base TCP close.
> +		 */

This is a whole commit message, not a code comment.

> +#if IS_ENABLED(CONFIG_IPV6)
> +		if (sk->sk_family == AF_INET6)
> +			tcpv6_prot.close(sk, timeout);
> +		else
> +#endif
> +			tcp_prot.close(sk, timeout);

Isn't that a separate problem? (detach racing with close)
Or it only appears after the other changes because you removed
clearing the CBs?

-- 
Sabrina

Re: [PATCH net] net: ovpn: refuse TCP socket layering with an active ULP

[Antonio Quartulli]

On 19/05/2026 16:32, Sabrina Dubroca wrote:
> 2026-05-17, 11:56:45 -0400, Michael Bommarito wrote:
>> ovpn's TCP transport replaces sk_prot, sk_data_ready, sk_write_space
>> and sk_socket->ops by direct field writes when a peer is attached, and
>> restores them by direct field writes when the peer is detached. That
>> layering scheme is not compatible with a TCP ULP (e.g. kTLS) being
>> installed on the same socket: ULP setup also captures the current
>> sk_prot as its lower layer and replaces sk_prot with its own. The two
>> schemes are not aware of each other and can be combined in either
>> order on the same fd.
> 
> It looks like we should make ovpn-tcp a ULP (without requesting
> userspace to do the setsockopt, since we messed that up a year ago. I
> thought I had suggested that at some point, but maybe not). That's
> what it is anyway, and layering ovpn on top of (or underneath) ktls or
> espintcp makes no sense. It would save us the messy restore hacks in
> this patch.

I agree. I was just thinking about a way to prevent this entirely 
without having to make each module (somewhat) aware of each other.

> 
> mptcp calls tcp_set_ulp() from the setup, maybe ovpn can also do
> that. detach() may be an issue.


Yeah, looking into using tcp_set_ulp() makes sense.

> 
> 
> [...]
>> @@ -583,6 +610,26 @@ static void ovpn_tcp_close(struct sock *sk, long timeout)
>>   	sock = rcu_dereference_sk_user_data(sk);
>>   	if (!sock || !sock->peer || !ovpn_peer_hold(sock->peer)) {
>>   		rcu_read_unlock();
>> +		/* No (or no-longer-attached) ovpn state on this socket.
>> +		 * That happens when ovpn_tcp_socket_detach() already ran
>> +		 * and cleared sk_user_data, but the caller chain still
>> +		 * dispatches ->close() on ovpn_tcp_prot. The two cases
>> +		 * that exhibit this are:
>> +		 *   - a TCP ULP layered on top of ovpn whose close hook
>> +		 *     chains via the captured ctx->sk_proto (which is
>> +		 *     &ovpn_tcp_prot) after detach has run;
>> +		 *   - the close-vs-detach race window where sk_user_data
>> +		 *     has been cleared but sk_prot has not yet been
>> +		 *     restored.
>> +		 * In both cases the TCP layer still owes the peer a
>> +		 * graceful close, so chain to the base TCP close.
>> +		 */
> 
> This is a whole commit message, not a code comment.

I am also not sure what would happen if ovpn goes away first and then 
kTLS is left with the ovpn CBs saved in its captured sk_proto.

Anyway, if we can prevent this from happening at all, as per Sabrina's 
comment above, we would make our life much simpler.

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net] net: ovpn: refuse TCP socket layering with an active ULP

[Michael Bommarito]

On Tue, May 19, 2026 at 10:37 AM Antonio Quartulli <antonio@openvpn.net> wrote:
> > This is a whole commit message, not a code comment.
>
> I am also not sure what would happen if ovpn goes away first and then
> kTLS is left with the ovpn CBs saved in its captured sk_proto.
>
> Anyway, if we can prevent this from happening at all, as per Sabrina's
> comment above, we would make our life much simpler.

Yeah, I had to iterate on this 2 or 3 times before catching these
loose ends.  Here are some cscope/clang+Claude notes that might help:

  1. ULP before ovpn needed the attach-side inet_csk_has_ulp() refusal.
  2. ovpn before ULP made simple detach restoration unsafe because it
overwrote the TLS sk_prot and skipped tls_sk_proto_close().
  3. The second design, leaving ULP callbacks intact on detach,
exposed the close-chain issue: TLS had captured ovpn_tcp_close as its
lower close, but ovpn detach had already cleared sk_user_data.
  4. That explains the “extra junk”: the sent patch’s ovpn_tcp_close()
fallback to base TCP close was the S1 fix for that close-chain case.

I have no horse in the race when it comes to keeping my patch or
throwing it away in favor of a better design.

Most useful thing I can provide is probably the setup to reproduce the
issue.  Let me know and I'll send off chain.

Thanks,Mike

Re: [PATCH net] net: ovpn: refuse TCP socket layering with an active ULP

[Antonio Quartulli]

Hi Sabrina,

On 19/05/2026 16:32, Sabrina Dubroca wrote:
> mptcp calls tcp_set_ulp() from the setup, maybe ovpn can also do
> that. detach() may be an issue.

What is your exact concern with detach()? if we prevent other protocols 
from stacking, what is there to deal with in detach()?

Cheers,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net] net: ovpn: refuse TCP socket layering with an active ULP

[Sabrina Dubroca]

2026-05-20, 09:22:01 +0200, Antonio Quartulli wrote:
> Hi Sabrina,
> 
> On 19/05/2026 16:32, Sabrina Dubroca wrote:
> > mptcp calls tcp_set_ulp() from the setup, maybe ovpn can also do
> > that. detach() may be an issue.
> 
> What is your exact concern with detach()? if we prevent other protocols from
> stacking, what is there to deal with in detach()?

IIRC, ULP doesn't have a concept of "detach" (without close). So if
you want to detach and then reattach the same socket, I think the ULP
infra will need some work.

-- 
Sabrina