Re: [PATCH net 3/7] ovpn: reject UDP remotes incompatible with socket family

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In this commit, ovpn_nl_udp_remote_compatible()
  (drivers/net/ovpn/netlink.c) checks sk->sk_family and…
- [Low] The concern says the new rejection in
  ovpn_nl_udp_remote_compatible() (AF_INET6 socket + ipv6_only_sock() +…
- [Low] The concern says the NULL inet6_sk(sk) dereference the commit
  message describes is still reachable through the RX float path.

--

ovpn: reject UDP remotes incompatible with socket family

This commit adds ovpn_nl_udp_remote_compatible(). It rejects UDP remotes
that the attached socket cannot use when a peer is created or modified
over netlink: an IPv6 remote on an AF_INET socket, or an IPv4 remote on an
IPv6-only socket. The remote endpoint is now parsed once in the new/set
paths and passed to ovpn_nl_peer_modify(), and the commit message notes
that the transmit path still has to re-check the socket later.

> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
> index 1af7e0e624ba6..bf27a7a89f76b 100644
> --- a/drivers/net/ovpn/netlink.c
> +++ b/drivers/net/ovpn/netlink.c

[ ... ]

> @@ -383,12 +385,45 @@ static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
>  	return ret;
>  }
>  
> +/**
> + * ovpn_nl_udp_remote_compatible - check if a UDP socket can use a remote

[ ... ]

> +static int ovpn_nl_udp_remote_compatible(const struct sock *sk,
> +					 const struct sockaddr_storage *remote,
> +					 struct netlink_ext_ack *extack)
> +{
> +	const unsigned short sock_family = READ_ONCE(sk->sk_family);

[Severity: Low]
This check reads sk->sk_family and ipv6_only_sock(sk) once, without
lock_sock(), while the PEER_NEW/PEER_SET request is handled. In
ovpn_nl_peer_set_doit() it runs under rcu_read_lock(), and that lock is
dropped before ovpn_nl_peer_modify() installs the new bind.

The commit message says:

  The transmit path therefore still has to re-check socket/remote
  compatibility before sending.

At this commit, ovpn_udp_output() still picks ovpn_udp4_output() or
ovpn_udp6_output() based only on bind->remote.in4.sin_family. A later
IPV6_V6ONLY or IPV6_ADDRFORM change is therefore not caught on TX.

Is this patch meant to be complete on its own for stable? The next patch
in the series, "ovpn: recheck UDP socket family before transmit", adds the
sock_family check in ovpn_udp_output() and closes this gap. It looks like
the two patches need to be backported together.

> +
> +	if (sock_family == AF_INET && remote->ss_family == AF_INET6) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "UDP socket is IPv4 but remote is IPv6");
> +		return -EAFNOSUPPORT;
> +	}

[Severity: Low]
This rejects an IPv6 remote on an AF_INET socket when the remote comes from
netlink. Can the same mismatched bind still be installed through the RX
float path, which does not go through this helper?

drivers/net/ovpn/io.c:ovpn_recv() {
    ...
	if (sock && sock->sk->sk_protocol == IPPROTO_UDP)
		/* check if this peer changed local or remote endpoint */
		ovpn_peer_endpoints_update(peer, skb);
    ...
}

ovpn_peer_endpoints_update() updates the bind from the received packet and
does not look at the socket family. If the bind becomes AF_INET6 on a peer
whose socket is AF_INET, the next transmit would take this path:

ovpn_udp_output()
  ovpn_udp6_output()
    ip6_dst_lookup_flow(sock_net(sk), sk, &fl, NULL)

That path reaches the inet6_sk(sk) NULL dereference described in the
commit message.

At the end of the series, "ovpn: recheck UDP socket family before transmit"
covers this case. Before calling ovpn_udp6_output(), ovpn_udp_output() now
rejects AF_INET6 binds when sock_family != AF_INET6. With that patch, the
only remaining effect is that the floating peer is deleted with a transport
error.

Without that patch, does this commit alone prevent the crash it describes?

> +
> +	if (sock_family == AF_INET6 && ipv6_only_sock(sk) &&
> +	    remote->ss_family == AF_INET) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "UDP socket is IPv6-only but remote is IPv4");
> +		return -EAFNOSUPPORT;
> +	}

[Severity: Low]
This isn't a bug, but I have a question about this part of the commit
message:

  An IPv4 UDP socket cannot be used with an IPv6 remote endpoint, and an
  IPv6-only UDP socket cannot send to an IPv4 remote endpoint.

At this commit, ovpn_udp4_output() gets the route with
ip_route_output_flow(sock_net(sk), &fl, sk) and does not check
ipv6_only_sock(sk). So ovpn's own TX path would still send in this
configuration.

What actually makes the configuration unusable is the receive side, since
compute_score() in net/ipv4/udp.c skips IPv6-only sockets. The regular
udpv6_sendmsg() behaviour for v4-mapped destinations points the same way.

The next patch in the series adds the matching ipv6_only_sock() check in
ovpn_udp_output(), so the netlink and TX rules agree by the end of the
series. Since this patch rejects a configuration that used to be accepted,
would it be worth making the reasoning clearer here?

> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022121606.732029-2-antonio%40openvpn.net