netdev - Re: [PATCH] af_netlink: give correct bounds to dump skb for NLMSG

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1510121793.8974.1.camel@sipsolutions.net>
Date:   Wed, 08 Nov 2017 07:16:33 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     "Jason A. Donenfeld" <Jason@...c4.com>, davem@...emloft.net,
        Netdev <netdev@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] af_netlink: give correct bounds to dump skb for
 NLMSG_DONE

On Tue, 2017-11-07 at 20:29 +0900, Jason A. Donenfeld wrote:
> 
> This patch thus reserves and restores the required length for
> NLMSG_DONE during the call to the dump function.
> 

That basically removes that space though, even when the dump isn't
complete... wouldn't it be better to do something like this?

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f34750691c5c..fccf83598dab 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2183,9 +2183,15 @@ static int netlink_dump(struct sock *sk)
 	skb_reserve(skb, skb_tailroom(skb) - alloc_size);
 	netlink_skb_set_owner_r(skb, sk);
 
-	len = cb->dump(skb, cb);
+	/* if the dump is already done, just complete */
+	if (nlk->dump_done)
+		len = 0;
+	else
+		len = cb->dump(skb, cb);
+
+	nlk->dump_done = len == 0;
 
-	if (len > 0) {
+	if (len > 0 || skb_tailroom(skb) < nlmsg_total_size(sizeof(len))) {
 		mutex_unlock(nlk->cb_mutex);
 
 		if (sk_filter(sk, skb))
@@ -2196,7 +2202,7 @@ static int netlink_dump(struct sock *sk)
 	}
 
 	nlh = nlmsg_put_answer(skb, cb, NLMSG_DONE, sizeof(len), NLM_F_MULTI);
-	if (!nlh)
+	if (WARN_ON(!nlh))
 		goto errout_skb;
 
 	nl_dump_check_consistent(cb, nlh);
@@ -2273,6 +2279,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	}
 
 	nlk->cb_running = true;
+	nlk->dump_done = false;
 
 	mutex_unlock(nlk->cb_mutex);
 
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 3490f2430532..91a3652d384f 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -33,6 +33,7 @@ struct netlink_sock {
 	wait_queue_head_t	wait;
 	bool			bound;
 	bool			cb_running;
+	bool			dump_done;
 	struct netlink_callback	cb;
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;


https://p.sipsolutions.net/90574c3c0116d68a.txt

(untested)

johannes