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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZkTM9b8oU8Rw31Qp@hog>
Date: Wed, 15 May 2024 16:55:49 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	Sergey Ryazanov <ryazanov.s.a@...il.com>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew@...n.ch>, Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 13/24] ovpn: implement TCP transport

2024-05-15, 14:54:49 +0200, Antonio Quartulli wrote:
> On 15/05/2024 12:19, Sabrina Dubroca wrote:
> > 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?
> 
> it's just me that likes to be verbose when doing error reporting.

With the "already owned by this interface" message? Sure, I get that.

> But eventually the exact error is ignored and we release the reference. From
> netlink.c:
> 
> 342                 peer->sock = ovpn_socket_new(sock, peer);
> 343                 if (IS_ERR(peer->sock)) {
> 344                         sockfd_put(sock);
> 345                         peer->sock = NULL;
> 346                         ret = -ENOTSOCK;
> 
> so no added value in distinguishing the two cases.

But ovpn_socket_new currently turns EALREADY into a valid result, so
we won't go through the error hanadling here. That's the part I'm
unclear about.

-- 
Sabrina