iv/linux
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

genetlink: get rid of family->attrbuf

Browse Source 
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: 75cdbdd089 ("net: ieee802154: have genetlink code to parse the attrs during dumpit")
Fixes: 057af70713 ("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>
This commit is contained in:
Cong Wang 2020-06-27 00:12:24 -07:00 committed by David S. Miller
parent 33c568ba49
commit bf64ff4c2a
2 changed files with 13 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
include/net/genetlink.h
View File

@ -41,7 +41,6 @@ struct genl_info;
* Note that unbind() will not be called symmetrically if the * Note that unbind() will not be called symmetrically if the
* generic netlink family is removed while there are still open * generic netlink family is removed while there are still open
* sockets. * sockets.
* @attrbuf: buffer to store parsed attributes (private)
* @mcgrps: multicast groups used by this family * @mcgrps: multicast groups used by this family
* @n_mcgrps: number of multicast groups * @n_mcgrps: number of multicast groups
* @mcgrp_offset: starting number of multicast group IDs in this family * @mcgrp_offset: starting number of multicast group IDs in this family
@ -66,7 +65,6 @@ struct genl_family {
struct genl_info *info); struct genl_info *info);
int (*mcast_bind)(struct net *net, int group); int (*mcast_bind)(struct net *net, int group);
void (*mcast_unbind)(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_ops * ops;
const struct genl_multicast_group *mcgrps; const struct genl_multicast_group *mcgrps;
unsigned int n_ops; unsigned int n_ops;

48
net/netlink/genetlink.c
View File

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