[PATCH 1/2] ppp_generic: pull 2 bytes so that PPP_PROTO(skb) is valid

[PATCH 1/2] ppp_generic: pull 2 bytes so that PPP_PROTO(skb) is valid

[Simon Arlott]

In ppp_input(), PPP_PROTO(skb) may refer to invalid data in the skb.

If this happens and (proto >= 0xc000 || proto == PPP_CCPFRAG) then
the packet is passed directly to pppd.

This occurs frequently when using PPPoE with an interface MTU
greater than 1500 because the skb is more likely to be non-linear.

The next 2 bytes need to be pulled in ppp_input(). The pull of 2
bytes in ppp_receive_frame() has been removed as it is no longer
required.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
Tested with PPPoE over e1000 at MTU 16110.

 drivers/net/ppp_generic.c |   28 ++++++++++++++++++----------
 1 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 6e281bc..fdd8deb 100644
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -1572,8 +1572,18 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 		return;
 	}
 
-	proto = PPP_PROTO(skb);
+
 	read_lock_bh(&pch->upl);
+	if (!pskb_may_pull(skb, 2)) {
+		kfree_skb(skb);
+		if (pch->ppp) {
+			++pch->ppp->dev->stats.rx_length_errors;
+			ppp_receive_error(pch->ppp);
+		}
+		goto done;
+	}
+
+	proto = PPP_PROTO(skb);
 	if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {
 		/* put it on the channel queue */
 		skb_queue_tail(&pch->file.rq, skb);
@@ -1585,6 +1595,8 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 	} else {
 		ppp_do_recv(pch->ppp, skb, pch);
 	}
+
+done:
 	read_unlock_bh(&pch->upl);
 }
 
@@ -1617,7 +1629,8 @@ ppp_input_error(struct ppp_channel *chan, int code)
 static void
 ppp_receive_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 {
-	if (pskb_may_pull(skb, 2)) {
+	/* note: a 0-length skb is used as an error indication */
+	if (skb->len > 0) {
 #ifdef CONFIG_PPP_MULTILINK
 		/* XXX do channel-level decompression here */
 		if (PPP_PROTO(skb) == PPP_MP)
@@ -1625,15 +1638,10 @@ ppp_receive_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 		else
 #endif /* CONFIG_PPP_MULTILINK */
 			ppp_receive_nonmp_frame(ppp, skb);
-		return;
+	} else {
+		kfree_skb(skb);
+		ppp_receive_error(ppp);
 	}
-
-	if (skb->len > 0)
-		/* note: a 0-length skb is used as an error indication */
-		++ppp->dev->stats.rx_length_errors;
-
-	kfree_skb(skb);
-	ppp_receive_error(ppp);
 }
 
 static void
-- 
1.7.0.4

-- 
Simon Arlott

[PATCH 2/2] ppp_generic: linearise skbs before passing them to pppd

[Simon Arlott]

Frequently when using PPPoE with an interface MTU greater than 1500,
the skb is likely to be non-linear. If the skb needs to be passed to
pppd then the skb must be linearised first.

The previous commit fixes an issue with accidentally sending skbs
to pppd based on an invalid read of the protocol type. When that
error occurred pppd was reading invalid skb data too.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
Tested with PPPoE over e1000 at MTU 16110.

 drivers/net/ppp_generic.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index fdd8deb..6855a7b 100644
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -1222,6 +1222,8 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
 	if (ppp->flags & SC_LOOP_TRAFFIC) {
 		if (ppp->file.rq.qlen > PPP_MAX_RQLEN)
 			goto drop;
+		if (skb_linearize(skb))
+			goto drop;
 		skb_queue_tail(&ppp->file.rq, skb);
 		wake_up_interruptible(&ppp->file.rwait);
 		return;
@@ -1586,6 +1588,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 	proto = PPP_PROTO(skb);
 	if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {
 		/* put it on the channel queue */
+		if (skb_linearise(skb)) {
+			kfree_skb(skb);
+			if (pch->ppp)
+				ppp_receive_error(pch->ppp);
+			goto done;
+		}
 		skb_queue_tail(&pch->file.rq, skb);
 		/* drop old frames if queue too long */
 		while (pch->file.rq.qlen > PPP_MAX_RQLEN &&
@@ -1733,6 +1741,8 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
 	npi = proto_to_npindex(proto);
 	if (npi < 0) {
 		/* control or unknown frame - pass it to pppd */
+		if (skb_linearize(skb))
+			goto err;
 		skb_queue_tail(&ppp->file.rq, skb);
 		/* limit queue length by dropping old frames */
 		while (ppp->file.rq.qlen > PPP_MAX_RQLEN &&
-- 
1.7.0.4

-- 
Simon Arlott

Re: [PATCH 2/2] ppp_generic: linearise skbs before passing them to pppd

[David Miller]

From: Simon Arlott <simon@fire.lp0.eu>
Date: Fri, 30 Apr 2010 19:41:45 +0100

> Frequently when using PPPoE with an interface MTU greater than 1500,
> the skb is likely to be non-linear. If the skb needs to be passed to
> pppd then the skb must be linearised first.
> 
> The previous commit fixes an issue with accidentally sending skbs
> to pppd based on an invalid read of the protocol type. When that
> error occurred pppd was reading invalid skb data too.
> 
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>

Don't propagate stupidity.

The real problem is that ppp_read() can't handle non-linear SKBs, so
fix that instead.  The easiest way to do that is to put a "struct
iovec iov;" on ppp_read()'s stack, fill it in with ther user buffer
pointer and length, then use that to call
skb_copy_datagram_const_iovec().

[PATCH 2/2] ppp_generic: handle non-linear skbs when passing them to pppd

[Simon Arlott]

Frequently when using PPPoE with an interface MTU greater than 1500,
the skb is likely to be non-linear. If the skb needs to be passed to
pppd then the skb data must be read correctly.

The previous commit fixes an issue with accidentally sending skbs
to pppd based on an invalid read of the protocol type. When that
error occurred pppd was reading invalid skb data too.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
On 03/05/10 07:27, David Miller wrote:
> From: Simon Arlott <simon@fire.lp0.eu>
> Date: Fri, 30 Apr 2010 19:41:45 +0100
> 
>> Frequently when using PPPoE with an interface MTU greater than 1500,
>> the skb is likely to be non-linear. If the skb needs to be passed to
>> pppd then the skb must be linearised first.
> 
> Don't propagate stupidity.
> 
> The real problem is that ppp_read() can't handle non-linear SKBs, so
> fix that instead.  The easiest way to do that is to put a "struct
> iovec iov;" on ppp_read()'s stack, fill it in with ther user buffer
> pointer and length, then use that to call
> skb_copy_datagram_const_iovec().

I've updated it to use skb_copy_datagram_iovec():

 drivers/net/ppp_generic.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 75e8903..8518a2e 100644
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -405,6 +405,7 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
 	DECLARE_WAITQUEUE(wait, current);
 	ssize_t ret;
 	struct sk_buff *skb = NULL;
+	struct iovec iov;
 
 	ret = count;
 
@@ -448,7 +449,9 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
 	if (skb->len > count)
 		goto outf;
 	ret = -EFAULT;
-	if (copy_to_user(buf, skb->data, skb->len))
+	iov.iov_base = buf;
+	iov.iov_len = count;
+	if (skb_copy_datagram_iovec(skb, 0, &iov, skb->len))
 		goto outf;
 	ret = skb->len;
 
-- 
1.7.0.4

-- 
Simon Arlott

Re: [PATCH 1/2] ppp_generic: pull 2 bytes so that PPP_PROTO(skb) is valid

[David Miller]

From: Simon Arlott <simon@fire.lp0.eu>
Date: Fri, 30 Apr 2010 19:41:17 +0100

> @@ -1572,8 +1572,18 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
>  		return;
>  	}
>  
> -	proto = PPP_PROTO(skb);
> +
>  	read_lock_bh(&pch->upl);
> +	if (!pskb_may_pull(skb, 2)) {
> +		kfree_skb(skb);
> +		if (pch->ppp) {
> +			++pch->ppp->dev->stats.rx_length_errors;
> +			ppp_receive_error(pch->ppp);
> +		}
> +		goto done;
> +	}
> +
> +	proto = PPP_PROTO(skb);

This makes the skb->len == 0 test at the beginning completely redundant.

Put your pskb_may_pull(skb, 2) call there and remove the skb->len==0
check entirely.

Re: [PATCH 1/2] ppp_generic: pull 2 bytes so that PPP_PROTO(skb) is valid

[Simon Arlott]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Mon, May 3, 2010 07:25, David Miller wrote:
> From: Simon Arlott <simon@fire.lp0.eu>
> Date: Fri, 30 Apr 2010 19:41:17 +0100
>> @@ -1572,8 +1572,18 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
>>  		return;
>>  	}
>>
>> -	proto = PPP_PROTO(skb);
>> +
>>  	read_lock_bh(&pch->upl);
>> +	if (!pskb_may_pull(skb, 2)) {
>
> This makes the skb->len == 0 test at the beginning completely redundant.
>
> Put your pskb_may_pull(skb, 2) call there and remove the skb->len==0
> check entirely.

If I move pskb_may_pull(skb, 2) up to where skb->len == 0 is then it can't
increment rx_length_errors because it doesn't have the read lock on pch->upl,
so I can only remove the redundant skb->len == 0 if that error count is to
remain.

Updated patch attached.

-- 
Simon Arlott

[-- Attachment #2: 0001-ppp_generic-pull-2-bytes-so-that-PPP_PROTO-skb-is-va.patch --]
[-- Type: application/octet-stream, Size: 2627 bytes --]

From f6d225971143db1ff5353008d20579e1de75f00d Mon Sep 17 00:00:00 2001
From: Simon Arlott <simon@fire.lp0.eu>
Date: Fri, 30 Apr 2010 19:04:33 +0100
Subject: [PATCH 1/2] ppp_generic: pull 2 bytes so that PPP_PROTO(skb) is valid

In ppp_input(), PPP_PROTO(skb) may refer to invalid data in the skb.

If this happens and (proto >= 0xc000 || proto == PPP_CCPFRAG) then
the packet is passed directly to pppd.

This occurs frequently when using PPPoE with an interface MTU
greater than 1500 because the skb is more likely to be non-linear.

The next 2 bytes need to be pulled in ppp_input(). The pull of 2
bytes in ppp_receive_frame() has been removed as it is no longer
required.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/net/ppp_generic.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 6e281bc..75e8903 100644
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -1567,13 +1567,22 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 	struct channel *pch = chan->ppp;
 	int proto;
 
-	if (!pch || skb->len == 0) {
+	if (!pch) {
 		kfree_skb(skb);
 		return;
 	}
 
-	proto = PPP_PROTO(skb);
 	read_lock_bh(&pch->upl);
+	if (!pskb_may_pull(skb, 2)) {
+		kfree_skb(skb);
+		if (pch->ppp) {
+			++pch->ppp->dev->stats.rx_length_errors;
+			ppp_receive_error(pch->ppp);
+		}
+		goto done;
+	}
+
+	proto = PPP_PROTO(skb);
 	if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {
 		/* put it on the channel queue */
 		skb_queue_tail(&pch->file.rq, skb);
@@ -1585,6 +1594,8 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 	} else {
 		ppp_do_recv(pch->ppp, skb, pch);
 	}
+
+done:
 	read_unlock_bh(&pch->upl);
 }
 
@@ -1617,7 +1628,8 @@ ppp_input_error(struct ppp_channel *chan, int code)
 static void
 ppp_receive_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 {
-	if (pskb_may_pull(skb, 2)) {
+	/* note: a 0-length skb is used as an error indication */
+	if (skb->len > 0) {
 #ifdef CONFIG_PPP_MULTILINK
 		/* XXX do channel-level decompression here */
 		if (PPP_PROTO(skb) == PPP_MP)
@@ -1625,15 +1637,10 @@ ppp_receive_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 		else
 #endif /* CONFIG_PPP_MULTILINK */
 			ppp_receive_nonmp_frame(ppp, skb);
-		return;
+	} else {
+		kfree_skb(skb);
+		ppp_receive_error(ppp);
 	}
-
-	if (skb->len > 0)
-		/* note: a 0-length skb is used as an error indication */
-		++ppp->dev->stats.rx_length_errors;
-
-	kfree_skb(skb);
-	ppp_receive_error(ppp);
 }
 
 static void
-- 
1.7.0.4

Re: [PATCH 1/2] ppp_generic: pull 2 bytes so that PPP_PROTO(skb) is valid

[David Miller]

From: "Simon Arlott" <simon@fire.lp0.eu>
Date: Mon, 3 May 2010 12:50:04 +0100

> Updated patch attached.

Two things:

1) Please don't post patches as binary attachments, it doesn't get
   queued up properly into patchwork if you post it that way.

2) Always make a fresh email posting for new versions of your patch
   when possible, as this way what to include in the commit log
   message is clear and always applies to the patch posted rather
   than bits and pieces of quoted material from previous postings
   that may or may not apply to the current version of the patch.

So please repost this, and your new 2/2 patch, as clean new submissions.

Thanks.