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: <ZzOGqP9AAGSN2E7y@hog>
Date: Tue, 12 Nov 2024 17:47:36 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Antonio Quartulli <antonio@...nvpn.net>,
	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

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.


> > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> > index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644
> > --- a/include/uapi/linux/if_link.h
> > +++ b/include/uapi/linux/if_link.h
> > @@ -1975,4 +1975,19 @@ enum {
> >   #define IFLA_DSA_MAX	(__IFLA_DSA_MAX - 1)
> > +/* OVPN section */
> > +
> > +enum ovpn_mode {
> > +	OVPN_MODE_P2P,
> > +	OVPN_MODE_MP,
> > +};
> 
> Mode min/max values can be defined here and the netlink policy can reference
> these values:
> 
> enum ovpn_mode {
>   OVPN_MODE_P2P,
>   OVPN_MODE_MP,
>   __OVPN_MODE_MAX
> };
> 
> #define OVPN_MODE_MIN OVPN_MODE_P2P
> #define OVPN_MODE_MAX (__OVPN_MODE_MAX - 1)
> 
> ... = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_MIN, OVPN_MODE_MAX)

I don't think there's much benefit to that, other than making the diff
smaller on a (very unlikely) patch that would add a new mode in the
future. It even looks more inconvenient to me when reading the code
("ok what are _MIN and _MAX?  the code is using _P2P and _MP, do they
match?").

-- 
Sabrina