netdev - Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush

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: <1444165938.1468.48.camel@mattb-dl>
Date:	Tue, 6 Oct 2015 21:12:18 +0000
From:	Matt Bennett <Matt.Bennett@...iedtelesis.co.nz>
To:	"g.nault@...halink.fr" <g.nault@...halink.fr>
CC:	"core@....lg.ua" <core@....lg.ua>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"paulus@...ba.org" <paulus@...ba.org>,
	"nuclearcat@...learcat.com" <nuclearcat@...learcat.com>
Subject: Re: [PATCH net] ppp: don't override sk->sk_state in
 pppoe_flush_dev()

On Tue, 2015-10-06 at 11:46 +0200, Guillaume Nault wrote:
> On Tue, Oct 06, 2015 at 04:46:04AM +0000, Matt Bennett wrote:
> > > > The second one seems to be trickier. It looks like a race wrt. PADT
> > > > message reception. Reproducing the bug will probably require to
> > > > generate some PADT flooding to a host that creates and releases PPPoE
> > > > connections.
> > 
> > Ok I think I can see the potential race here, specifically the PADT
> > frame is received while the pppoe interface is being deleted. (I will
> > have a go inducing this with msleep() in the code tomorrow)
> > 
> > 1. pppoe_flush_dev() - sk->sk_state = PPPOX_DEAD, po->pppoe_dev = NULL
> > 
> > 2. pppoe_connect() - sk->sk_state = PPPOX_NONE, po->pppoe_dev = NULL
> > 
> > 3. pppoe_disc_rcv() - sk->sk_state = PPPOX_ZOMBIE po->pppoe_dev = NULL
> > 
> > 4. pppoe_release() - dev_put(po->pppoe_dev) ----> Oops
> > 
> Again, I don't know why you introduce pppoe_connect() into the mix.
Sorry, I'm just going off the function calls I can see happening with my
kernel (3.16).
> But anyway, you got the point. Note that pppoe_flush_dev() could be
> replaced by other calls since we just need to reset po->pppoe_dev
> (another pppoe_unbind_sock_work() call, due to duplicated PADT, would
> also trigger the bug). Note also that pppoe_release() needs to be run
> before pppoe_unbind_sock_work() gets scheduled (or at least before it
> locks the socket).
> 
> > Either in pppoe_disc_rcv() we add the condition:
> > 
> > @@ -496,7 +499,8 @@ static int pppoe_disc_rcv(struct sk_buff *skb,
> > struct net_device *dev,
> >                         /* We're no longer connect at the PPPOE layer,
> >                          * and must wait for ppp channel to disconnect
> > us.
> >                          */
> > -                       sk->sk_state = PPPOX_ZOMBIE;
> > +                       if (sk->sk_state & PPPOX_CONNECTED)
> > +                               sk->sk_state = PPPOX_ZOMBIE;
> >                 }
> > 
> > Or perhaps we remove the assumption that the state PPPOX_ZOMBIE has a
> > non-null pppoe_dev on it.
> > 
> I don't think adding complexity in the socket state management would be
> a good think. Actually I event think about dropping the PPPOX_ZOMBIE
> state altogether. But that's probably something for net-next.
> 
> > I don't know why the code isn't like the following anyway.
> > 
> > -if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> > +if (po->pppoe_dev) {
> > 	dev_put(po->pppoe_dev);
> > 	po->pppoe_dev = NULL;
> > }
> I was thinking about that same approach. pppoe_release() is the only
> function making that assumption. Other parts of the code seem to only
> require that PPPOX_CONNECTED => pppoe_dev != NULL.
> 
> But I think the original condition was valid. Adding PPPOX_ZOMBIE into
> the test and resetting pppoe_dev upon reception of PADT have changed the
> relationship between sk_state and pppoe_dev, which is where the problem
> stands.
Yes originally the condition was valid. But I think the issue is plain
to see when you look at the comment beside PPPOX_ZOMBIE declared in the
enum.

 PPPOX_ZOMBIE	= 8,  /* dead, but still bound to ppp device */

We have seen in the situation we have described previously that we can
be in this state without being bound to the ppp device.

In my opinion the entire logic around
pppoe_disc_rcv()/pppoe_unbind_sock_work() looks wrong and I agree we
should do what you suggested a few emails back.

i.e in pppoe_disc_rcv():

if (po) {
		struct sock *sk = sk_pppox(po);

-		bh_lock_sock(sk);
-
-		/* If the user has locked the socket, just ignore
-		 * the packet.  With the way two rcv protocols hook into
-		 * one socket family type, we cannot (easily) distinguish
-		 * what kind of SKB it is during backlog rcv.
-		 */
-		if (sock_owned_by_user(sk) == 0) {
-			/* We're no longer connect at the PPPOE layer,
-			 * and must wait for ppp channel to disconnect us.
-			 */
-			sk->sk_state = PPPOX_ZOMBIE;
-		}
-
-		bh_unlock_sock(sk);
		if (!schedule_work(&po->proto.pppoe.padt_work))
			sock_put(sk);
	}

Subsequently the PPPOX_ZOMBIE state can be completely removed?

Matt