netdev - Re: [PATCH net-next v2 03/22] ovpn: add basic netlink support

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: <e0375bdb-8ef8-4a46-a5cf-351d77840874@lunn.ch>
Date: Mon, 4 Mar 2024 22:20:38 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Antonio Quartulli <antonio@...nvpn.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>
Subject: Re: [PATCH net-next v2 03/22] ovpn: add basic netlink support

On Mon, Mar 04, 2024 at 04:08:54PM +0100, Antonio Quartulli wrote:
> This commit introduces basic netlink support with
> registration/unregistration functionalities and stub pre/post-doit.
> 
> More importantly it introduces the UAPI header file that contains
> the attributes that are inteded to be used by the netlink API

intended.

> implementation.
> 
> For convience, packet.h is also added containing some macros about
> the OpenVPN packet format.
> 
> +/** KEYDIR policy. Can be used for configuring an encryption and a decryption key */
> +static const struct nla_policy ovpn_nl_policy_keydir[NUM_OVPN_A_KEYDIR] = {
> +	[OVPN_A_KEYDIR_CIPHER_KEY] = NLA_POLICY_MAX_LEN(U8_MAX),

I don't know netlink that well. Is this saying keys are limited to 256
bytes? How future proof is that? I'm not a crypto person, but
symmetric algorithms, e.g. AES, seem to have reasonably short keys, 32
bytes, so this seems O.K, to me.

> +	[OVPN_A_KEYDIR_NONCE_TAIL] = NLA_POLICY_EXACT_LEN(NONCE_TAIL_SIZE),
> +};
> +
> +/** KEYCONF policy */
> +static const struct nla_policy ovpn_nl_policy_keyconf[NUM_OVPN_A_KEYCONF] = {
> +	[OVPN_A_KEYCONF_SLOT] = NLA_POLICY_RANGE(NLA_U8, __OVPN_KEY_SLOT_FIRST,
> +						 NUM_OVPN_KEY_SLOT - 1),
> +	[OVPN_A_KEYCONF_KEY_ID] = { .type = NLA_U8 },

Is that 256 keys globally, or just associated to one session?

> +	[OVPN_A_KEYCONF_CIPHER_ALG] = { .type = NLA_U16 },
> +	[OVPN_A_KEYCONF_ENCRYPT_DIR] = NLA_POLICY_NESTED(ovpn_nl_policy_keydir),
> +	[OVPN_A_KEYCONF_DECRYPT_DIR] = NLA_POLICY_NESTED(ovpn_nl_policy_keydir),
> +};
> +

> +/** Generic message container policy */
> +static const struct nla_policy ovpn_nl_policy[NUM_OVPN_A] = {
> +	[OVPN_A_IFINDEX] = { .type = NLA_U32 },
> +	[OVPN_A_IFNAME] = NLA_POLICY_MAX_LEN(IFNAMSIZ),

Generally, ifnames are not passed around, only ifindex. An interface
can have multiple names:

12: enlr0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq master br0 state DOWN group default qlen 1000
    link/ether 3c:ec:ef:7e:0a:90 brd ff:ff:ff:ff:ff:ff
    altname enp183s0f2
    altname eno7

It is better to let userspace figure out the name from the index,
since the name is mostly a user space concept.

> +	[OVPN_A_MODE] = NLA_POLICY_RANGE(NLA_U8, __OVPN_MODE_FIRST,
> +					 NUM_OVPN_MODE - 1),
> +	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_nl_policy_peer),
> +};

> +static int ovpn_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
> +			 struct genl_info *info)
> +{
> +	struct net *net = genl_info_net(info);
> +	struct net_device *dev;
> +
> +	/* the OVPN_CMD_NEW_IFACE command is different from the rest as it
> +	 * just expects an IFNAME, while all the others expect an IFINDEX
> +	 */

Could you explain that some more. In general, the name should not
matter to the kernel, udev/systemd might rename it soon after creation
etc. If it gets moved into a network namespace it might need renaming
etc.

> +enum ovpn_nl_peer_attrs {
> +	OVPN_A_PEER_UNSPEC = 0,
> +	OVPN_A_PEER_ID,
> +	OVPN_A_PEER_RX_STATS,
> +	OVPN_A_PEER_TX_STATS,

Probably answered later in the patch series: What sort of statistics
do you expect here. Don't overlap any of the normal network statistics
with this here, please use the existing kAPIs for those. Anything you
return here need to be very specific to ovpn.

> +	OVPN_A_PEER_VPN_RX_BYTES,
> +	OVPN_A_PEER_VPN_TX_BYTES,
> +	OVPN_A_PEER_VPN_RX_PACKETS,
> +	OVPN_A_PEER_VPN_TX_PACKETS,
> +	OVPN_A_PEER_LINK_RX_BYTES,
> +	OVPN_A_PEER_LINK_TX_BYTES,
> +	OVPN_A_PEER_LINK_RX_PACKETS,
> +	OVPN_A_PEER_LINK_TX_PACKETS,

How do these differ to standard network statistics? e.g. what is in
/sys/class/net/*/statistics/ ?

	Andrew