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.
/P
