netdev - Re: [PATCH] negative dev use in /proc/net/rose

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 for Android: free password hash cracker in your pocket

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

Message-Id: <20081013.003009.241543234.davem@davemloft.net>
Date:	Mon, 13 Oct 2008 00:30:09 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	bpidoux@...e.fr
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] negative dev use in /proc/net/rose_neigh

From: Bernard Pidoux <bpidoux@...e.fr>
Date: Tue, 30 Sep 2008 23:44:55 +0200

> I suspect that the bug was unravelled when we added more than one
> neighbour per route. The protocole accepts three, but this was not
> much used during the early days since the density of radio stations
> on the network was not big (only one node station per destination
> address usually). The network is now denser with Internet links.
>
> However, I don't understand why the test
> 
> if (rose->neighbour == neigh)
> 
> does not work, for
> rose->neighbour = NULL;
> should prevent next comparison to be valid and thus instruction
> rose->neighbour->use--; not executed.
>
> I have seen that a problem with sk_for_each() macro was identified a
> while ago into ax25 code. The problem here could be similar ?

I took a look at this some more.

That neighbour case loop you mention does set ->neighbour to NULL.

But other paths do not.  Just look for all of the pieces of code
that do "rose->neighbour->use--;" and you'll find a few that do not
NULL it out.

One such example is rose_kill_by_device().

That would cause a problem because, while rose_disconnect() marks
the socket DEAD, it doesn't actually remove it from "rose_list".
That happens later when the user actually closes the socket or
some other similar event occurs.

Therefore if rose_kill_by_neigh() happens next, the ->neighbour could
match and we'll decrement again.

But I have no idea how safe it is to NULL out this ->neighbour
unconditionally.  Lots of code seems to deref the thing unconditionally.
For example the ROSE_STATE_2 handling in rose_release().

I guess since rose_disconnect() sets sk->sk_state to TCP_CLOSE, we'll
be OK here.

Can you try this patch?

diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index a7f1ce1..41dd630 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -197,6 +197,7 @@ static void rose_kill_by_device(struct net_device *dev)
 		if (rose->device == dev) {
 			rose_disconnect(s, ENETUNREACH, ROSE_OUT_OF_ORDER, 0);
 			rose->neighbour->use--;
+			rose->neighbour = NULL;
 			rose->device = NULL;
 		}
 	}
@@ -625,6 +626,7 @@ static int rose_release(struct socket *sock)

 	case ROSE_STATE_2:
 		rose->neighbour->use--;
+		rose->neighbour = NULL;
 		release_sock(sk);
 		rose_disconnect(sk, 0, -1, -1);
 		lock_sock(sk);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html