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

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.

...

• NL_SET_ERR_MSG_MOD(info->extack,
  

• 		   "cannot specify local IPV6 address without remote");
  

• return -EINVAL;
  

• }

[...]

...

int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info) {

[...]

...

• ret = ovpn_nl_peer_modify(peer, info, attrs);
• if (ret < 0) {

• ovpn_peer_put(peer);
  

• return ret;
  

• }
• /* ret == 1 means that VPN IPv4/6 has been modified and rehashing

• * is required
  

• */
  

• if (ret > 0) {

&& mode == MP ?

I don't see ovpn_nl_peer_modify checking that before returning 1, and in P2P mode ovpn->peers will be NULL.

Right. I was wondering if it's better to add the check on the return statement of ovpn_nl_peer_modify...but I think it's more functional to add it here, as per your suggestion.

...

• spin_lock_bh(&ovpn->peers->lock);
  

• ovpn_peer_hash_vpn_ip(peer);
  

• spin_unlock_bh(&ovpn->peers->lock);
  

• }
• ovpn_peer_put(peer);
• return 0;

+}

...

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?

IIRC I got inspired by the station dump in the mac80211 code, which probably assumes the same risk.

...

• 	if (ovpn_nl_send_peer(skb, info, peer,
  

• 			      NETLINK_CB(cb->skb).portid,
  

• 			      cb->nlh->nlmsg_seq,
  

• 			      NLM_F_MULTI) < 0)
  

• 		break;
  

• 	/* count peers being dumped during this invocation */
  

• 	dumped++;
  

• }
  

• rcu_read_unlock();
  

• }

+out:

• netdev_put(ovpn->dev, &ovpn->dev_tracker);
• /* sum up peers dumped in this message, so that at the next invocation

• * we can continue from where we left
  

• */
  

• cb->args[1] += dumped;
• return skb->len; }

int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info) {

• return -EOPNOTSUPP;

• struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
• struct ovpn_struct *ovpn = info->user_ptr[0];
• struct ovpn_peer *peer;
• u32 peer_id;
• int ret;
• if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER))

• return -EINVAL;
  

• ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER],

• 	       ovpn_peer_nl_policy, info->extack);
  

• if (ret)

• return ret;
  

• if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,

• 	      OVPN_A_PEER_ID))
  

• return -EINVAL;
  

• peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
• peer = ovpn_peer_get_by_id(ovpn, peer_id);
• if (!peer)

maybe c/p the extack from ovpn_nl_peer_get_doit?

Yes, will do.

Thanks a lot. Regards,

...

• return -ENOENT;
  

• netdev_dbg(ovpn->dev, "%s: peer id=%u\n", __func__, peer->id);
• ret = ovpn_peer_del(peer, OVPN_DEL_PEER_REASON_USERSPACE);
• ovpn_peer_put(peer);
• return ret; }