netdev - Re: [PATCH net-next v11 17/23] ovpn: add support for peer floating

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: <0a5e0d83-2fd5-4281-8aea-7f5c80aee06b@openvpn.net>
Date: Tue, 12 Nov 2024 14:52:50 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: 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@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 17/23] ovpn: add support for peer floating



On 04/11/2024 12:24, Sabrina Dubroca wrote:
> 2024-10-29, 11:47:30 +0100, Antonio Quartulli wrote:
>> +static int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer,
>> +				    const struct sockaddr_storage *ss,
>> +				    const u8 *local_ip)
>> +	__must_hold(&peer->lock)
>> +{
>> +	struct ovpn_bind *bind;
>> +	size_t ip_len;
>> +
>> +	/* create new ovpn_bind object */
>> +	bind = ovpn_bind_from_sockaddr(ss);
>> +	if (IS_ERR(bind))
>> +		return PTR_ERR(bind);
>> +
>> +	if (local_ip) {
>> +		if (ss->ss_family == AF_INET) {
>> +			ip_len = sizeof(struct in_addr);
>> +		} else if (ss->ss_family == AF_INET6) {
>> +			ip_len = sizeof(struct in6_addr);
>> +		} else {
>> +			netdev_dbg(peer->ovpn->dev, "%s: invalid family for remote endpoint\n",
>> +				   __func__);
> 
> ratelimited since that can be triggered from packet processing?

ACK

> 
> 
> [...]
>> +void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb)
>> +{
> [...]
>> +
>> +	switch (family) {
>> +	case AF_INET:
>> +		sa = (struct sockaddr_in *)&ss;
>> +		sa->sin_family = AF_INET;
>> +		sa->sin_addr.s_addr = ip_hdr(skb)->saddr;
>> +		sa->sin_port = udp_hdr(skb)->source;
>> +		salen = sizeof(*sa);
>> +		break;
>> +	case AF_INET6:
>> +		sa6 = (struct sockaddr_in6 *)&ss;
>> +		sa6->sin6_family = AF_INET6;
>> +		sa6->sin6_addr = ipv6_hdr(skb)->saddr;
>> +		sa6->sin6_port = udp_hdr(skb)->source;
>> +		sa6->sin6_scope_id = ipv6_iface_scope_id(&ipv6_hdr(skb)->saddr,
>> +							 skb->skb_iif);
>> +		salen = sizeof(*sa6);
>> +		break;
>> +	default:
>> +		goto unlock;
>> +	}
>> +
>> +	netdev_dbg(peer->ovpn->dev, "%s: peer %d floated to %pIScp", __func__,
> 
>                                                %u for peer->id?
> 
> and ratelimited too, probably.
> 
> (also in ovpn_peer_update_local_endpoint in the previous patch)

Technically we don't expect that frequent float/endpoint updates, but 
should they happen..better to be protected.

ACK

> 
>> +		   peer->id, &ss);
>> +	ovpn_peer_reset_sockaddr(peer, (struct sockaddr_storage *)&ss,
>> +				 local_ip);
> 
> skip the rehash if this fails? peer->bind will still be the old one so
> moving it to the new hash chain won't help (the lookup will fail).

Yeah, it makes sense.

Thanks a lot.
Regards,

-- 
Antonio Quartulli
OpenVPN Inc.