netdev - Re: [PATCH net-next v12 17/22] ovpn: implement peer add/get/dump/delete 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: <b66e3ee0-4283-4de2-9131-2cfe13d868f9@openvpn.net>
Date: Wed, 4 Dec 2024 09:43:18 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net, ryazanov.s.a@...il.com,
 Andrew Lunn <andrew@...n.ch>
Cc: Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v12 17/22] ovpn: implement peer
 add/get/dump/delete via netlink

On 03/12/2024 18:46, Paolo Abeni wrote:
> On 12/2/24 16:07, Antonio Quartulli wrote:
>> +/**
>> + * ovpn_nl_peer_modify - modify the peer attributes according to the incoming msg
>> + * @peer: the peer to modify
>> + * @info: generic netlink info from the user request
>> + * @attrs: the attributes from the user request
>> + *
>> + * Return: a negative error code in case of failure, 0 on success or 1 on
>> + *	   success and the VPN IPs have been modified (requires rehashing in MP
>> + *	   mode)
>> + */
>> +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
>> +			       struct nlattr **attrs)
>> +{
>> +	struct sockaddr_storage ss = {};
>> +	u32 sockfd, interv, timeout;
>> +	struct socket *sock = NULL;
>> +	u8 *local_ip = NULL;
>> +	bool rehash = false;
>> +	int ret;
>> +
>> +	if (attrs[OVPN_A_PEER_SOCKET]) {
>> +		if (peer->sock) {
>> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +					       "peer socket can't be modified");
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* lookup the fd in the kernel table and extract the socket
>> +		 * object
>> +		 */
>> +		sockfd = nla_get_u32(attrs[OVPN_A_PEER_SOCKET]);
>> +		/* sockfd_lookup() increases sock's refcounter */
>> +		sock = sockfd_lookup(sockfd, &ret);
>> +		if (!sock) {
>> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +					       "cannot lookup peer socket (fd=%u): %d",
>> +					       sockfd, ret);
>> +			return -ENOTSOCK;
>> +		}
>> +
>> +		/* Only when using UDP as transport protocol the remote endpoint
>> +		 * can be configured so that ovpn knows where to send packets
>> +		 * to.
>> +		 *
>> +		 * In case of TCP, the socket is connected to the peer and ovpn
>> +		 * will just send bytes over it, without the need to specify a
>> +		 * destination.
>> +		 */
>> +		if (sock->sk->sk_protocol != IPPROTO_UDP &&
>> +		    (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
>> +		     attrs[OVPN_A_PEER_REMOTE_IPV6])) {
>> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +					       "unexpected remote IP address for non UDP socket");
>> +			sockfd_put(sock);
>> +			return -EINVAL;
>> +		}
>> +
>> +		peer->sock = ovpn_socket_new(sock, peer);
>> +		if (IS_ERR(peer->sock)) {
>> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
>> +					       "cannot encapsulate socket: %ld",
>> +					       PTR_ERR(peer->sock));
>> +			sockfd_put(sock);
>> +			peer->sock = NULL;
> 
> This looks race-prone. If any other CPU can do concurrent read access to
> peer->sock it could observe an invalid pointer
> Even if such race does not exist, it would be cleaner store
> ovpn_socket_new() return value in a local variable and set peer->sock
> only on successful creation.

Yeah, this race is not possible because at this time the peer is not 
hashed yet, so it only exists in this local 'peer' variable.

However, you're not the first one to comment this piece of code.
I'll definitely switch to using a local variable for the sock.

Thanks!

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.