Re: [PATCH net-next v3 13/24] ovpn: implement TCP transport

[Sabrina Dubroca <sd@queasysnail.net>]

2024-05-15, 00:11:28 +0200, Antonio Quartulli wrote:
> On 14/05/2024 10:58, Sabrina Dubroca wrote:
> > > > 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.

I don't know. What was the reason for the EALREADY handling in udp.c
and the corresponding refcount increase in ovpn_socket_new?


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

ok.


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

What would prevent this? The kernel_sendmsg call in ovpn_tcp_send_one
could send a partial message, and then what would stop userspace from
sending its own message during the cond_resched from ovpn_tcp_tx_work?

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

I doubt dealing with possible interleaving will make the code simpler,
but I think it has to be done.

-- 
Sabrina