netdev - 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: <71c1db26-f147-4578-89ae-c5b95da0ec9a@openvpn.net>
Date: Wed, 5 Mar 2025 02:00:21 +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 05/03/2025 00:09, Sabrina Dubroca wrote:
> 2025-03-04, 13:11:28 +0100, Antonio Quartulli wrote:
>> 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.
> 
> Right.

What's wrong with key_id being u8 tough?

I am a bit reluctant to change all key_id fields/variables to u32, just 
because the netlink APIs prefers using u32 instead of u8.

Keeping variables/fields u8 allows to understand what values we're going 
to store internally.

And thanks to the netlink policy we know that no larger value will be 
attempted to be saved, even if the field is actually u32.


Cheers,


-- 
Antonio Quartulli
OpenVPN Inc.