net: shaper: reject duplicate leaves in GROUP request

net_shaper_nl_group_doit() does not deduplicate NET_SHAPER_A_LEAVES
entries. When userspace supplies the same leaf handle twice, the same
old-parent pointer lands twice in old_nodes[]. The cleanup loop double
frees the parent. Of course the same parent may still be in old_nodes[]
twice if we are moving multiple of its leaves.

Note that this patch also implicitly fixes the fact that the
i >= leaves_count path forgets to set ret.

Fixes: 5d5d4700e75d ("net-shapers: implement NL group operation")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Link: https://patch.msgid.link/20260510192904.3987113-4-kuba@kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/net/shaper/shaper.c b/net/shaper/shaper.c
index 86319ddbf290585ea4a52cd2156367145e2fc0e4..c8960821cf236f8194f48231b89ab98c9c19877c 100644 (file)
--- a/net/shaper/shaper.c
+++ b/net/shaper/shaper.c
@@ -941,6 +941,46 @@ static int net_shaper_handle_cmp(const struct net_shaper_handle *a,
        return memcmp(a, b, sizeof(*a));
 }
 
+static int net_shaper_parse_leaves(struct net_shaper_binding *binding,
+                                  struct genl_info *info,
+                                  const struct net_shaper *node,
+                                  struct net_shaper *leaves,
+                                  int leaves_count)
+{
+       struct nlattr *attr;
+       int i, j, ret, rem;
+
+       i = 0;
+       nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
+                              genlmsg_data(info->genlhdr),
+                              genlmsg_len(info->genlhdr), rem) {
+               if (WARN_ON_ONCE(i >= leaves_count))
+                       return -EINVAL;
+
+               ret = net_shaper_parse_leaf(binding, attr, info,
+                                           node, &leaves[i]);
+               if (ret)
+                       return ret;
+
+               /* Reject duplicates */
+               for (j = 0; j < i; j++) {
+                       if (net_shaper_handle_cmp(&leaves[i].handle,
+                                                 &leaves[j].handle))
+                               continue;
+
+                       NL_SET_ERR_MSG_ATTR_FMT(info->extack, attr,
+                                               "Duplicate leaf shaper %d:%d",
+                                               leaves[i].handle.scope,
+                                               leaves[i].handle.id);
+                       return -EINVAL;
+               }
+
+               i++;
+       }
+
+       return 0;
+}
+
 static int net_shaper_parent_from_leaves(int leaves_count,
                                         const struct net_shaper *leaves,
                                         struct net_shaper *node,
@@ -1197,10 +1237,9 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
        struct net_shaper **old_nodes, *leaves, node = {};
        struct net_shaper_hierarchy *hierarchy;
        struct net_shaper_binding *binding;
-       int i, ret, rem, leaves_count;
+       int i, ret, leaves_count;
        int old_nodes_count = 0;
        struct sk_buff *msg;
-       struct nlattr *attr;
 
        if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_LEAVES))
                return -EINVAL;
@@ -1228,19 +1267,10 @@ int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
        if (ret)
                goto free_leaves;
 
-       i = 0;
-       nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
-                              genlmsg_data(info->genlhdr),
-                              genlmsg_len(info->genlhdr), rem) {
-               if (WARN_ON_ONCE(i >= leaves_count))
-                       goto free_leaves;
-
-               ret = net_shaper_parse_leaf(binding, attr, info,
-                                           &node, &leaves[i]);
-               if (ret)
-                       goto free_leaves;
-               i++;
-       }
+       ret = net_shaper_parse_leaves(binding, info, &node,
+                                     leaves, leaves_count);
+       if (ret)
+               goto free_leaves;
 
        /* Prepare the msg reply in advance, to avoid device operation
         * rollback on allocation failure.