| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220223202509.439b9c6b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date: Wed, 23 Feb 2022 20:25:09 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Roopa Prabhu <roopa@...dia.com>
Cc: <davem@...emloft.net>, <netdev@...r.kernel.org>,
<stephen@...workplumber.org>, <nikolay@...ulusnetworks.com>,
<idosch@...dia.com>, <dsahern@...il.com>, <bpoirier@...dia.com>
Subject: Re: [PATCH net-next v2 09/12] vxlan: vni filtering support on
collect metadata device
On Tue, 22 Feb 2022 02:52:27 +0000 Roopa Prabhu wrote:
> +static int vxlan_vni_add(struct vxlan_dev *vxlan,
> + struct vxlan_vni_group *vg,
> + u32 vni, union vxlan_addr *group,
> + struct netlink_ext_ack *extack)
> +{
> + struct vxlan_vni_node *vninode;
> + __be32 v = cpu_to_be32(vni);
> + bool changed = false;
> + int err = 0;
> +
> + if (vxlan_vnifilter_lookup(vxlan, v))
> + return vxlan_vni_update(vxlan, vg, v, group, &changed, extack);
> +
> + err = vxlan_vni_in_use(vxlan->net, vxlan, &vxlan->cfg, v);
> + if (err) {
> + NL_SET_ERR_MSG(extack, "VNI in use");
> + return err;
> + }
> +
> + vninode = vxlan_vni_alloc(vxlan, v);
> + if (!vninode)
> + return -ENOMEM;
> +
> + err = rhashtable_lookup_insert_fast(&vg->vni_hash,
> + &vninode->vnode,
> + vxlan_vni_rht_params);
> + if (err)
leak ?
> + return err;
> +
> + __vxlan_vni_add_list(vg, vninode);
> +
> + if (vxlan->dev->flags & IFF_UP)
> + vxlan_vs_add_del_vninode(vxlan, vninode, false);
> +
> + err = vxlan_vni_update_group(vxlan, vninode, group, true, &changed,
> + extack);
> +
> + if (changed)
> + vxlan_vnifilter_notify(vxlan, vninode, RTM_NEWTUNNEL);
> +
> + return err;
> +}
> +
> +static void vxlan_vni_node_rcu_free(struct rcu_head *rcu)
> +{
> + struct vxlan_vni_node *v;
> +
> + v = container_of(rcu, struct vxlan_vni_node, rcu);
> + kfree(v);
> +}
kfree_rcu()?
> +static int vxlan_vni_del(struct vxlan_dev *vxlan,
> + struct vxlan_vni_group *vg,
> + u32 vni, struct netlink_ext_ack *extack)
> +{
> + struct vxlan_vni_node *vninode;
> + __be32 v = cpu_to_be32(vni);
> + int err = 0;
> +
> + vg = rtnl_dereference(vxlan->vnigrp);
> +
> + vninode = rhashtable_lookup_fast(&vg->vni_hash, &v,
> + vxlan_vni_rht_params);
> + if (!vninode) {
> + err = -ENOENT;
> + goto out;
> + }
> +
> + vxlan_vni_delete_group(vxlan, vninode);
> +
> + err = rhashtable_remove_fast(&vg->vni_hash,
> + &vninode->vnode,
> + vxlan_vni_rht_params);
> + if (err)
> + goto out;
> +
> + __vxlan_vni_del_list(vg, vninode);
> +
> + vxlan_vnifilter_notify(vxlan, vninode, RTM_DELTUNNEL);
> +
> + if (vxlan->dev->flags & IFF_UP)
> + vxlan_vs_add_del_vninode(vxlan, vninode, true);
> +
> + call_rcu(&vninode->rcu, vxlan_vni_node_rcu_free);
> +
> + return 0;
> +out:
> + return err;
> +}
> +
> +static int vxlan_vni_add_del(struct vxlan_dev *vxlan, __u32 start_vni,
> + __u32 end_vni, union vxlan_addr *group,
> + int cmd, struct netlink_ext_ack *extack)
> +{
> + struct vxlan_vni_group *vg;
> + int v, err = 0;
> +
> + vg = rtnl_dereference(vxlan->vnigrp);
> +
> + for (v = start_vni; v <= end_vni; v++) {
> + switch (cmd) {
> + case RTM_NEWTUNNEL:
> + err = vxlan_vni_add(vxlan, vg, v, group, extack);
> + break;
> + case RTM_DELTUNNEL:
> + err = vxlan_vni_del(vxlan, vg, v, extack);
> + break;
> + default:
> + err = -EOPNOTSUPP;
> + break;
> + }
> + if (err)
> + goto out;
> + }
> +
> + return 0;
> +out:
> + return err;
> +}
> +
> +static int vxlan_process_vni_filter(struct vxlan_dev *vxlan,
> + struct nlattr *nlvnifilter,
> + int cmd, struct netlink_ext_ack *extack)
> +{
> + struct nlattr *vattrs[VXLAN_VNIFILTER_ENTRY_MAX + 1];
> + u32 vni_start = 0, vni_end = 0;
> + union vxlan_addr group;
> + int err = 0;
unnecessary init
> + err = nla_parse_nested(vattrs,
> + VXLAN_VNIFILTER_ENTRY_MAX,
> + nlvnifilter, vni_filter_entry_policy,
> + extack);
> + if (err)
> + return err;
> +
> + if (vattrs[VXLAN_VNIFILTER_ENTRY_START]) {
> + vni_start = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_START]);
> + vni_end = vni_start;
> + }
> +
> + if (vattrs[VXLAN_VNIFILTER_ENTRY_END])
> + vni_end = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_END]);
> +
> + if (!vni_start && !vni_end) {
> + NL_SET_ERR_MSG_ATTR(extack, nlvnifilter,
> + "vni start nor end found in vni entry");
> + return -EINVAL;
> + }
> +
> + if (vattrs[VXLAN_VNIFILTER_ENTRY_GROUP]) {
> + group.sin.sin_addr.s_addr =
> + nla_get_in_addr(vattrs[VXLAN_VNIFILTER_ENTRY_GROUP]);
> + group.sa.sa_family = AF_INET;
> + } else if (vattrs[VXLAN_VNIFILTER_ENTRY_GROUP6]) {
> + group.sin6.sin6_addr =
> + nla_get_in6_addr(vattrs[VXLAN_VNIFILTER_ENTRY_GROUP6]);
> + group.sa.sa_family = AF_INET6;
> + } else {
> + memset(&group, 0, sizeof(group));
> + }
> +
> + err = vxlan_vni_add_del(vxlan, vni_start, vni_end, &group, cmd,
> + extack);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +void vxlan_vnigroup_uninit(struct vxlan_dev *vxlan)
> +{
> + struct vxlan_vni_node *v, *tmp;
> + struct vxlan_vni_group *vg;
> +
> + vg = rtnl_dereference(vxlan->vnigrp);
> + list_for_each_entry_safe(v, tmp, &vg->vni_list, vlist) {
> + rhashtable_remove_fast(&vg->vni_hash, &v->vnode,
> + vxlan_vni_rht_params);
> + hlist_del_init_rcu(&v->hlist4.hlist);
> +#if IS_ENABLED(CONFIG_IPV6)
> + hlist_del_init_rcu(&v->hlist6.hlist);
> +#endif
no need to generate the notifications here?
> + __vxlan_vni_del_list(vg, v);
> + call_rcu(&v->rcu, vxlan_vni_node_rcu_free);
> + }
> + rhashtable_destroy(&vg->vni_hash);
> + kfree(vg);
> +}
> +
> +int vxlan_vnigroup_init(struct vxlan_dev *vxlan)
> +{
> + struct vxlan_vni_group *vg;
> + int ret = -ENOMEM;
> +
> + vg = kzalloc(sizeof(*vg), GFP_KERNEL);
> + if (!vg)
> + goto out;
return -ENOMEM;
the jumping dance is really not worth it here
> + ret = rhashtable_init(&vg->vni_hash, &vxlan_vni_rht_params);
> + if (ret)
> + goto err_rhtbl;
> + INIT_LIST_HEAD(&vg->vni_list);
> + rcu_assign_pointer(vxlan->vnigrp, vg);
> +
> + return 0;
> +
> +out:
> + return ret;
> +
> +err_rhtbl:
> + kfree(vg);
> +
> + goto out;
> +}
> +
> +static int vxlan_vnifilter_process(struct sk_buff *skb, struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack)
> +{
> + struct net *net = sock_net(skb->sk);
> + struct tunnel_msg *tmsg;
> + struct vxlan_dev *vxlan;
> + struct net_device *dev;
> + struct nlattr *attr;
> + int err, vnis = 0;
> + int rem;
> +
> + /* this should validate the header and check for remaining bytes */
> + err = nlmsg_parse(nlh, sizeof(*tmsg), NULL, VXLAN_VNIFILTER_MAX, NULL,
> + extack);
Could be useful to provide a policy here, even if it only points to
single type (entry which is nested). Otherwise we will not reject
UNSPEC, and validate if ENTRY has NLA_F_NESTED set, no?
> + if (err < 0)
> + return err;
> +
> + tmsg = nlmsg_data(nlh);
> + dev = __dev_get_by_index(net, tmsg->ifindex);
> + if (!dev)
> + return -ENODEV;
> +
> + if (!netif_is_vxlan(dev)) {
> + NL_SET_ERR_MSG_MOD(extack, "The device is not a vxlan device");
> + return -EINVAL;
> + }
> +
> + vxlan = netdev_priv(dev);
> +
> + if (!(vxlan->cfg.flags & VXLAN_F_VNIFILTER))
> + return -EOPNOTSUPP;
> +
> + nlmsg_for_each_attr(attr, nlh, sizeof(*tmsg), rem) {
> + switch (nla_type(attr)) {
> + case VXLAN_VNIFILTER_ENTRY:
> + err = vxlan_process_vni_filter(vxlan, attr,
> + nlh->nlmsg_type, extack);
> + break;
> + default:
> + continue;
> + }
> + vnis++;
> + if (err)
> + break;
> + }
> +
> + if (!vnis) {
> + NL_SET_ERR_MSG_MOD(extack, "No vnis found to process");
> + err = -EINVAL;
> + }
> +
> + return err;
> +}
Powered by blists - more mailing lists