Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink

[Sabrina Dubroca <sd@queasysnail.net>]

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