[PATCH net] ppp: prevent unregistered channels from connecting to PPP units

[PATCH net] ppp: prevent unregistered channels from connecting to PPP units

[Guillaume Nault]

PPP units don't hold any reference on the channels connected to it.
It is the channel's responsibility to ensure that it disconnects from
its unit before being destroyed.
In practice, this is ensured by ppp_unregister_channel() disconnecting
the channel from the unit before dropping a reference on the channel.

However, it is possible for an unregistered channel to connect to a PPP
unit: register a channel with ppp_register_net_channel(), attach a
/dev/ppp file to it with ioctl(PPPIOCATTCHAN), unregister the channel
with ppp_unregister_channel() and finally connect the /dev/ppp file to
a PPP unit with ioctl(PPPIOCCONNECT).

Once in this situation, the channel is only held by the /dev/ppp file,
which can be released at anytime and free the channel without letting
the parent PPP unit know. Then the ppp structure ends up with dangling
pointers in its ->channels list.

Prevent this scenario by forbidding unregistered channels from
connecting to PPP units. This maintains the code logic by keeping
ppp_unregister_channel() responsible from disconnecting the channel if
necessary and avoids modification on the reference counting mechanism.

This issue seems to predate git history (successfully reproduced on
Linux 2.6.26 and earlier PPP commits are unrelated).

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 255a5def56e9..fa2a9bdd1866 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -3161,6 +3161,15 @@ ppp_connect_channel(struct channel *pch, int unit)
 		goto outl;
 
 	ppp_lock(ppp);
+	spin_lock_bh(&pch->downl);
+	if (!pch->chan) {
+		/* Don't connect unregistered channels */
+		spin_unlock_bh(&pch->downl);
+		ppp_unlock(ppp);
+		ret = -ENOTCONN;
+		goto outl;
+	}
+	spin_unlock_bh(&pch->downl);
 	if (pch->file.hdrlen > ppp->file.hdrlen)
 		ppp->file.hdrlen = pch->file.hdrlen;
 	hdrlen = pch->file.hdrlen + 2;	/* for protocol bytes */
-- 
2.16.2

Re: [PATCH net] ppp: prevent unregistered channels from connecting to PPP units

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Fri, 2 Mar 2018 18:41:16 +0100

> PPP units don't hold any reference on the channels connected to it.
> It is the channel's responsibility to ensure that it disconnects from
> its unit before being destroyed.
> In practice, this is ensured by ppp_unregister_channel() disconnecting
> the channel from the unit before dropping a reference on the channel.
> 
> However, it is possible for an unregistered channel to connect to a PPP
> unit: register a channel with ppp_register_net_channel(), attach a
> /dev/ppp file to it with ioctl(PPPIOCATTCHAN), unregister the channel
> with ppp_unregister_channel() and finally connect the /dev/ppp file to
> a PPP unit with ioctl(PPPIOCCONNECT).
> 
> Once in this situation, the channel is only held by the /dev/ppp file,
> which can be released at anytime and free the channel without letting
> the parent PPP unit know. Then the ppp structure ends up with dangling
> pointers in its ->channels list.
> 
> Prevent this scenario by forbidding unregistered channels from
> connecting to PPP units. This maintains the code logic by keeping
> ppp_unregister_channel() responsible from disconnecting the channel if
> necessary and avoids modification on the reference counting mechanism.
> 
> This issue seems to predate git history (successfully reproduced on
> Linux 2.6.26 and earlier PPP commits are unrelated).
> 
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

Applied and queued up for -stable, thank you.