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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52bdddf9-99e0-4b87-8bae-c0a8f32beba8@openvpn.net>
Date: Wed, 5 Mar 2025 14:17:13 +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 11:11, Sabrina Dubroca wrote:
> 2025-03-05, 02:00:21 +0100, Antonio Quartulli wrote:
>> 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?
> 
> Nothing. It would make a little bit more sense to use nla_get_u16 if
> key_id was u16 (even with OVPN_A_KEYCONF_KEY_ID defined as U32), or to
> use nla_get_u8 for u8, but here it was just 3 different int sizes and
> that triggered my "uh? what?" :)
> 
>> 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.
> 
> Sure.
> 
>> 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.
> 
> Yes.
> 

Ok :)

Then I'll make sure they all use nla_get/put_u32, but will still store 
the key_id in a u8 field.

Cheers!

-- 
Antonio Quartulli
OpenVPN Inc.