[PATCH] isdn/i4l: check the message proto does not change across fetches

In isdn_ppp_write(), the header (i.e., protobuf) of the buffer is fetched
twice from userspace. The first fetch is used to peek at the protocol
of the message and reset the huptimer if necessary; while the second
fetch copies in the whole buffer. However, given that buf resides in
userspace memory, a user process can race to change its memory content
across fetches. By doing so, we can either avoid resetting the huptimer
for any type of packets (by first setting proto to PPP_LCP and later
change to the actual type) or force resetting the huptimer for LCP packets.

This patch does a memcmp between the two fetches and abort if changes to
the protobuf is detected across fetches.

Signed-off-by: Meng Xu <redacted>
---
 drivers/isdn/i4l/isdn_ppp.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index 6c44609..21a9ae8 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c

@@ -857,6 +857,7 @@ isdn_ppp_write(int min, struct file *file, const char __user *buf, int count)
 		    (lp->flags & ISDN_NET_CONNECTED)) {
 			unsigned short hl;
 			struct sk_buff *skb;
+			void *skb_tail;
 			/*
 			 * we need to reserve enough space in front of
 			 * sk_buff. old call to dev_alloc_skb only reserved

@@ -869,11 +870,21 @@ isdn_ppp_write(int min, struct file *file, const char __user *buf, int count)
 				return count;
 			}
 			skb_reserve(skb, hl);
-			if (copy_from_user(skb_put(skb, count), buf, count))
+			skb_tail = skb_put(skb, count);
+			if (copy_from_user(skb_tail, buf, count))
 			{
 				kfree_skb(skb);
 				return -EFAULT;
 			}
+
+			/*
+			 * abort if the message proto is changed between the fetches
+			 */
+			if (memcmp(skb_tail, protobuf, 4)) {
+				kfree_skb(skb);
+				return -EFAULT;
+			}
+
 			if (is->debug & 0x40) {
 				printk(KERN_DEBUG "ppp xmit: len %d\n", (int) skb->len);
 				isdn_ppp_frame_log("xmit", skb->data, skb->len, 32, is->unit, lp->ppp_slot);

-- 
2.7.4

From: Meng Xu <redacted>
Date: Tue, 19 Sep 2017 14:52:58 -0400

In isdn_ppp_write(), the header (i.e., protobuf) of the buffer is fetched
twice from userspace. The first fetch is used to peek at the protocol
of the message and reset the huptimer if necessary; while the second
fetch copies in the whole buffer. However, given that buf resides in
userspace memory, a user process can race to change its memory content
across fetches. By doing so, we can either avoid resetting the huptimer
for any type of packets (by first setting proto to PPP_LCP and later
change to the actual type) or force resetting the huptimer for LCP packets.

This patch does a memcmp between the two fetches and abort if changes to
the protobuf is detected across fetches.

Signed-off-by: Meng Xu <redacted>

Doing a memcmp() for every buffer is expensive, ugly, and not the
way we usually handle this kind of issue.

Instead, atomically copy the entire buffer, as needed.

Something like:

	struct sk_buff *skb = NULL;
	unsigned char protobuf[4];
	unsigned char *cpy_buf;

	if (lp->isdn_device >= 0 && lp->isdn_channel >= 0 &&
	    (dev->drv[lp->isdn_device]->flags & DRV_FLAG_RUNNING) &&
	    lp->dialstate == 0 &&
	    (lp->flags & ISDN_NET_CONNECTED)) {
			/*
			 * we need to reserve enough space in front of
			 * sk_buff. old call to dev_alloc_skb only reserved
			 * 16 bytes, now we are looking what the driver want
			 */
			hl = dev->drv[lp->isdn_device]->interface->hl_hdrlen;
			skb = alloc_skb(hl + count, GFP_ATOMIC);
			if (!skb) {
				printk(KERN_WARNING "isdn_ppp_write: out of memory!\n");
				return count;
			}
			skb_reserve(skb, hl);
			cpy_buf = skb_put(skb, count);
	} else {
		cpy_buf = protobuf;
		count = sizeof(protobuf);
	}
	if (copy_from_user(cpy_buf, buf, count)) {
		kfree_skb(skb);
		return -EFAULT;
	}
	proto = PPP_PROTOCOL(cpy_buf);
	if (proto != PPP_LCP)
		lp->huptimer = 0;
	...