ppp: fix locking issues related to ppp_ioctl()

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

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.

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

index fc8ad00..4d342ae 100644
@@ -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

From: Guillaume Nault <[email protected]>
Date: Mon, 22 Feb 2016 20:47:13 +0100

...

Ok, I didn't think we could assume atomic stores for int on all arch.

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.

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

From: Guillaume Nault <[email protected]>
Date: Thu, 25 Feb 2016 20:16:55 +0100

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

Thanks.

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.

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

index 4d342ae..4af548b 100644
@@ -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

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.

---
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(-)

index 4af548b..183d89c 100644
@@ -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

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.

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

index 183d89c..24777f4 100644
@@ -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

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().

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

index 24777f4..2eb39cd 100644
@@ -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