linux-kernel - Re: [PATCH net-next v11 07/23] ovpn: introduce the ovpn

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: <ca5c4c4b-bd9b-4ccc-9258-e78ec7684a85@gmail.com>
Date: Thu, 21 Nov 2024 01:58:58 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 07/23] ovpn: introduce the ovpn_socket object

On 15.11.2024 16:28, Antonio Quartulli wrote:
> On 10/11/2024 19:26, Sergey Ryazanov wrote:
>> On 29.10.2024 12:47, Antonio Quartulli wrote:

[...]

>>> +static bool ovpn_socket_hold(struct ovpn_socket *sock)
>>> +{
>>> +    return kref_get_unless_zero(&sock->refcount);
>>
>> Why do we need to wrap this kref acquiring call into the function. Why 
>> we cannot simply call kref_get_unless_zero() from ovpn_socket_get()?
> 
> Generally I prefer to keep the API among objects consistent.
> In this specific case, it means having hold() and put() helpers in order 
> to avoid calling kref_* functions directly in the code.
> 
> This is a pretty simple case because hold() is called only once, but I 
> still like to be consistent.

Make sense. The counterpart ovpn_socket_hold() function declared in the 
header file. Probably that's why I missed it. Shall we move the holding 
routine there as well?

[...]

>>> +int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct 
>>> *ovpn)
>>> +{
>>> +    struct ovpn_socket *old_data;
>>> +    int ret = 0;
>>> +
>>> +    /* sanity check */
>>> +    if (sock->sk->sk_protocol != IPPROTO_UDP) {
>>
>> The function will be called only for a UDP socket. The caller makes 
>> sure this is truth. So, why do we need this check?
> 
> To avoid this function being copied/called somewhere else in the future 
> and we forget about this critical assumption.

Shall we do the same for all other functions in this file? E.g. 
ovpn_udp_socket_detach/ovpn_udp_send_skb? And who is giving guarantee 
that the code will be copied together with the check?

> Indeed it's a just sanity check.

Shall we check for pointers validity before dereferencing them?

if (!ovpn || !sock || !sock->sk || !sock->sk->sk_protocol != IPPROTO_UDP) {

With the above questions I would like to show that it's endless number 
of possible mistakes. And no matter how much do we check, a creative 
engineer will find a way to ruin the kernel.

So, is it worth to spend code lines for checking socket for being UDP 
inside a function that has '_udp_' in its name and is called only inside 
the module?

>>> +        DEBUG_NET_WARN_ON_ONCE(1);
>>> +        return -EINVAL;
>>> +    }

--
Sergey