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: <7a3909c1-818d-4701-b3b3-012976db7a34@openvpn.net>
Date: Wed, 15 May 2024 21:44:44 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.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

On 15/05/2024 16:55, Sabrina Dubroca wrote:
> 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.

you're right. I had forgotten a little but important detail.

With UDP OpenVPN creates one socket and uses it for all peers.
With TCP we forcefully need one socket per client.

Consequently, when a UDP socket is found to be used by our own instance, 
  we can happily increase the refcounter and use it as if it was free 
(we are just attaching it to yet another peer).

In TCP this is not possible, so the socket must be unused, otherwise we 
can't attach it.

I hope it makes sense.

> 

-- 
Antonio Quartulli
OpenVPN Inc.