[PATCH net 0/5] ppp: fix locking issues related to ppp_ioctl()

[PATCH net 0/5] ppp: fix locking issues related to ppp_ioctl()

[Guillaume Nault]

This series fixes unprotected accesses to several struct ppp fields.
Only fields used in ppp_ioctl() have been considered, though.

Locking of the xstate and rstate fields remains incomplete: although a
side effect of patch #2 provides protection in ppp_ioctl(), xstate and
rstate can still be modified without appropriate locking by
ppp_ccp_peek(). Taking the missing locks in ppp_ccp_peek() isn't
possible as this would lead to lock inversion (when protecting xstate
with ppp_xmit_lock() while ppp_ccp_peek() is called in the Rx path).

Using a workqueue to run ppp_ccp_peek() might be a solution, but this
is left for another series.

Guillaume Nault (5):
  ppp: lock ppp structure before modifying mru in ppp_ioctl()
  ppp: fix unprotected accesses to ppp->flags and ppp->n_channels
  ppp: protect ppp->debug in ppp_ioctl()
  ppp: protect access to ppp->last{xmit,recv} in ppp_ioctl()
  ppp: protect ppp->npmode

 drivers/net/ppp/ppp_generic.c | 42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

-- 
2.7.0

[PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[Guillaume Nault]

PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
and ppp_recv_lock() respectively.
Therefore ppp_ioctl() must hold the xmit and recv locks before
concurrently updating ppp->mru.

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

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fc8ad00..4d342ae 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case PPPIOCSMRU:
 		if (get_user(val, p))
 			break;
+		ppp_lock(ppp);
 		ppp->mru = val;
+		ppp_unlock(ppp);
+
 		err = 0;
 		break;
 
-- 
2.7.0

Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Mon, 22 Feb 2016 20:47:13 +0100

> PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
> and ppp_recv_lock() respectively.
> Therefore ppp_ioctl() must hold the xmit and recv locks before
> concurrently updating ppp->mru.
> 
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
 ...
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index fc8ad00..4d342ae 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case PPPIOCSMRU:
>  		if (get_user(val, p))
>  			break;
> +		ppp_lock(ppp);
>  		ppp->mru = val;
> +		ppp_unlock(ppp);
> +

I see no bug here at all.

The store here is atomic, and all of those mentioned code paths only
read the MRU once and then use that value for the duration of the
rest of the processing of that PPP frame.

No possible corruptions or misbehavior can occur and I therefore think
the lack of locking here is completely legitimate.

You absolutely must demonstrate a case of corruption or misbehavior
when you want to add supposedly "missing locking".  Otherwise I'll have
a hard time accepting your changes.  This is especially for a subsystem
that as been around as long as PPP.

Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[Guillaume Nault]

On Wed, Feb 24, 2016 at 03:32:02PM -0500, David Miller wrote:
> From: Guillaume Nault <g.nault@alphalink.fr>
> Date: Mon, 22 Feb 2016 20:47:13 +0100
> 
> > PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
> > and ppp_recv_lock() respectively.
> > Therefore ppp_ioctl() must hold the xmit and recv locks before
> > concurrently updating ppp->mru.
> > 
> > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
>  ...
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index fc8ad00..4d342ae 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >  	case PPPIOCSMRU:
> >  		if (get_user(val, p))
> >  			break;
> > +		ppp_lock(ppp);
> >  		ppp->mru = val;
> > +		ppp_unlock(ppp);
> > +
> 
> I see no bug here at all.
> 
> The store here is atomic, and all of those mentioned code paths only
> read the MRU once and then use that value for the duration of the
> rest of the processing of that PPP frame.
> 
Ok, I didn't think we could assume atomic stores for int on all arch.

> No possible corruptions or misbehavior can occur and I therefore think
> the lack of locking here is completely legitimate.
> 
Then this is also legitimate for most of the other fields considered in
this series. I'll drop the patches.

One exception is the n_channels and flags fields (patch #2). The update
side is done with read-modify-write instructions ('ppp->flags &= ~XXX'
in ppp_ccp_closed(), '++ppp->n_channels' in ppp_connect_channel()). So
locking should be required. I haven't succeeded in triggering any
misbehaviour from userspace though.

> You absolutely must demonstrate a case of corruption or misbehavior
> when you want to add supposedly "missing locking".  Otherwise I'll have
> a hard time accepting your changes.  This is especially for a subsystem
> that as been around as long as PPP.
Understood. Just to be sure, does patch #2 falls under lack of
demonstration? Or should I repost it separately?

Re: [PATCH net 1/5] ppp: lock ppp structure before modifying mru in ppp_ioctl()

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Thu, 25 Feb 2016 20:16:55 +0100

> Understood. Just to be sure, does patch #2 falls under lack of
> demonstration? Or should I repost it separately?

Please repost it if you feel this way, with the race and corruption
possibility explained in detail in the commit log message.

Thanks.

[PATCH net 2/5] ppp: fix unprotected accesses to ppp->flags and ppp->n_channels

[Guillaume Nault]

Reading ppp->flags has to be done under protection of ppp_recv_lock()
or ppp_xmit_lock() (for Rx and Tx paths respectively). Therefore, both
locks have to be held before writing.

This patch adds missing locking in ppp_read(), ppp_poll() and
ppp_ioctl(). As a side effect, it fixes unprotected access to
ppp->n_channels in ppp_read() and ppp_poll(), which has the same
locking requirements as ppp->flags.

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

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4d342ae..4af548b 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -443,9 +443,14 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
 			 * network traffic (demand mode).
 			 */
 			struct ppp *ppp = PF_TO_PPP(pf);
+
+			ppp_recv_lock(ppp);
 			if (ppp->n_channels == 0 &&
-			    (ppp->flags & SC_LOOP_TRAFFIC) == 0)
+			    (ppp->flags & SC_LOOP_TRAFFIC) == 0) {
+				ppp_recv_unlock(ppp);
 				break;
+			}
+			ppp_recv_unlock(ppp);
 		}
 		ret = -EAGAIN;
 		if (file->f_flags & O_NONBLOCK)
@@ -532,9 +537,12 @@ static unsigned int ppp_poll(struct file *file, poll_table *wait)
 	else if (pf->kind == INTERFACE) {
 		/* see comment in ppp_read */
 		struct ppp *ppp = PF_TO_PPP(pf);
+
+		ppp_recv_lock(ppp);
 		if (ppp->n_channels == 0 &&
 		    (ppp->flags & SC_LOOP_TRAFFIC) == 0)
 			mask |= POLLIN | POLLRDNORM;
+		ppp_recv_unlock(ppp);
 	}
 
 	return mask;
@@ -678,7 +686,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PPPIOCGFLAGS:
+		ppp_lock(ppp);
 		val = ppp->flags | ppp->xstate | ppp->rstate;
+		ppp_unlock(ppp);
+
 		if (put_user(val, p))
 			break;
 		err = 0;
-- 
2.7.0

[PATCH net 3/5] ppp: protect ppp->debug in ppp_ioctl()

[Guillaume Nault]

ppp->debug is read in the Tx and Rx paths while under protection of
ppp_xmit_lock() and ppp_recv_lock() respectively.
So ppp_ioctl() must hold both locks before concurrently updating it.

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
Locking is not strictly necessary for PPPIOCGDEBUG, because ppp->debug
can only be modified by ioctl(PPPIOCSDEBUG) which is guaranteed not to
run concurrently thanks to ppp_mutex. I've added the locking in
PPPIOCGDEBUG in order to respect the general locking semantic of
ppp->debug and to avoid relying on ppp_mutex.

 drivers/net/ppp/ppp_generic.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 4af548b..183d89c 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -708,12 +708,19 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case PPPIOCSDEBUG:
 		if (get_user(val, p))
 			break;
+		ppp_lock(ppp);
 		ppp->debug = val;
+		ppp_unlock(ppp);
+
 		err = 0;
 		break;
 
 	case PPPIOCGDEBUG:
-		if (put_user(ppp->debug, p))
+		ppp_lock(ppp);
+		val = ppp->debug;
+		ppp_unlock(ppp);
+
+		if (put_user(val, p))
 			break;
 		err = 0;
 		break;
-- 
2.7.0

[PATCH net 4/5] ppp: protect access to ppp->last_{xmit,recv} in ppp_ioctl()

[Guillaume Nault]

ppp->last_xmit is witten to by ppp_send_frame() while protected by
ppp_xmit_lock().
Likewise, ppp->last_recv is written to by ppp_receive_nonmp_frame()
while protected by ppp_recv_lock().

Holding both locks is therefore necessary before ppp_ioctl() can safely
read these fields.

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

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 183d89c..24777f4 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -726,8 +726,11 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		break;
 
 	case PPPIOCGIDLE:
+		ppp_lock(ppp);
 		idle.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
 		idle.recv_idle = (jiffies - ppp->last_recv) / HZ;
+		ppp_unlock(ppp);
+
 		if (copy_to_user(argp, &idle, sizeof(idle)))
 			break;
 		err = 0;
-- 
2.7.0

[PATCH net 5/5] ppp: protect ppp->npmode

[Guillaume Nault]

ppp->npmode is read by ppp_ioctl(), ppp_receive_nonmp_frame() and
ppp_start_xmit(). But it is only modified by ppp_ioctl().
However, the only protected access is done by ppp_receive_nonmp_frame()
which runs under ppp_recv_lock() protection.

We could protect ppp->npmode with ppp_recv_lock() in ppp_start_xmit()
too, but taking the recv lock in the xmit path would look strange. So
this patch takes the xmit lock instead and holds both locks before
writing in ppp_ioctl().

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

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 24777f4..2eb39cd 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -767,11 +767,18 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		i = err;
 		if (cmd == PPPIOCGNPMODE) {
 			err = -EFAULT;
+
+			ppp_lock(ppp);
 			npi.mode = ppp->npmode[i];
+			ppp_unlock(ppp);
+
 			if (copy_to_user(argp, &npi, sizeof(npi)))
 				break;
 		} else {
+			ppp_lock(ppp);
 			ppp->npmode[i] = npi.mode;
+			ppp_unlock(ppp);
+
 			/* we may be able to transmit more packets now (??) */
 			netif_wake_queue(ppp->dev);
 		}
@@ -1023,13 +1030,18 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct ppp *ppp = netdev_priv(dev);
 	int npi, proto;
 	unsigned char *pp;
+	enum NPmode npmode;
 
 	npi = ethertype_to_npindex(ntohs(skb->protocol));
 	if (npi < 0)
 		goto outf;
 
+	ppp_xmit_lock(ppp);
+	npmode = ppp->npmode[npi];
+	ppp_xmit_unlock(ppp);
+
 	/* Drop, accept or reject the packet */
-	switch (ppp->npmode[npi]) {
+	switch (npmode) {
 	case NPMODE_PASS:
 		break;
 	case NPMODE_QUEUE:
-- 
2.7.0