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

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

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)

+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

+	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)
+{

[...]

+	/* 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.

+		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;
+		}
+	}

[...]

+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);
+

[...]

+	/* 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.

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

And same here, since strp_done also calls cancel_work_sync.

+	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.

+	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)?

+
+			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.

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.


[...]

+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?

-- 
Sabrina