On 13/11/2024 17:56, Sabrina Dubroca wrote:
2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote:
On 04/11/2024 16:14, Sabrina Dubroca wrote:
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
+ struct genl_info *info,
+ struct nlattr **attrs)
+{
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
+ OVPN_A_PEER_ID))
+ return -EINVAL;
+
+ if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify both remote IPv4 or IPv6 address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify remote port without IP address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV4]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify local IPv4 address without remote");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV6]) {
I think these consistency checks should account for v4mapped
addresses. With remote=v4mapped and local=v6 we'll end up with an
incorrect ipv4 "local" address (taken out of the ipv6 address's first
4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
out of that.
Right, a v4mapped address would fool this check.
How about checking if both or none addresses are v4mapped? This way we
should prevent such cases.
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.
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 }
remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4], attrs[OVPN_A_PEER_REMOTE_IPV6]);
local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4], attrs[OVPN_A_PEER_LOCAL_IPV6]);
if (remote_family != local_family) {
extack "incompatible address families";
return -EINVAL;
}
That would mirror the conversion that
ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do.
Yeah, pretty much what I was suggested, but in a more explicit manner.
I like it.
int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
{
[...]
+ } else {
+ rcu_read_lock();
+ hash_for_each_rcu(ovpn->peers->by_id, bkt, peer,
+ hash_entry_id) {
+ /* skip already dumped peers that were dumped by
+ * previous invocations
+ */
+ if (last_idx > 0) {
+ last_idx--;
+ continue;
+ }
If a peer that was dumped during a previous invocation is removed in
between, we'll miss one that's still present in the overall dump. I
don't know how much it matters (I guses it depends on how the results
of this dump are used by userspace), so I'll let you decide if this
needs to be fixed immediately or if it can be ignored for now.
True, this is a risk I assumed.
Not extremely important if you ask me, but do you have any suggestion how to
avoid this in an elegant and lockless way?
No, inconsistent dumps are an old problem with netlink, so I'm just
mentioning it as something to be aware of. You can add
genl_dump_check_consistent to let userspace know that it may have
gotten incorrect information (you'll need to keep a counter and
increment it when a peer is added/removed). On a very busy server you
may never manage to get a consistent dump, if peers are going up and
down very fast.
There's been some progress for dumping netdevices in commit
759ab1edb56c ("net: store netdevs in an xarray"), but that can still
return incorrect data.
Got it. I'll keep it as it is for now, since this is not critical.
Thanks a lot.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
