linux-kernel - 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: <7ba4b3a0-06ca-4273-aaf0-19f92b4de3e9@gmail.com>
Date: Fri, 15 Nov 2024 00:57:09 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>,
 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 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.

> 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.

--
Sergey