unix_diag: Fix incoming connections nla length

PATCH Applied Pavel Emelyanov 3 participants 4 messages Dec 25, 2011 – Dec 26, 2011

Subsystems (3)

Networking Network drivers Networking unix sockets

lore

Share & tools

lore

Pavel Emelyanov

author

2011-12-25 19:58 UTC #0 lore raw

PATCH 1/1 unix_diag: Fix incoming connections nla length

The NLA_PUT macro should accept the actual attribute length, not the amount of elements in array :(

Signed-off-by Pavel Emelyanov <[email protected]>

---
 net/unix/diag.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/unix/diag.c +2 -1

index 91d5782..39e44c9 100644
@@ -72,7 +72,8 @@ static int sk_diag_dump_icons(struct sock *sk, struct sk_buff *nlskb)
 
 	if (sk->sk_state == TCP_LISTEN) {
 		spin_lock(&sk->sk_receive_queue.lock);
-		buf = UNIX_DIAG_PUT(nlskb, UNIX_DIAG_ICONS, sk->sk_receive_queue.qlen);
+		buf = UNIX_DIAG_PUT(nlskb, UNIX_DIAG_ICONS,
+				sk->sk_receive_queue.qlen * sizeof(u32));
 		i = 0;
 		skb_queue_walk(&sk->sk_receive_queue, skb) {
 			struct sock *req, *peer;

Signature · 1 line

1.5.5.6

David Miller

maintainer Applied

2011-12-26 19:09 UTC #1 lore raw

From: Pavel Emelyanov <[email protected]>
Date: Sun, 25 Dec 2011 23:58:05 +0400

Quoted 4 lines

The NLA_PUT macro should accept the actual attribute length, not the amount of elements in array :(
Signed-off-by Pavel Emelyanov <[email protected]>

Applied, thanks.

Eric Dumazet

2011-12-26 19:36 UTC #2 lore raw

In reply to Pavel Emelyanov

Quoted 14 lines · Pavel Emelyanov

The NLA_PUT macro should accept the actual attribute length, not the amount of elements in array :(
Signed-off-by Pavel Emelyanov <[email protected]>
---
 net/unix/diag.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
net/unix/diag.c +0 -0
index 91d5782..39e44c9 100644

net/unix/diag.c static int sk_diag_dump_icons(struct sock *sk, struct sk_buff *nlskb) · line 79

Quoted 9 lines

net/unix/diag.c +2 -1

@@ -72,7 +72,8 @@ static int sk_diag_dump_icons(struct sock *sk, struct sk_buff *nlskb)
 
 	if (sk->sk_state == TCP_LISTEN) {
 		spin_lock(&sk->sk_receive_queue.lock);
-		buf = UNIX_DIAG_PUT(nlskb, UNIX_DIAG_ICONS, sk->sk_receive_queue.qlen);
+		buf = UNIX_DIAG_PUT(nlskb, UNIX_DIAG_ICONS,
+				sk->sk_receive_queue.qlen * sizeof(u32));
 		i = 0;
 		skb_queue_walk(&sk->sk_receive_queue, skb) {
 			struct sock *req, *peer;

Hmm, I must say sk_diag_dump_icons() looks buggy, since it does :

if (peer)

	buf[i++] = sock_i_ino(peer);

So we probably leak kernel memory content to user for the (!peer) case, since we did :

UNIX_DIAG_PUT(nlskb, UNIX_DIAG_ICONS,

		sk->sk_receive_queue.qlen * sizeof(u32));

David Miller

maintainer

2011-12-26 19:42 UTC #3 lore raw

From: Eric Dumazet <[email protected]>
Date: Mon, 26 Dec 2011 20:36:12 +0100

Quoted 8 lines

if (peer)
	buf[i++] = sock_i_ino(peer);
So we probably leak kernel memory content to user for the (!peer) case, since we did :
UNIX_DIAG_PUT(nlskb, UNIX_DIAG_ICONS,
		sk->sk_receive_queue.qlen * sizeof(u32));

I just commited the following fix for this, it probably takes less effort to post a patch for this kind of bug than explain it don't you think? :)

--------------------
unix: If we happen to find peer NULL when diag dumping, write zero.

Otherwise we leave uninitialized kernel memory in there.

Reported-by Eric Dumazet <[email protected]>

Signed-off-by David S. Miller <[email protected]>

---
 net/unix/diag.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

net/unix/diag.c +1 -2

index 39e44c9..c5bdbcb 100644
@@ -86,8 +86,7 @@ static int sk_diag_dump_icons(struct sock *sk, struct sk_buff *nlskb)
 			 */
 			unix_state_lock_nested(req);
 			peer = unix_sk(req)->peer;
-			if (peer)
-				buf[i++] = sock_i_ino(peer);
+			buf[i++] = (peer ? sock_i_ino(peer) : 0);
 			unix_state_unlock(req);
 		}
 		spin_unlock(&sk->sk_receive_queue.lock);

Signature · 1 line

1.7.7.4