linux-kernel - [Patch] mqueue: fix netlink sock refcnt and skb refcnt

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

Message-Id: <1499663303-4514-1-git-send-email-xiyou.wangcong@gmail.com>
Date:   Sun,  9 Jul 2017 22:08:23 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, Cong Wang <xiyou.wangcong@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Manfred Spraul <manfred@...orfullife.com>
Subject: [Patch] mqueue: fix netlink sock refcnt and skb refcnt

netlink_sendskb() is problematic, it releases sock refcnt
silently which could cause troubles we can call it multiple
times. info->notify_sock is a good example where we
setup once and use it to send netlink skb's for many times.
It should not hold or release any refcnt, but needs to rely
on netlink_attachskb()/netlink_detachskb() to hold/release
the corresponding refcnt.

Same for the skb attached to this sock, it is allocated once
and used for multiple times, so we should hold its refcnt
in netlink_attachskb().

At last, we need to call netlink_detachskb() to release
both refcnt's after we remove the notification.

Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: Manfred Spraul <manfred@...orfullife.com>
Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
---
 ipc/mqueue.c             |  1 +
 net/netlink/af_netlink.c | 25 ++++++++++---------------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index eb1391b..8b0a0ce 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -683,6 +683,7 @@ static void remove_notification(struct mqueue_inode_info *info)
 	    info->notify.sigev_notify == SIGEV_THREAD) {
 		set_cookie(info->notify_cookie, NOTIFY_REMOVED);
 		netlink_sendskb(info->notify_sock, info->notify_cookie);
+		netlink_detachskb(info->notify_sock, info->notify_cookie);
 	}
 	put_pid(info->notify_owner);
 	put_user_ns(info->notify_user_ns);
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 5acee49..9f2d6ca 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1159,8 +1159,8 @@ static struct sk_buff *netlink_alloc_large_skb(unsigned int size,
  * all error checks are performed and memory in the queue is reserved.
  * Return values:
  * < 0: error. skb freed, reference to sock dropped.
- * 0: continue
- * 1: repeat lookup - reference dropped while waiting for socket memory.
+ * 0: continue - skb reference is held.
+ * 1: repeat lookup - sock reference dropped while waiting for socket memory.
  */
 int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
 		      long *timeo, struct sock *ssk)
@@ -1198,11 +1198,12 @@ int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
 		}
 		return 1;
 	}
+	skb_get(skb);
 	netlink_skb_set_owner_r(skb, sk);
 	return 0;
 }
 
-static int __netlink_sendskb(struct sock *sk, struct sk_buff *skb)
+int netlink_sendskb(struct sock *sk, struct sk_buff *skb)
 {
 	int len = skb->len;
 
@@ -1213,14 +1214,6 @@ static int __netlink_sendskb(struct sock *sk, struct sk_buff *skb)
 	return len;
 }
 
-int netlink_sendskb(struct sock *sk, struct sk_buff *skb)
-{
-	int len = __netlink_sendskb(sk, skb);
-
-	sock_put(sk);
-	return len;
-}
-
 void netlink_detachskb(struct sock *sk, struct sk_buff *skb)
 {
 	kfree_skb(skb);
@@ -1303,7 +1296,9 @@ int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
 	if (err)
 		return err;
 
-	return netlink_sendskb(sk, skb);
+	err = netlink_sendskb(sk, skb);
+	netlink_detachskb(sk, skb);
+	return err;
 }
 EXPORT_SYMBOL(netlink_unicast);
 
@@ -1333,7 +1328,7 @@ static int netlink_broadcast_deliver(struct sock *sk, struct sk_buff *skb)
 	if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf &&
 	    !test_bit(NETLINK_S_CONGESTED, &nlk->state)) {
 		netlink_skb_set_owner_r(skb, sk);
-		__netlink_sendskb(sk, skb);
+		netlink_sendskb(sk, skb);
 		return atomic_read(&sk->sk_rmem_alloc) > (sk->sk_rcvbuf >> 1);
 	}
 	return -1;
@@ -2183,7 +2178,7 @@ static int netlink_dump(struct sock *sk)
 		if (sk_filter(sk, skb))
 			kfree_skb(skb);
 		else
-			__netlink_sendskb(sk, skb);
+			netlink_sendskb(sk, skb);
 		return 0;
 	}
 
@@ -2198,7 +2193,7 @@ static int netlink_dump(struct sock *sk)
 	if (sk_filter(sk, skb))
 		kfree_skb(skb);
 	else
-		__netlink_sendskb(sk, skb);
+		netlink_sendskb(sk, skb);
 
 	if (cb->done)
 		cb->done(cb);
-- 
2.5.5