ppp: ensure file->private_data can't be overridden

commit e8e56ffd9d2973398b60ece1f1bebb8d67b4d032 upstream.

Locking ppp_mutex must be done before dereferencing file->private_data,
otherwise it could be modified before ppp_unattached_ioctl() takes the
lock. This could lead ppp_unattached_ioctl() to override ->private_data,
thus leaking reference to the ppp_file previously pointed to.

v2: lock all ppp_ioctl() instead of just checking private_data in
    ppp_unattached_ioctl(), to avoid ambiguous behaviour.

Fixes: f3ff8a4d80e8 ("ppp: push BKL down into the driver")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 82d4421..e948e53 100644 (file)
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -555,7 +555,7 @@ static int get_filter(void __user *arg, struct sock_filter **p)
 
 static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 
 static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {

-       struct ppp_file *pf = file->private_data;
+       struct ppp_file *pf;

        struct ppp *ppp;
        int err = -EFAULT, val, val2, i;
        struct ppp_idle idle;
        struct ppp *ppp;
        int err = -EFAULT, val, val2, i;
        struct ppp_idle idle;


@@ -565,9 +565,14 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
        void __user *argp = (void __user *)arg;
        int __user *p = argp;
 
        void __user *argp = (void __user *)arg;
        int __user *p = argp;
 

-       if (!pf)
-               return ppp_unattached_ioctl(current->nsproxy->net_ns,
-                                       pf, file, cmd, arg);
+       mutex_lock(&ppp_mutex);
+
+       pf = file->private_data;
+       if (!pf) {
+               err = ppp_unattached_ioctl(current->nsproxy->net_ns,
+                                          pf, file, cmd, arg);
+               goto out;
+       }

 
        if (cmd == PPPIOCDETACH) {
                /*
 
        if (cmd == PPPIOCDETACH) {
                /*


@@ -582,7 +587,6 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                 * this fd and reopening /dev/ppp.
                 */
                err = -EINVAL;
                 * this fd and reopening /dev/ppp.
                 */
                err = -EINVAL;

-               mutex_lock(&ppp_mutex);

                if (pf->kind == INTERFACE) {
                        ppp = PF_TO_PPP(pf);
                        if (file == ppp->owner)
                if (pf->kind == INTERFACE) {
                        ppp = PF_TO_PPP(pf);
                        if (file == ppp->owner)


@@ -594,15 +598,13 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                } else
                        pr_warn("PPPIOCDETACH file->f_count=%ld\n",
                                atomic_long_read(&file->f_count));
                } else
                        pr_warn("PPPIOCDETACH file->f_count=%ld\n",
                                atomic_long_read(&file->f_count));

-               mutex_unlock(&ppp_mutex);
-               return err;
+               goto out;

        }
 
        if (pf->kind == CHANNEL) {
                struct channel *pch;
                struct ppp_channel *chan;
 
        }
 
        if (pf->kind == CHANNEL) {
                struct channel *pch;
                struct ppp_channel *chan;
 

-               mutex_lock(&ppp_mutex);

                pch = PF_TO_CHANNEL(pf);
 
                switch (cmd) {
                pch = PF_TO_CHANNEL(pf);
 
                switch (cmd) {


@@ -624,17 +626,16 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
                                err = chan->ops->ioctl(chan, cmd, arg);
                        up_read(&pch->chan_sem);
                }
                                err = chan->ops->ioctl(chan, cmd, arg);
                        up_read(&pch->chan_sem);
                }

-               mutex_unlock(&ppp_mutex);
-               return err;
+               goto out;

        }
 
        if (pf->kind != INTERFACE) {
                /* can't happen */
                pr_err("PPP: not interface or channel??\n");
        }
 
        if (pf->kind != INTERFACE) {
                /* can't happen */
                pr_err("PPP: not interface or channel??\n");

-               return -EINVAL;
+               err = -EINVAL;
+               goto out;

        }
 
        }
 

-       mutex_lock(&ppp_mutex);

        ppp = PF_TO_PPP(pf);
        switch (cmd) {
        case PPPIOCSMRU:
        ppp = PF_TO_PPP(pf);
        switch (cmd) {
        case PPPIOCSMRU:


@@ -781,7 +782,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
        default:
                err = -ENOTTY;
        }
        default:
                err = -ENOTTY;
        }

+
+out:

        mutex_unlock(&ppp_mutex);
        mutex_unlock(&ppp_mutex);

+

        return err;
 }
 
        return err;
 }
 


@@ -794,7 +798,6 @@ static int ppp_unattached_ioctl(struct net *net, struct ppp_file *pf,
        struct ppp_net *pn;
        int __user *p = (int __user *)arg;
 
        struct ppp_net *pn;
        int __user *p = (int __user *)arg;
 

-       mutex_lock(&ppp_mutex);

        switch (cmd) {
        case PPPIOCNEWUNIT:
                /* Create a new ppp unit */
        switch (cmd) {
        case PPPIOCNEWUNIT:
                /* Create a new ppp unit */


@@ -845,7 +848,7 @@ static int ppp_unattached_ioctl(struct net *net, struct ppp_file *pf,
        default:
                err = -ENOTTY;
        }
        default:
                err = -ENOTTY;
        }

-       mutex_unlock(&ppp_mutex);
+

        return err;
 }
 
        return err;
 }