2024-11-20, 12:34:08 +0100, Antonio Quartulli wrote:
> On 20/11/2024 12:12, Sabrina Dubroca wrote:
[...]
> > > > I don't know when userspace would use v4mapped addresses,
> > >
> > > It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY"
> > > set to true (you can check ipv6(7) for more details).
> > > This socket can receive IPv4 connections, which are implemented using
> > > v4mapped addresses. In this case both remote and local are going to be
> > > v4mapped.
> >
> > I'm familiar with v4mapped addresses, but I wasn't sure the userspace
> > part would actually passed them as peer. But I guess it would when the
> > peer connects over ipv4 on an ipv6 socket.
> >
> > So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never
> > happen? In that case I guess we just need to check that we got 2
> > attributes of the same type (both _IPV4 or both _IPV6) and if we got
> > _IPV6, that they're either both v4mapped or both not. Might be a tiny
> > bit simpler than what I was suggesting below.
>
> Exactly - this is what I was originally suggesting, but your solution is
> just a bit cleaner imho.
Ok.
> > > However, the sanity check should make sure nobody can inject bogus
> > > combinations.
> > >
> > > > but treating
> > > > a v4mapped address as a "proper" ipv4 address should work with the
> > > > rest of the code, since you already have the conversion in
> > > > ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you
> > > > could do something like (rough idea and completely untested):
> > > >
> > > > static int get_family(attr_v4, attr_v6)
> > > > {
> > > > if (attr_v4)
> > > > return AF_INET;
> > > > if (attr_v6) {
> > > > if (ipv6_addr_v4mapped(attr_v6)
> > > > return AF_INET;
> > > > return AF_INET6;
> > > > }
> > > > return AF_UNSPEC;
> > > > }
> > > >
> > > >
> > > > // in _precheck:
> > > > // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6] check
> > > > // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6
> > >
> > > the latter is already covered by:
> > >
> > > 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) {
> > > 194 NL_SET_ERR_MSG_MOD(info->extack,
> > > 195 "cannot specify local IPv4 address
> > > without remote");
> > > 196 return -EINVAL;
> > > 197 }
> > > 198
> > > 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
> > > 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) {
> > > 201 NL_SET_ERR_MSG_MOD(info->extack,
> > > 202 "cannot specify local IPV6 address
> > > without remote");
> > > 203 return -EINVAL;
> > > 204 }
> >
> > LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the remote is
> > v4mapped. And conversely, LOCAL_IPV6 combined with REMOTE_IPV6 isn't
> > ok if remote is v4mapped. So those checks should go away and be
> > replaced with the "get_family" thing, but that requires at most one of
> > the _IPV4/_IPV6 attributes to be present to behave consistently.
>
> I don't expect to receive a mix of _IPV4 and _IPV6, because the assumption
> is that either both addresses are v4mapped or none.
>
> Userspace fetches the addresses from the received packet, so I presume they
> will both be exposed as v4mapped if we are in this special case.
>
> Hence, I don't truly want to allow combining them.
>
> Does it make sense?
Yup, thanks.
--
Sabrina
