genetlink: get rid of family->attrbuf

genl_family_rcv_msg_attrs_parse() reuses the global family->attrbuf
when family->parallel_ops is false. However, family->attrbuf is not
protected by any lock on the genl_family_rcv_msg_doit() code path.

This leads to several different consequences, one of them is UAF,
like the following:

genl_family_rcv_msg_doit():		genl_start():
					  genl_family_rcv_msg_attrs_parse()
					    attrbuf = family->attrbuf
					    __nlmsg_parse(attrbuf);
  genl_family_rcv_msg_attrs_parse()
    attrbuf = family->attrbuf
    __nlmsg_parse(attrbuf);
					  info->attrs = attrs;
					  cb->data = info;

netlink_unicast_kernel():
 consume_skb()
					genl_lock_dumpit():
					  genl_dumpit_info(cb)->attrs

Note family->attrbuf is an array of pointers to the skb data, once
the skb is freed, any dereference of family->attrbuf will be a UAF.

Maybe we could serialize the family->attrbuf with genl_mutex too, but
that would make the locking more complicated. Instead, we can just get
rid of family->attrbuf and always allocate attrbuf from heap like the
family->parallel_ops==true code path. This may add some performance
overhead but comparing with taking the global genl_mutex, it still
looks better.

Fixes: 1bd0a4c050f3 ("net: ieee802154: have genetlink code to parse the attrs during dumpit")
Fixes: 3a3cc4be2327 ("net: tipc: have genetlink code to parse the attrs during dumpit")
Reported-and-tested-by: syzbot+3039ddf6d7b13daf3787@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+80cad1e3cb4c41cde6ff@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+736bcbcb11b60d0c0792@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+520f8704db2b68091d44@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+c96e4dfb32f8987fdeed@syzkaller.appspotmail.com
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 74950663bb00d2be53d3cfb82bcb23ffc35e5324..ad71ed4f55ffb10268074dd5bbb4b17c3067e397 100644 (file)
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -41,7 +41,6 @@ struct genl_info;
  *     Note that unbind() will not be called symmetrically if the
  *     generic netlink family is removed while there are still open
  *     sockets.
- * @attrbuf: buffer to store parsed attributes (private)
  * @mcgrps: multicast groups used by this family
  * @n_mcgrps: number of multicast groups
  * @mcgrp_offset: starting number of multicast group IDs in this family
@@ -66,7 +65,6 @@ struct genl_family {
                                             struct genl_info *info);
        int                     (*mcast_bind)(struct net *net, int group);
        void                    (*mcast_unbind)(struct net *net, int group);
-       struct nlattr **        attrbuf;        /* private */
        const struct genl_ops * ops;
        const struct genl_multicast_group *mcgrps;
        unsigned int            n_ops;

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 55ee680e9db180b2d2e6d56db194c006df8049d8..a914b9365a4664c05bc3eb79d70456363be3efc9 100644 (file)
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -351,22 +351,11 @@ int genl_register_family(struct genl_family *family)
                start = end = GENL_ID_VFS_DQUOT;
        }
 
-       if (family->maxattr && !family->parallel_ops) {
-               family->attrbuf = kmalloc_array(family->maxattr + 1,
-                                               sizeof(struct nlattr *),
-                                               GFP_KERNEL);
-               if (family->attrbuf == NULL) {
-                       err = -ENOMEM;
-                       goto errout_locked;
-               }
-       } else
-               family->attrbuf = NULL;
-
        family->id = idr_alloc_cyclic(&genl_fam_idr, family,
                                      start, end + 1, GFP_KERNEL);
        if (family->id < 0) {
                err = family->id;
-               goto errout_free;
+               goto errout_locked;
        }
 
        err = genl_validate_assign_mc_groups(family);
@@ -385,8 +374,6 @@ int genl_register_family(struct genl_family *family)
 
 errout_remove:
        idr_remove(&genl_fam_idr, family->id);
-errout_free:
-       kfree(family->attrbuf);
 errout_locked:
        genl_unlock_all();
        return err;
@@ -419,8 +406,6 @@ int genl_unregister_family(const struct genl_family *family)
                   atomic_read(&genl_sk_destructing_cnt) == 0);
        genl_unlock();
 
-       kfree(family->attrbuf);
-
        genl_ctrl_event(CTRL_CMD_DELFAMILY, family, NULL, 0);
 
        return 0;
@@ -485,30 +470,23 @@ genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
        if (!family->maxattr)
                return NULL;
 
-       if (family->parallel_ops) {
-               attrbuf = kmalloc_array(family->maxattr + 1,
-                                       sizeof(struct nlattr *), GFP_KERNEL);
-               if (!attrbuf)
-                       return ERR_PTR(-ENOMEM);
-       } else {
-               attrbuf = family->attrbuf;
-       }
+       attrbuf = kmalloc_array(family->maxattr + 1,
+                               sizeof(struct nlattr *), GFP_KERNEL);
+       if (!attrbuf)
+               return ERR_PTR(-ENOMEM);
 
        err = __nlmsg_parse(nlh, hdrlen, attrbuf, family->maxattr,
                            family->policy, validate, extack);
        if (err) {
-               if (family->parallel_ops)
-                       kfree(attrbuf);
+               kfree(attrbuf);
                return ERR_PTR(err);
        }
        return attrbuf;
 }
 
-static void genl_family_rcv_msg_attrs_free(const struct genl_family *family,
-                                          struct nlattr **attrbuf)
+static void genl_family_rcv_msg_attrs_free(struct nlattr **attrbuf)
 {
-       if (family->parallel_ops)
-               kfree(attrbuf);
+       kfree(attrbuf);
 }
 
 struct genl_start_context {
@@ -542,7 +520,7 @@ static int genl_start(struct netlink_callback *cb)
 no_attrs:
        info = genl_dumpit_info_alloc();
        if (!info) {
-               genl_family_rcv_msg_attrs_free(ctx->family, attrs);
+               genl_family_rcv_msg_attrs_free(attrs);
                return -ENOMEM;
        }
        info->family = ctx->family;
@@ -559,7 +537,7 @@ no_attrs:
        }
 
        if (rc) {
-               genl_family_rcv_msg_attrs_free(info->family, info->attrs);
+               genl_family_rcv_msg_attrs_free(info->attrs);
                genl_dumpit_info_free(info);
                cb->data = NULL;
        }
@@ -588,7 +566,7 @@ static int genl_lock_done(struct netlink_callback *cb)
                rc = ops->done(cb);
                genl_unlock();
        }
-       genl_family_rcv_msg_attrs_free(info->family, info->attrs);
+       genl_family_rcv_msg_attrs_free(info->attrs);
        genl_dumpit_info_free(info);
        return rc;
 }
@@ -601,7 +579,7 @@ static int genl_parallel_done(struct netlink_callback *cb)
 
        if (ops->done)
                rc = ops->done(cb);
-       genl_family_rcv_msg_attrs_free(info->family, info->attrs);
+       genl_family_rcv_msg_attrs_free(info->attrs);
        genl_dumpit_info_free(info);
        return rc;
 }
@@ -694,7 +672,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
                family->post_doit(ops, skb, &info);
 
 out:
-       genl_family_rcv_msg_attrs_free(family, attrbuf);
+       genl_family_rcv_msg_attrs_free(attrbuf);
 
        return err;
 }