netdev - Re: [Patch net] genetlink: get rid of family->attrbuf

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 29 Jun 2020 17:16:38 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     xiyou.wangcong@...il.com
Cc:     netdev@...r.kernel.org,
        syzbot+3039ddf6d7b13daf3787@...kaller.appspotmail.com,
        syzbot+80cad1e3cb4c41cde6ff@...kaller.appspotmail.com,
        syzbot+736bcbcb11b60d0c0792@...kaller.appspotmail.com,
        syzbot+520f8704db2b68091d44@...kaller.appspotmail.com,
        syzbot+c96e4dfb32f8987fdeed@...kaller.appspotmail.com,
        jiri@...lanox.com
Subject: Re: [Patch net] genetlink: get rid of family->attrbuf

From: Cong Wang <xiyou.wangcong@...il.com>
Date: Sat, 27 Jun 2020 00:12:24 -0700

> 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@...kaller.appspotmail.com
> Reported-and-tested-by: syzbot+80cad1e3cb4c41cde6ff@...kaller.appspotmail.com
> Reported-and-tested-by: syzbot+736bcbcb11b60d0c0792@...kaller.appspotmail.com
> Reported-and-tested-by: syzbot+520f8704db2b68091d44@...kaller.appspotmail.com
> Reported-and-tested-by: syzbot+c96e4dfb32f8987fdeed@...kaller.appspotmail.com
> Cc: Jiri Pirko <jiri@...lanox.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>

Applied and queued up for -stable, thank you.