netdev - Re: [PATCH net-next v11 04/23] ovpn: add basic interface creation/destruction/management routines

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: <353a7ffd-852a-4da6-b382-ea1714c69dc1@openvpn.net>
Date: Fri, 15 Nov 2024 14:45:41 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>,
 Sabrina Dubroca <sd@...asysnail.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>, 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 04/23] ovpn: add basic interface
 creation/destruction/management routines

On 14/11/2024 23:57, Sergey Ryazanov wrote:
> On 14.11.2024 10:07, Antonio Quartulli wrote:
>> On 12/11/2024 17:47, Sabrina Dubroca wrote:
>>> 2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote:
>>>> On 29.10.2024 12:47, Antonio Quartulli wrote:
>>>>> +/* When the OpenVPN protocol is ran in AEAD mode, use
>>>>> + * the OpenVPN packet ID as the AEAD nonce:
>>>>> + *
>>>>> + *    00000005 521c3b01 4308c041
>>>>> + *    [seq # ] [  nonce_tail   ]
>>>>> + *    [     12-byte full IV    ] -> NONCE_SIZE
>>>>> + *    [4-bytes                   -> NONCE_WIRE_SIZE
>>>>> + *    on wire]
>>>>> + */
>>>>
>>>> Nice diagram! Can we go futher and define the OpenVPN packet header 
>>>> as a
>>>> stucture? Referencing the structure instead of using magic sizes and 
>>>> offsets
>>>> can greatly improve the code readability. Especially when it comes 
>>>> to header
>>>> construction/parsing in the encryption/decryption code.
>>>>
>>>> E.g. define a structures like this:
>>>>
>>>> struct ovpn_pkt_hdr {
>>>>    __be32 op;
>>>>    __be32 pktid;
>>>>    u8 auth[];
>>>> } __attribute__((packed));
>>>>
>>>> struct ovpn_aead_iv {
>>>>    __be32 pktid;
>>>>    u8 nonce[OVPN_NONCE_TAIL_SIZE];
>>>> } __attribute__((packed));
>>>
>>> __attribute__((packed)) should not be needed here as the fields in
>>> both structs look properly aligned, and IIRC using packed can cause
>>> the compiler to generate worse code.
>>
>> Agreed. Using packed will make certain architecture read every field 
>> byte by byte (I remember David M. biting us on this in batman-adv :))
> 
> Still curious to see an example of that strange architecture/compiler 
> combination. Anyway, as Sabrina mentioned, the header is already pretty 
> aligned. So it's up to you how to document the structure.

IIRC MIPS was one of those, but don't take my word for granted.

> 
>> This said, I like the idea of using a struct, but I don't feel 
>> confident enough to change the code now that we are hitting v12.
>> This kind of change will be better implemented later and tested 
>> carefully. (and patches are always welcome! :))
> 
> The main reason behind the structure introduction is to improve the code 
> readability. To reduce a shadow, where bugs can reside. I wonder how 
> many people have invested their time to dig through the encryption 
> preparation function?
> 
> As for risk of breaking something I should say that it can be addressed 
> by connecting the kernel implementation to pure usespace implementation, 
> which can be assumed the reference. And, I believe, it worth the benefit 
> of merging easy to understand code.
> 

I understand your point, but this is something I need to spend time on 
because the openvpn packet format is not "very stable", as in "it can 
vary depending on negotiated features".

When implementing ovpn I decided what was the supported set of features 
so to create a stable packet header, but this may change moving forward 
(there is already some work going on in userspace regarding new features 
that ovpn will have to support).
Therefore I want to take some time thinking about what's best.

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.