netdev - Re: [PATCH net-next v18 15/25] ovpn: implement multi-peer 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: <35eb070a-26c9-43f5-975c-d624fc2d13d1@openvpn.net>
Date: Mon, 3 Feb 2025 10:01:38 +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 net-next v18 15/25] ovpn: implement multi-peer support

On 03/02/2025 00:00, Sabrina Dubroca wrote:
> 2025-01-13, 10:31:34 +0100, Antonio Quartulli wrote:
>>   static int ovpn_newlink(struct net *src_net, struct net_device *dev,
>>   			struct nlattr *tb[], struct nlattr *data[],
>>   			struct netlink_ext_ack *extack)
>>   {
>>   	struct ovpn_priv *ovpn = netdev_priv(dev);
>>   	enum ovpn_mode mode = OVPN_MODE_P2P;
>> +	int err;
>>   
>>   	if (data && data[IFLA_OVPN_MODE]) {
>>   		mode = nla_get_u8(data[IFLA_OVPN_MODE]);
>> @@ -136,6 +183,10 @@ static int ovpn_newlink(struct net *src_net, struct net_device *dev,
>>   	ovpn->mode = mode;
>>   	spin_lock_init(&ovpn->lock);
>>   
>> +	err = ovpn_mp_alloc(ovpn);
> 
> If register_netdevice fails, ovpn->peers won't get freed in some cases
> (only if we got past ndo_init). So this should go into ndo_init.

This is something I was investigating during the weekend.
You just confirmed my suspicion. Thanks.

Will move it to ndo_init().

> 
>> +	if (err < 0)
>> +		return err;
>> +
>>   	/* turn carrier explicitly off after registration, this way state is
>>   	 * clearly defined
>>   	 */
> 
> 
> [...]
>> +static int ovpn_peer_add_mp(struct ovpn_priv *ovpn, struct ovpn_peer *peer)
>> +{
> [...]
>> +	hlist_add_head_rcu(&peer->hash_entry_id,
>> +			   ovpn_get_hash_head(ovpn->peers->by_id, &peer->id,
>> +					      sizeof(peer->id)));
>> +
>> +	if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
>> +		nhead = ovpn_get_hash_head(ovpn->peers->by_vpn_addr,
>> +					   &peer->vpn_addrs.ipv4,
>> +					   sizeof(peer->vpn_addrs.ipv4));
>> +		hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead);
>> +	}
>> +
>> +	if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) {
>> +		nhead = ovpn_get_hash_head(ovpn->peers->by_vpn_addr,
>> +					   &peer->vpn_addrs.ipv6,
>> +					   sizeof(peer->vpn_addrs.ipv6));
>> +		hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead);
>> +	}
> 
> You can't add hash_entry_addr4 and hash_entry_addr6 to the same
> hashtable.  ovpn_peer_get_by_vpn_addr{4,6} use those fields as
> "member" for hlist_nulls_for_each_entry_rcu, so container_of (in
> hlist_nulls_entry) will return a "peer" that's not really a peer
> object in memory when we walk past an entry for the wrong address
> family:
>    container_of(peer_v4->hash_entry_addr4, struct ovpn_peer, hash_entry_addr6)
> or
>    container_of(peer_v6->hash_entry_addr6, struct ovpn_peer, hash_entry_addr4)
> 
> (probably not visible in testing since we'll never really get 2 peers
> (and of different families) into the same bucket, and then also get
> them to pass the addr_equal test in ovpn_peer_get_by_vpn_addr{4,6}.
> easiest way to try to trigger problems would be making the hashtable
> single bucket, and even then...)

cr0p.
This has been lurking here for so long.
You can imagine what the original idea was, but I didn't see that issue 
with walking the list.

I have been wondering for so long if using just one hashtable could hide 
some problem, but until now I couldn't come up with any.

Ans yes, this is close to impossible to trigger, but yet, it is not correct.

Will add an extra hashtable in order to separate v4 from v6.

In the background we're already working on moving to rhashtables and 
there we were forced to have v4 and v6 separate anyway.

Thanks a lot.
Regards,

> 

-- 
Antonio Quartulli
OpenVPN Inc.