linux-kernel - Re: [PATCH v21 20/24] ovpn: implement key add/get/del/swap via netlink

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: <07c73e1d-3c9c-46c7-92cd-28d728929d18@openvpn.net>
Date: Tue, 4 Mar 2025 13:11:28 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, 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>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH v21 20/24] ovpn: implement key add/get/del/swap via
 netlink

On 04/03/2025 13:00, Sabrina Dubroca wrote:
> 2025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
>>   int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
> ...
>> +	pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
>> +	pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
>> +	pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
> 
> 
> [...]
>> +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
>> +			    u32 peer_id, enum ovpn_key_slot slot,
>> +			    const struct ovpn_key_config *keyconf)
>> +{
> ...
>> +	if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
>> +	    nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
>> +	    nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
> 
> That's a bit inconsistent. nla_put_u32 matches the generated policy,
> but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
> key_id" it's getting stored into).
> 
> [also kind of curious that the policy/spec uses U32 with max values of 1/2/7]

 From 
https://www.kernel.org/doc/html/next/userspace-api/netlink/specs.html#fix-width-integer-types

"Note that types smaller than 32 bit should be avoided as using them 
does not save any memory in Netlink messages (due to alignment)."

Hence I went for u32 attributes, although values stored into them are 
much smaller.

> 
> 
> 
>>   int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
> ...
>> +	slot = nla_get_u32(attrs[OVPN_A_KEYCONF_SLOT]);
> 
> 
> 
>>   int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
> ...
>> +	slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);

Right. Since actual values are smaller I sometimes used the smaller macro.

I guess I better convert them all to u32.

> 
> 
> 
> A few more inconsistencies:
> 
> * OVPN_A_IFNAME is defined in the uapi but never used (I guess
>    leftover from genl link creation)

Ouch, I guess you're right about it being a leftover.

> 
> * OVPN_A_PEER_DEL_REASON (u32 vs u8)
> drivers/net/ovpn/netlink-gen.c:52:      [OVPN_A_PEER_DEL_REASON] = NLA_POLICY_MAX(NLA_U32, 4),
> drivers/net/ovpn/netlink.c:1131:        if (nla_put_u8(msg, OVPN_A_PEER_DEL_REASON, peer->delete_reason))

Like above, probably better to convert them all to u32.

> 
> * OVPN_A_PEER_LINK_TX_PACKETS/OVPN_A_PEER_LINK_RX_PACKETS (uint vs u32):
> drivers/net/ovpn/netlink-gen.c-57-      [OVPN_A_PEER_LINK_RX_BYTES] = { .type = NLA_UINT, },
> drivers/net/ovpn/netlink-gen.c-58-      [OVPN_A_PEER_LINK_TX_BYTES] = { .type = NLA_UINT, },
> drivers/net/ovpn/netlink-gen.c-59-      [OVPN_A_PEER_LINK_RX_PACKETS] = { .type = NLA_U32, },
> drivers/net/ovpn/netlink-gen.c:60:      [OVPN_A_PEER_LINK_TX_PACKETS] = { .type = NLA_U32, },
> --
> drivers/net/ovpn/netlink.c-618-     /* link RX stats */
> drivers/net/ovpn/netlink.c-619-     nla_put_uint(skb, OVPN_A_PEER_LINK_RX_BYTES,
> drivers/net/ovpn/netlink.c-620-                  atomic64_read(&peer->link_stats.rx.bytes)) ||
> drivers/net/ovpn/netlink.c-621-     nla_put_uint(skb, OVPN_A_PEER_LINK_RX_PACKETS,
> drivers/net/ovpn/netlink.c-622-                  atomic64_read(&peer->link_stats.rx.packets)) ||
> drivers/net/ovpn/netlink.c-623-     /* link TX stats */
> drivers/net/ovpn/netlink.c-624-     nla_put_uint(skb, OVPN_A_PEER_LINK_TX_BYTES,
> drivers/net/ovpn/netlink.c-625-                  atomic64_read(&peer->link_stats.tx.bytes)) ||
> drivers/net/ovpn/netlink.c:626:     nla_put_uint(skb, OVPN_A_PEER_LINK_TX_PACKETS,
> drivers/net/ovpn/netlink.c-627-                  atomic64_read(&peer->link_stats.tx.packets)))
> 
> I guess all the stats should be UINT.

u32 should be enough for packets - but yeah, going UINT is better as I 
don't need to care if it really fits 4B or not.

Will do.

Thanks!


-- 
Antonio Quartulli
OpenVPN Inc.