[PATCH net] ppp: ensure file->private_data can't be overridden

[PATCH net] ppp: ensure file->private_data can't be overridden

[Guillaume Nault]

Lock ppp_mutex and check that file->private_data is NULL before
executing any action in ppp_unattached_ioctl().
The test done by ppp_ioctl() can't be relied upon, because
file->private_data may have been updated meanwhile. In which case
ppp_unattached_ioctl() will override file->private_data and mess up
reference counters or loose pointer to previously allocated PPP unit.

In case the test fails, -ENOTTY is returned, just like if ppp_ioctl()
had rejected the ioctl in the first place.

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
This seems to originate in BKL removal :
f3ff8a4d80e8 ("ppp: push BKL down into the driver") moved the test on
file->private_data (in ppp_ioctl()) out of BKL protection. BKL was then
replaced by ppp_mutex in 15fd0cd9a2ad ("net: autoconvert trivial BKL
users to private mutex").

 drivers/net/ppp/ppp_generic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index d61da9ec..d1dbcb6 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -845,6 +845,11 @@ static int ppp_unattached_ioctl(struct net *net, struct ppp_file *pf,
 	int __user *p = (int __user *)arg;
 
 	mutex_lock(&ppp_mutex);
+	if (file->private_data) {
+		mutex_unlock(&ppp_mutex);
+		return -ENOTTY;
+	}
+
 	switch (cmd) {
 	case PPPIOCNEWUNIT:
 		/* Create a new ppp unit */
-- 
2.7.0

Re: [PATCH net] ppp: ensure file->private_data can't be overridden

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Tue, 8 Mar 2016 20:14:30 +0100

> Lock ppp_mutex and check that file->private_data is NULL before
> executing any action in ppp_unattached_ioctl().
> The test done by ppp_ioctl() can't be relied upon, because
> file->private_data may have been updated meanwhile. In which case
> ppp_unattached_ioctl() will override file->private_data and mess up
> reference counters or loose pointer to previously allocated PPP unit.
> 
> In case the test fails, -ENOTTY is returned, just like if ppp_ioctl()
> had rejected the ioctl in the first place.
> 
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

If this thing can disappear on us, then we need to make the entirety
of ppp_ioctl() run with the mutex held to fix this properly.

Otherwise ->private_data could go NULL on us meanwhile as well.

We should hold the mutex, to stabilize the value of ->private_data.

Re: [PATCH net] ppp: ensure file->private_data can't be overridden

[Guillaume Nault]

On Fri, Mar 11, 2016 at 02:42:16PM -0500, David Miller wrote:
> From: Guillaume Nault <g.nault@alphalink.fr>
> Date: Tue, 8 Mar 2016 20:14:30 +0100
> 
> > Lock ppp_mutex and check that file->private_data is NULL before
> > executing any action in ppp_unattached_ioctl().
> > The test done by ppp_ioctl() can't be relied upon, because
> > file->private_data may have been updated meanwhile. In which case
> > ppp_unattached_ioctl() will override file->private_data and mess up
> > reference counters or loose pointer to previously allocated PPP unit.
> > 
> > In case the test fails, -ENOTTY is returned, just like if ppp_ioctl()
> > had rejected the ioctl in the first place.
> > 
> > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
> 
> If this thing can disappear on us, then we need to make the entirety
> of ppp_ioctl() run with the mutex held to fix this properly.
> 
> Otherwise ->private_data could go NULL on us meanwhile as well.
> 
> We should hold the mutex, to stabilize the value of ->private_data.

Actually, only ppp_release() can reset ->private_data to NULL. Beyond
closing the file's last reference, the only way to trigger it is
to run the PPPIOCDETACH ioctl. But even then, ppp_release() isn't
called if the file has more than one reference.
So ->private_data should never go NULL from under another user.

As for setting ->private_data to non-NULL value, this is exclusively
handled by ppp_unattached_ioctl(). Since the ppp_mutex is held at the
beginning of the function, calls are serialised, but one may still
overwrite ->private_data and leak the memory previously pointed to.
By testing ->private_data with ppp_mutex held, this patch fixes this
issue, and ->private_data is now guaranteed to remain constant after
it's been set.

Testing ->private_data without lock in ppp_ioctl() before calling
ppp_unattached_ioctl() is fine, because either ->private_data is
not NULL and thus is stable, or it is and ppp_unattached_ioctl()
takes care of not overriding ->private_data, should its value get
modified before taking the mutex.


I considered moving ppp_mutex up to cover the entirety of ppp_ioctl()
too, but finally choosed to handle everything in ppp_unattached_ioctl()
because that's where the problem really stands.
ppp_ioctl() takes the mutex for historical reasons (semi-automatic BKL
removal) and there are several places where holding ppp_mutex seems
unnecessary (e.g. for PPPIOCDETACH). So I felt the right direction was
to move ppp_mutex further down rather than moving it up to cover the
entirety of ppp_ioctl().

In particular, with regard to adding rtnetlink handlers for PPP (which
is the objective that lead to those PPP fixes), holding ppp_mutex for
too long is a problem. An rtnetlink handler would run under protection
of the rtnl mutex, and would need to grab ppp_mutex too (unless we
don't associate the PPP unit fd to the net device in the .newlink
callback).
But currently the PPPIOCNEWUNIT ioctl holds ppp_mutex before taking the
rtnl mutex (in ppp_create_interface()). In this context moving
ppp_mutex up to ppp_ioctl() makes things more difficult because what's
required is, on the contrary, moving it further down so that it gets
held after the rtnl mutex.
However I'd agree that such consideration shouldn't come into play for
fixes on net. It weighted a bit in my decision to not push ppp_mutex
up though.

Re: [PATCH net] ppp: ensure file->private_data can't be overridden

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Mon, 14 Mar 2016 18:59:40 +0100

> Testing ->private_data without lock in ppp_ioctl() before calling
> ppp_unattached_ioctl() is fine, because either ->private_data is
> not NULL and thus is stable, or it is and ppp_unattached_ioctl()
> takes care of not overriding ->private_data, should its value get
> modified before taking the mutex.

This is exactly the ambiguous behavior I want you to avoid.

The decision should be atomic from ppp_ioctl()'s test all the way
until ppp_unattached_ioctl() does it's work.

Re: [PATCH net] ppp: ensure file->private_data can't be overridden

[Guillaume Nault]

On Mon, Mar 14, 2016 at 02:57:59PM -0400, David Miller wrote:
> From: Guillaume Nault <g.nault@alphalink.fr>
> Date: Mon, 14 Mar 2016 18:59:40 +0100
> 
> > Testing ->private_data without lock in ppp_ioctl() before calling
> > ppp_unattached_ioctl() is fine, because either ->private_data is
> > not NULL and thus is stable, or it is and ppp_unattached_ioctl()
> > takes care of not overriding ->private_data, should its value get
> > modified before taking the mutex.
> 
> This is exactly the ambiguous behavior I want you to avoid.
> 
> The decision should be atomic from ppp_ioctl()'s test all the way
> until ppp_unattached_ioctl() does it's work.

OK, will fix in v2.