@@ -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;
@@ -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 @@ static int genl_start(struct netlink_callback *cb)
}
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;
}
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: 75cdbdd08900 ("net: ieee802154: have genetlink code to parse the attrs during dumpit") Fixes: 057af7071344 ("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> --- include/net/genetlink.h | 2 -- net/netlink/genetlink.c | 48 +++++++++++------------------------------ 2 files changed, 13 insertions(+), 37 deletions(-)