On 14/05/2024 10:58, Sabrina Dubroca wrote:

quoted

quoted

quoted

diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index b5ff59a4b40f..ac4907705d98 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h
+ * @tcp.raw_len: next packet length as read from the stream (TCP only)
+ * @tcp.skb: next packet being filled with data from the stream (TCP only)
+ * @tcp.offset: position of the next byte to write in the skb (TCP only)
+ * @tcp.data_len: next packet length converted to host order (TCP only)

It would be nice to add information about whether they're used for TX or RX.

they are all about "from the stream" and "to the skb", meaning that we are
doing RX.
Will make it more explicit.

Maybe group them in a struct rx?

yap, makes sense.

quoted

quoted

quoted

+ * @tcp.sk_cb.sk_data_ready: pointer to original cb
+ * @tcp.sk_cb.sk_write_space: pointer to original cb
+ * @tcp.sk_cb.prot: pointer to original prot object
    * @crypto: the crypto configuration (ciphers, keys, etc..)
    * @dst_cache: cache for dst_entry used to send to peer
    * @bind: remote peer binding

@@ -59,6 +69,25 @@ struct ovpn_peer {
   	struct ptr_ring netif_rx_ring;
   	struct napi_struct napi;
   	struct ovpn_socket *sock;
+	/* state of the TCP reading. Needed to keep track of how much of a
+	 * single packet has already been read from the stream and how much is
+	 * missing
+	 */
+	struct {
+		struct ptr_ring tx_ring;
+		struct work_struct tx_work;
+		struct work_struct rx_work;
+
+		u8 raw_len[sizeof(u16)];

Why not u16 or __be16 for this one?

because in this array we are putting the bytes as we get them from the
stream.
We may be at the point where one out of two bytes is available on the
stream. For this reason I use an array to store this u16 byte by byte.

Once thw two bytes are ready, we convert the content in an actual int and
store it in "data_len" (a few lines below).

Ok, I see. Hopefully you can switch to strparser and make this one go
away.

quoted

quoted

quoted

diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index e099a61b03fa..004db5b13663 100644
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c

@@ -16,6 +16,7 @@
   #include "packet.h"
   #include "peer.h"
   #include "socket.h"
+#include "tcp.h"
   #include "udp.h"
   /* Finalize release of socket, called after RCU grace period */

@@ -26,6 +27,8 @@ static void ovpn_socket_detach(struct socket *sock)
   	if (sock->sk->sk_protocol == IPPROTO_UDP)
   		ovpn_udp_socket_detach(sock);
+	else if (sock->sk->sk_protocol == IPPROTO_TCP)
+		ovpn_tcp_socket_detach(sock);
   	sockfd_put(sock);
   }

@@ -69,6 +72,8 @@ static int ovpn_socket_attach(struct socket *sock, struct ovpn_peer *peer)
   	if (sock->sk->sk_protocol == IPPROTO_UDP)
   		ret = ovpn_udp_socket_attach(sock, peer->ovpn);
+	else if (sock->sk->sk_protocol == IPPROTO_TCP)
+		ret = ovpn_tcp_socket_attach(sock, peer);
   	return ret;
   }

@@ -124,6 +129,21 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
   	ovpn_sock->sock = sock;

The line above this is:

      ovpn_sock->ovpn = peer->ovpn;

It's technically fine since you then overwrite this with peer in case
we're on TCP, but ovpn_sock->ovpn only exists on UDP since you moved
it into a union in this patch.

Yeah, I did not want to make another branch, but having a UDP specific case
will make code easier to read.

Either that, or drop the union.

ACK

quoted

quoted

quoted

diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
new file mode 100644
index 000000000000..84ad7cd4fc4f
--- /dev/null
+++ b/drivers/net/ovpn/tcp.c

@@ -0,0 +1,511 @@
+static int ovpn_tcp_read_sock(read_descriptor_t *desc, struct sk_buff *in_skb,
+			      unsigned int in_offset, size_t in_len)
+{
+	struct sock *sk = desc->arg.data;
+	struct ovpn_socket *sock;
+	struct ovpn_skb_cb *cb;
+	struct ovpn_peer *peer;
+	size_t chunk, copied = 0;
+	void *data;
+	u16 len;
+	int st;
+
+	rcu_read_lock();
+	sock = rcu_dereference_sk_user_data(sk);
+	rcu_read_unlock();

You can't just release rcu_read_lock and keep using sock (here and in
the rest of this file). Either you keep rcu_read_lock, or you can take
a reference on the ovpn_socket.

I was just staring at this today, after having worked on the
rcu_read_lock/unlock for the peer get()s..

I thinkt the assumption was: if we are in this read_sock callback, it's
impossible that the ovpn_socket was invalidated, because it gets invalidated
upon detach, which also prevents any further calling of this callback. But
this sounds racy, and I guess we should somewhat hold a reference..

ovpn_tcp_read_sock starts

detach
kfree_rcu(ovpn_socket)
...
ovpn_socket actually freed
...
ovpn_tcp_read_sock continues with freed ovpn_socket


I don't think anything in the current code prevents this.

mh yeah, if something like this happens right after having started the 
read_sock we are doomed.
Will fix this.

quoted

quoted

quoted

+/* Set TCP encapsulation callbacks */
+int ovpn_tcp_socket_attach(struct socket *sock, struct ovpn_peer *peer)
+{
+	void *old_data;
+	int ret;
+
+	INIT_WORK(&peer->tcp.tx_work, ovpn_tcp_tx_work);
+
+	ret = ptr_ring_init(&peer->tcp.tx_ring, OVPN_QUEUE_LEN, GFP_KERNEL);
+	if (ret < 0) {
+		netdev_err(peer->ovpn->dev, "cannot allocate TCP TX ring\n");
+		return ret;
+	}
+
+	peer->tcp.skb = NULL;
+	peer->tcp.offset = 0;
+	peer->tcp.data_len = 0;
+
+	write_lock_bh(&sock->sk->sk_callback_lock);
+
+	/* make sure no pre-existing encapsulation handler exists */
+	rcu_read_lock();
+	old_data = rcu_dereference_sk_user_data(sock->sk);
+	rcu_read_unlock();
+	if (old_data) {
+		netdev_err(peer->ovpn->dev,
+			   "provided socket already taken by other user\n");
+		ret = -EBUSY;
+		goto err;

The UDP code differentiates "socket already owned by this interface"
from "already taken by other user". That doesn't apply to TCP?

This makes me wonder: how safe it is to interpret the user data as an object
of type ovpn_socket?

When we find the user data already assigned, we don't know what was really
stored in there, right?
Technically this socket could have gone through another module which
assigned its own state.

Therefore I think that what UDP does [ dereferencing ((struct ovpn_socket
*)user_data)->ovpn ] is probably not safe. Would you agree?

Hmmm, yeah, I think you're right. If you checked encap_type ==
UDP_ENCAP_OVPNINUDP before (sk_prot for TCP), then you'd know it's
really your data. Basically call ovpn_from_udp_sock during attach if
you want to check something beyond EBUSY.

right. Maybe we can leave with simply reporting EBUSY and be done with 
it, without adding extra checks and what not.

Once you're in your own callbacks, it should be safe. If some other
code sends packet with a non-ovpn socket to ovpn's ->encap_rcv,
something is really broken.

yup

quoted

quoted

quoted

+int __init ovpn_tcp_init(void)
+{
+	/* We need to substitute the recvmsg and the sock_is_readable
+	 * callbacks in the sk_prot member of the sock object for TCP
+	 * sockets.
+	 *
+	 * However sock->sk_prot is a pointer to a static variable and
+	 * therefore we can't directly modify it, otherwise every socket
+	 * pointing to it will be affected.
+	 *
+	 * For this reason we create our own static copy and modify what
+	 * we need. Then we make sk_prot point to this copy
+	 * (in ovpn_tcp_socket_attach())
+	 */
+	ovpn_tcp_prot = tcp_prot;

Don't you need a separate variant for IPv6, like TLS does?

Never did so far.

My wild wild wild guess: for the time this socket is owned by ovpn, we only
use callbacks that are IPvX agnostic, hence v4 vs v6 doesn't make any
difference.
When this socket is released, we reassigned the original prot.

That seems a bit suspicious to me. For example, tcpv6_prot has a
different backlog_rcv. And you don't control if the socket is detached
before being closed, or which callbacks are needed. Your userspace
client doesn't use them, but someone else's might.

quoted

quoted

quoted

+	ovpn_tcp_prot.recvmsg = ovpn_tcp_recvmsg;

You don't need to replace ->sendmsg as well? The userspace client is
not expected to send messages?

It is, but my assumption is that those packets will just go through the
socket as usual. No need to be handled by ovpn (those packets are not
encrypted/decrypted, like data traffic is).
And this is how it has worked so far.

Makes sense?

Two things come to mind:

- userspace is expected to prefix the messages it inserts on the
   stream with the 2-byte length field? otherwise, the peer won't be
   able to parse them out of the stream

correct. userspace sends those packets as if ovpn is not running, 
therefore this happens naturally.

- I'm not convinced this would be safe wrt kernel writing partial
   messages. if ovpn_tcp_send_one doesn't send the full message, you
   could interleave two messages:

   +------+-------------------+------+--------+----------------+
   | len1 | (bytes from msg1) | len2 | (msg2) | (rest of msg1) |
   +------+-------------------+------+--------+----------------+

   and the RX side would parse that as:

   +------+-----------------------------------+------+---------
   | len1 | (bytes from msg1) | len2 | (msg2) | ???? | ...
   +------+-------------------+---------------+------+---------

   and try to interpret some random bytes out of either msg1 or msg2 as
   a length prefix, resulting in a broken stream.

hm you are correct. if multiple sendmsg can overlap, then we might be in 
troubles, but are we sure this can truly happen?

The stream format looks identical to ESP in TCP [1] (2B length prefix
followed by the actual message), so I think the espintcp code (both tx
and rx, except for actual protocol parsing) should look very
similar. The problems that need to be solved for both protocols are
pretty much the same.

ok, will have a look. maybe this will simplify the code even more and we 
will get rid of some of the issues we were discussing above.

Thanks!

[1] https://www.rfc-editor.org/rfc/rfc8229#section-3

-- 
Antonio Quartulli
OpenVPN Inc.