2024-11-20, 12:34:08 +0100, Antonio Quartulli wrote:

On 20/11/2024 12:12, Sabrina Dubroca wrote:

[...]

quoted

quoted

quoted

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.

quoted

quoted

However, the sanity check should make sure nobody can inject bogus
combinations.

quoted

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