[Patch net] tipc: fix uninit skb->data in tipc_nl_compat_dumpit()

[Patch net] tipc: fix uninit skb->data in tipc_nl_compat_dumpit()

[Cong Wang]

__tipc_nl_compat_dumpit() has two callers, and it expects them to
pass a valid nlmsghdr via arg->data. This header is artificial and
crafted just for __tipc_nl_compat_dumpit().

tipc_nl_compat_publ_dump() does so by putting a genlmsghdr as well
as some nested attribute, TIPC_NLA_SOCK. But the other caller
tipc_nl_compat_dumpit() does not, this leaves arg->data uninitialized
on this call path.

Fix this by just adding a similar nlmsghdr without any payload in
tipc_nl_compat_dumpit().

This bug exists since day 1, but the recent commit 6ea67769ff33
("net: tipc: prepare attrs in __tipc_nl_compat_dumpit()") makes it
easier to appear.

Reported-and-tested-by: syzbot+0e7181deafa7e0b79923@syzkaller.appspotmail.com
Fixes: d0796d1ef63d ("tipc: convert legacy nl bearer dump to nl compat")
Cc: Jon Maloy <jmaloy@redhat.com>
Cc: Ying Xue <ying.xue@windriver.com>
Cc: Richard Alpe <richard.alpe@ericsson.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/tipc/netlink_compat.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
index 217516357ef2..90e3c70a91ad 100644
--- a/net/tipc/netlink_compat.c
+++ b/net/tipc/netlink_compat.c
@@ -275,8 +275,9 @@ static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
 static int tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
 				 struct tipc_nl_compat_msg *msg)
 {
-	int err;
+	struct nlmsghdr *nlh;
 	struct sk_buff *arg;
+	int err;
 
 	if (msg->req_type && (!msg->req_size ||
 			      !TLV_CHECK_TYPE(msg->req, msg->req_type)))
@@ -305,6 +306,15 @@ static int tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
 		return -ENOMEM;
 	}
 
+	nlh = nlmsg_put(arg, 0, 0, tipc_genl_family.id, 0, NLM_F_MULTI);
+	if (!nlh) {
+		kfree_skb(arg);
+		kfree_skb(msg->rep);
+		msg->rep = NULL;
+		return -EMSGSIZE;
+	}
+	nlmsg_end(arg, nlh);
+
 	err = __tipc_nl_compat_dumpit(cmd, msg, arg);
 	if (err) {
 		kfree_skb(msg->rep);
-- 
2.28.0

Re: [Patch net] tipc: fix uninit skb->data in tipc_nl_compat_dumpit()

[Ying Xue]

On 8/16/20 7:29 AM, Cong Wang wrote:
> __tipc_nl_compat_dumpit() has two callers, and it expects them to
> pass a valid nlmsghdr via arg->data. This header is artificial and
> crafted just for __tipc_nl_compat_dumpit().
> 
> tipc_nl_compat_publ_dump() does so by putting a genlmsghdr as well
> as some nested attribute, TIPC_NLA_SOCK. But the other caller
> tipc_nl_compat_dumpit() does not, this leaves arg->data uninitialized
> on this call path.
> 
> Fix this by just adding a similar nlmsghdr without any payload in
> tipc_nl_compat_dumpit().
> 
> This bug exists since day 1, but the recent commit 6ea67769ff33
> ("net: tipc: prepare attrs in __tipc_nl_compat_dumpit()") makes it
> easier to appear.
> 
> Reported-and-tested-by: syzbot+0e7181deafa7e0b79923@syzkaller.appspotmail.com
> Fixes: d0796d1ef63d ("tipc: convert legacy nl bearer dump to nl compat")
> Cc: Jon Maloy <jmaloy@redhat.com>
> Cc: Ying Xue <ying.xue@windriver.com>
> Cc: Richard Alpe <richard.alpe@ericsson.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Good finding and thanks to fix it!

Acked-by: Ying Xue <ying.xue@windriver.com>

> ---
>  net/tipc/netlink_compat.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c
> index 217516357ef2..90e3c70a91ad 100644
> --- a/net/tipc/netlink_compat.c
> +++ b/net/tipc/netlink_compat.c
> @@ -275,8 +275,9 @@ static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
>  static int tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
>  				 struct tipc_nl_compat_msg *msg)
>  {
> -	int err;
> +	struct nlmsghdr *nlh;
>  	struct sk_buff *arg;
> +	int err;
>  
>  	if (msg->req_type && (!msg->req_size ||
>  			      !TLV_CHECK_TYPE(msg->req, msg->req_type)))
> @@ -305,6 +306,15 @@ static int tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd,
>  		return -ENOMEM;
>  	}
>  
> +	nlh = nlmsg_put(arg, 0, 0, tipc_genl_family.id, 0, NLM_F_MULTI);
> +	if (!nlh) {
> +		kfree_skb(arg);
> +		kfree_skb(msg->rep);
> +		msg->rep = NULL;
> +		return -EMSGSIZE;
> +	}
> +	nlmsg_end(arg, nlh);
> +
>  	err = __tipc_nl_compat_dumpit(cmd, msg, arg);
>  	if (err) {
>  		kfree_skb(msg->rep);
> 

Re: [Patch net] tipc: fix uninit skb->data in tipc_nl_compat_dumpit()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 15 Aug 2020 16:29:15 -0700

> __tipc_nl_compat_dumpit() has two callers, and it expects them to
> pass a valid nlmsghdr via arg->data. This header is artificial and
> crafted just for __tipc_nl_compat_dumpit().
> 
> tipc_nl_compat_publ_dump() does so by putting a genlmsghdr as well
> as some nested attribute, TIPC_NLA_SOCK. But the other caller
> tipc_nl_compat_dumpit() does not, this leaves arg->data uninitialized
> on this call path.
> 
> Fix this by just adding a similar nlmsghdr without any payload in
> tipc_nl_compat_dumpit().
> 
> This bug exists since day 1, but the recent commit 6ea67769ff33
> ("net: tipc: prepare attrs in __tipc_nl_compat_dumpit()") makes it
> easier to appear.
> 
> Reported-and-tested-by: syzbot+0e7181deafa7e0b79923@syzkaller.appspotmail.com
> Fixes: d0796d1ef63d ("tipc: convert legacy nl bearer dump to nl compat")
> Cc: Jon Maloy <jmaloy@redhat.com>
> Cc: Ying Xue <ying.xue@windriver.com>
> Cc: Richard Alpe <richard.alpe@ericsson.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable, thanks.