ovpn: set sk_user_data before overriding callbacks

During initialization, we override socket callbacks and set sk_user_data
to an ovpn_socket instance. Currently, these two operations are
decoupled: callbacks are overridden before sk_user_data is set. While
existing callbacks perform safety checks for NULL or non-ovpn
sk_user_data, this condition causes a "half-formed" state where valid
packets arriving during attachment trigger error logs (e.g., "invoked on
non ovpn socket").

Set sk_user_data before overriding the callbacks so that it can be
accessed safely from them. Since we already check that the socket has no
sk_user_data before setting it, this remains safe even if an interrupt
accesses the socket after sk_user_data is set but before the callbacks
are overridden.

This also requires initializing all protocol-specific fields (such as
tcp_tx_work and peer links) before calling ovpn_socket_attach, ensuring
the ovpn_socket is fully formed before it becomes visible to any
callback.

Fixes: f6226ae7a0cd ("ovpn: introduce the ovpn_socket object")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index 9750871ab65cee9e354a7a6aaffbae93cc0894cd..448cee3b3f9fa2c0459c5d3351ca61496a96b651 100644 (file)
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c
@@ -200,6 +200,22 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
        ovpn_sock->sk = sk;
        kref_init(&ovpn_sock->refcount);
 
+       /* TCP sockets are per-peer, therefore they are linked to their unique
+        * peer
+        */
+       if (sk->sk_protocol == IPPROTO_TCP) {
+               INIT_WORK(&ovpn_sock->tcp_tx_work, ovpn_tcp_tx_work);
+               ovpn_sock->peer = peer;
+               ovpn_peer_hold(peer);
+       } else if (sk->sk_protocol == IPPROTO_UDP) {
+               /* in UDP we only link the ovpn instance since the socket is
+                * shared among multiple peers
+                */
+               ovpn_sock->ovpn = peer->ovpn;
+               netdev_hold(peer->ovpn->dev, &ovpn_sock->dev_tracker,
+                           GFP_KERNEL);
+       }
+
        /* the newly created ovpn_socket is holding reference to sk,
         * therefore we increase its refcounter.
         *
@@ -212,29 +228,16 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
 
        ret = ovpn_socket_attach(ovpn_sock, sock, peer);
        if (ret < 0) {
+               if (sk->sk_protocol == IPPROTO_TCP)
+                       ovpn_peer_put(peer);
+               else if (sk->sk_protocol == IPPROTO_UDP)
+                       netdev_put(peer->ovpn->dev, &ovpn_sock->dev_tracker);
+
                sock_put(sk);
                kfree(ovpn_sock);
                ovpn_sock = ERR_PTR(ret);
-               goto sock_release;
-       }
-
-       /* TCP sockets are per-peer, therefore they are linked to their unique
-        * peer
-        */
-       if (sk->sk_protocol == IPPROTO_TCP) {
-               INIT_WORK(&ovpn_sock->tcp_tx_work, ovpn_tcp_tx_work);
-               ovpn_sock->peer = peer;
-               ovpn_peer_hold(peer);
-       } else if (sk->sk_protocol == IPPROTO_UDP) {
-               /* in UDP we only link the ovpn instance since the socket is
-                * shared among multiple peers
-                */
-               ovpn_sock->ovpn = peer->ovpn;
-               netdev_hold(peer->ovpn->dev, &ovpn_sock->dev_tracker,
-                           GFP_KERNEL);
        }
 
-       rcu_assign_sk_user_data(sk, ovpn_sock);
 sock_release:
        release_sock(sk);
        return ovpn_sock;

diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 0d7f30360d8746b53f6b75770f0a27f522de9233..f0b4e07ba9245ae0df501bb6ed522b25d5a85f8a 100644 (file)
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -487,6 +487,7 @@ 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;
+       rcu_assign_sk_user_data(ovpn_sock->sk, ovpn_sock);
 
        /* only a fully connected socket is expected. Connection should be
         * handled in userspace
@@ -495,13 +496,14 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
                net_err_ratelimited("%s: provided TCP socket is not in ESTABLISHED state: %d\n",
                                    netdev_name(peer->ovpn->dev),
                                    ovpn_sock->sk->sk_state);
-               return -EINVAL;
+               ret = -EINVAL;
+               goto err;
        }
 
        ret = strp_init(&peer->tcp.strp, ovpn_sock->sk, &cb);
        if (ret < 0) {
                DEBUG_NET_WARN_ON_ONCE(1);
-               return ret;
+               goto err;
        }
 
        INIT_WORK(&peer->tcp.defer_del_work, ovpn_tcp_peer_del_work);
@@ -536,6 +538,9 @@ int ovpn_tcp_socket_attach(struct ovpn_socket *ovpn_sock,
        strp_check_rcv(&peer->tcp.strp);
 
        return 0;
+err:
+       rcu_assign_sk_user_data(ovpn_sock->sk, NULL);
+       return ret;
 }
 
 static void ovpn_tcp_close(struct sock *sk, long timeout)

diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index d6a0f7a0b75d74c3615c826d33cc18b67077d608..272b535ecaad4c75ff2aa9d4589b4cfa36a0dea9 100644 (file)
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -386,6 +386,7 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock, struct socket *sock,
                           struct ovpn_priv *ovpn)
 {
        struct udp_tunnel_sock_cfg cfg = {
+               .sk_user_data = ovpn_sock,
                .encap_type = UDP_ENCAP_OVPNINUDP,
                .encap_rcv = ovpn_udp_encap_recv,
                .encap_destroy = ovpn_udp_encap_destroy,