2025-01-13, 10:31:39 +0100, Antonio Quartulli wrote:
> +static int ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,
> + struct sockaddr_storage *ss)
> +{
> + struct sockaddr_in6 *sin6;
> + struct sockaddr_in *sin;
> + struct in6_addr *in6;
> + __be16 port = 0;
> + __be32 *in;
> + int af;
> +
> + ss->ss_family = AF_UNSPEC;
> +
> + if (attrs[OVPN_A_PEER_REMOTE_PORT])
> + port = nla_get_be16(attrs[OVPN_A_PEER_REMOTE_PORT]);
> +
> + if (attrs[OVPN_A_PEER_REMOTE_IPV4]) {
> + af = AF_INET;
> + ss->ss_family = AF_INET;
> + in = nla_data(attrs[OVPN_A_PEER_REMOTE_IPV4]);
> + } else if (attrs[OVPN_A_PEER_REMOTE_IPV6]) {
> + af = AF_INET6;
> + ss->ss_family = AF_INET6;
> + in6 = nla_data(attrs[OVPN_A_PEER_REMOTE_IPV6]);
> + } else {
> + return AF_UNSPEC;
> + }
> +
> + switch (ss->ss_family) {
> + case AF_INET6:
> + /* If this is a regular IPv6 just break and move on,
> + * otherwise switch to AF_INET and extract the IPv4 accordingly
> + */
> + if (!ipv6_addr_v4mapped(in6)) {
> + sin6 = (struct sockaddr_in6 *)ss;
> + sin6->sin6_port = port;
> + memcpy(&sin6->sin6_addr, in6, sizeof(*in6));
> + break;
> + }
> +
> + /* v4-mapped-v6 address */
> + ss->ss_family = AF_INET;
> + in = &in6->s6_addr32[3];
> + fallthrough;
> + case AF_INET:
> + sin = (struct sockaddr_in *)ss;
> + sin->sin_port = port;
> + sin->sin_addr.s_addr = *in;
> + break;
> + }
> +
> + /* don't return ss->ss_family as it may have changed in case of
> + * v4-mapped-v6 address
> + */
nit: I'm not sure that matters since the only thing the caller checks
is ret != AF_UNSPEC, and at this point, while ss_family could have
been changed, it would have changed from AF_INET6 to AF_INET, so it's
!= AF_UNSPEC.
> + return af;
> +}
[...]
> +static int ovpn_nl_peer_precheck(struct ovpn_priv *ovpn,
> + struct genl_info *info,
> + struct nlattr **attrs)
> +{
[...]
> +
> + /* VPN IPs are needed only in MP mode for selecting the right peer */
> + if (ovpn->mode == OVPN_MODE_P2P && (attrs[OVPN_A_PEER_VPN_IPV4] ||
> + attrs[OVPN_A_PEER_VPN_IPV6])) {
And in MP mode, at least one VPN_IP* is required?
[...]
> int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
> {
[...]
> + /* 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])) {
Is a peer on a UDP socket without any remote (neither
OVPN_A_PEER_REMOTE_IPV4 nor OVPN_A_PEER_REMOTE_IPV6) valid? We just
wait until we get data from it to update the endpoint?
Or should there be a check to make sure that one was provided?
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "unexpected remote IP address for non UDP socket");
> + sockfd_put(sock);
> + return -EINVAL;
> + }
> +
> + ovpn_sock = ovpn_socket_new(sock, peer);
> + if (IS_ERR(ovpn_sock)) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "cannot encapsulate socket: %ld",
> + PTR_ERR(ovpn_sock));
> + sockfd_put(sock);
> + return -ENOTSOCK;
Maybe s/-ENOTSOCK/PTR_ERR(ovpn_sock)/ ?
Overwriting ovpn_socket_new's -EBUSY etc with -ENOTSOCK is a bit
misleading to the caller.
> + }
> +
> + peer->sock = ovpn_sock;
> +
> + ret = ovpn_nl_peer_modify(peer, info, attrs);
> + if (ret < 0)
> + goto peer_release;
> +
> + ret = ovpn_peer_add(ovpn, peer);
> + if (ret < 0) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "cannot add new peer (id=%u) to hashtable: %d\n",
> + peer->id, ret);
> + goto peer_release;
> + }
> +
> + return 0;
> +
> +peer_release:
I think you need to add:
ovpn_socket_release(peer);
If ovpn_socket_new succeeded, ovpn_peer_release only takes care of the
peer but not its socket.
> + /* release right away because peer is not used in any context */
> + ovpn_peer_release(peer);
> +
> + return ret;
> }
>
> int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
[...]
> + if (attrs[OVPN_A_PEER_SOCKET]) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "socket cannot be modified");
> + return -EINVAL;
> + }
> +
> + peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
> + peer = ovpn_peer_get_by_id(ovpn, peer_id);
> + if (!peer) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "cannot find peer with id %u", peer_id);
> + return -ENOENT;
> + }
The check for non-UDP socket with a remote address configured should
be replicated here, no?
--
Sabrina
