Hi,

On 15/07/2024 11:59, Sabrina Dubroca wrote:

2024-06-27, 15:08:32 +0200, Antonio Quartulli wrote:

quoted

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 0475440642dd..764b3df996bc 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c

@@ -21,6 +21,7 @@
  #include "netlink.h"
  #include "proto.h"
  #include "socket.h"
+#include "tcp.h"
  #include "udp.h"
  #include "skb.h"
  

@@ -84,8 +85,11 @@ void ovpn_decrypt_post(struct sk_buff *skb, int ret)
  	/* PID sits after the op */
  	pid = (__force __be32 *)(skb->data + OVPN_OP_SIZE_V2);
  	ret = ovpn_pktid_recv(&ks->pid_recv, ntohl(*pid), 0);
-	if (unlikely(ret < 0))
+	if (unlikely(ret < 0)) {
+		net_err_ratelimited("%s: PKT ID RX error: %d\n",
+				    peer->ovpn->dev->name, ret);

nit: this should be part of the "packet processing" patch?

Yap, makes sense.

quoted

diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index dd4d91dfabb5..86d4696b1529 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h

@@ -10,8 +10,8 @@
  #ifndef _NET_OVPN_OVPNPEER_H_
  #define _NET_OVPN_OVPNPEER_H_
  
-#include <linux/ptr_ring.h>

nit: I think you don't need it at all in this version and forgot to
drop it in a previous patch? (I didn't notice when it was introduced)

Ouch, you are correct

quoted

+static int ovpn_tcp_to_userspace(struct ovpn_socket *sock, struct sk_buff *skb)
+{
+	struct sock *sk = sock->sock->sk;
+
+	skb_set_owner_r(skb, sk);
+	memset(skb->cb, 0, sizeof(skb->cb));

nit: this was just done in ovpn_tcp_rcv

right!

quoted

+	skb_queue_tail(&sock->peer->tcp.user_queue, skb);
+	sock->peer->tcp.sk_cb.sk_data_ready(sk);
+
+	return 0;
+}
+
+static void ovpn_tcp_rcv(struct strparser *strp, struct sk_buff *skb)
+{

[...]

quoted

+	/* DATA_V2 packets are handled in kernel, the rest goes to user space */
+	if (likely(ovpn_opcode_from_skb(skb, 0) == OVPN_DATA_V2)) {
+		/* hold reference to peer as required by ovpn_recv().
+		 *
+		 * NOTE: in this context we should already be holding a
+		 * reference to this peer, therefore ovpn_peer_hold() is
+		 * not expected to fail
+		 */
+		WARN_ON(!ovpn_peer_hold(peer));

drop the packet if this fails? otherwise I suspect we'll crash later on.

yeah, jumping to "err" and dropping everything makes sense.

quoted

+		ovpn_recv(peer, skb);
+	} else {
+		/* The packet size header must be there when sending the packet
+		 * to userspace, therefore we put it back
+		 */
+		skb_push(skb, 2);
+		memset(skb->cb, 0, sizeof(skb->cb));
+		if (ovpn_tcp_to_userspace(peer->sock, skb) < 0) {
+			net_warn_ratelimited("%s: cannot send skb to userspace\n",
+					     peer->ovpn->dev->name);
+			goto err;
+		}
+	}

[...]

quoted

+void ovpn_tcp_socket_detach(struct socket *sock)
+{
+	struct ovpn_socket *ovpn_sock;
+	struct ovpn_peer *peer;
+
+	if (!sock)
+		return;
+
+	rcu_read_lock();
+	ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
+

[...]

quoted

+	/* cancel any ongoing work. Done after removing the CBs so that these
+	 * workers cannot be re-armed
+	 */
+	cancel_work_sync(&peer->tcp.tx_work);

I don't think that's ok to call under rcu_read_lock, it seems it can
sleep.

quoted

+	strp_done(&peer->tcp.strp);

And same here, since strp_done also calls cancel_work_sync.

hm you're right. I'll see how to re-arrange this part..I expect this to 
be tricky.

quoted

+	rcu_read_unlock();
+}
+
+static void ovpn_tcp_send_sock(struct ovpn_peer *peer)
+{
+	struct sk_buff *skb = peer->tcp.out_msg.skb;
+
+	if (!skb)
+		return;
+
+	if (peer->tcp.tx_in_progress)
+		return;
+
+	peer->tcp.tx_in_progress = true;

I'm not convinced this is safe. ovpn_tcp_send_sock could run
concurrently for the same peer (lock_sock doesn't exclude bh_lock_sock
after the short "grab ownership" phase), so I think both sides could
see tx_in_progress = false and then proceed.

I may be missing something here.
I was under the impression that ovpn_tcp_send_sock() is always invoked 
with lock_sock() held. Shouldn't that be enough to prevent concurrent 
executions for the same peer/sock?

quoted

+	do {
+		int ret = skb_send_sock_locked(peer->sock->sock->sk, skb,
+					       peer->tcp.out_msg.offset,
+					       peer->tcp.out_msg.len);
+		if (unlikely(ret < 0)) {
+			if (ret == -EAGAIN)
+				goto out;

This will silently drop the message? And then in case of a userspace
message, ovpn_tcp_sendmsg will lie to the user (the openvpn client),
claiming that the control message was sent (ret = size just above the
unlock)?

why do you think the message will be dropped?

By jumping to 'out' we are keeping the skb in peer->tcp.out_msg.skb, 
with peer->tcp.out_msg.offset and peer->tcp.out_msg.len left untouched 
and ready for the next attempt triggered by ovpn_tcp_write_space().

quoted

+
+			net_warn_ratelimited("%s: TCP error to peer %u: %d\n",
+					     peer->ovpn->dev->name, peer->id,
+					     ret);
+
+			/* in case of TCP error we can't recover the VPN
+			 * stream therefore we abort the connection
+			 */
+			ovpn_peer_del(peer,
+				      OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
+			break;
+		}
+
+		peer->tcp.out_msg.len -= ret;
+		peer->tcp.out_msg.offset += ret;
+	} while (peer->tcp.out_msg.len > 0);

Another thing that worries me: assume the receiver is a bit slow, the
underlying TCP socket gets stuck. skb_send_sock_locked manages to push
some data down the TCP socket, but not everything. We advance by that
amount, and restart this loop. The socket is still stuck, so
skb_send_sock_locked returns -EAGAIN. We have only pushed a partial
message down to the TCP socket, but we drop the rest? Now the stream
is broken, and the next call to ovpn_tcp_send_sock will happily send
its message.

I think this is answered above, where I say that we are actually keeping 
the skb (not dropping it) ready for the next sending attempt.

ovpn_tcp_send_sock with msg_len = 1000
iteration 1
   skb_send_sock_locked returns 100
   advance
iteration 2
   skb_send_sock_locked returns -EAGAIN
   goto out


So you'd have to keep that partially-sent message around until you can
finish pushing it out on the socket.

yap, see above.

[...]

quoted

+static int ovpn_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
+{
+	struct ovpn_socket *sock;
+	int ret, linear = PAGE_SIZE;
+	struct ovpn_peer *peer;
+	struct sk_buff *skb;
+
+	rcu_read_lock();
+	sock = rcu_dereference_sk_user_data(sk);
+	peer = sock->peer;
+	rcu_read_unlock();

What's stopping the peer being freed here?

I assumed that while we are in our own sk_cb it should not be possible 
for a peer to have refcnt reaching 0.

But after double checking I don't think there is any protection about 
this. I Will add a call to ovpn_peer_hold() and abort if that call fails.

-- 
Antonio Quartulli
OpenVPN Inc.