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: <faddc1f2-1637-4e72-a130-a6ccd4cc4449@openvpn.net>
Date: Wed, 15 May 2024 22:39:05 +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 22:35, Sabrina Dubroca wrote:
> 2024-05-15, 21:44:44 +0200, Antonio Quartulli wrote:
>> 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.
> 
> Yes, thanks. This behavior should be documented (for example, by
> putting exactly what you just wrote in a comment above
> ovpn_socket_new).

absolutely, will do.

> 
> So for TCP you just need the existing check and EBUSY return. For UDP,
> you need the EALREADY check, but with an extra encap_type test before
> looking at the contents of the sk_user_data.

ACK

-- 
Antonio Quartulli
OpenVPN Inc.