[PATCH] net: ppp: reject claimed-as-LCP but actually malformed packets

[PATCH] net: ppp: reject claimed-as-LCP but actually malformed packets

[Dmitry Antipov]

Since 'ppp_async_encode()' assumes valid LCP packets (with code
from 1 to 7 inclusive), add 'ppp_check_packet()' to ensure that
LCP packet has an actual body beyond PPP_LCP header bytes, and
reject claimed-as-LCP but actually malformed data otherwise.

Reported-by: syzbot+ec0723ba9605678b14bf@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ec0723ba9605678b14bf
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/ppp/ppp_generic.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 0a65b6d690fe..2c8dfeb8ca58 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -493,6 +493,18 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
 	return ret;
 }
 
+static bool ppp_check_packet(struct sk_buff *skb, size_t count)
+{
+	if (get_unaligned_be16(skb->data) == PPP_LCP &&
+	    count < PPP_PROTO_LEN + 4)
+		/* Claimed as LCP but has no actual LCP body,
+		 * which is 4 bytes at least (code, identifier,
+		 * and 2-byte length).
+		 */
+		return false;
+	return true;
+}
+
 static ssize_t ppp_write(struct file *file, const char __user *buf,
 			 size_t count, loff_t *ppos)
 {
@@ -515,6 +527,11 @@ static ssize_t ppp_write(struct file *file, const char __user *buf,
 		kfree_skb(skb);
 		goto out;
 	}
+	ret = -EINVAL;
+	if (unlikely(!ppp_check_packet(skb, count))) {
+		kfree_skb(skb);
+		goto out;
+	}
 
 	switch (pf->kind) {
 	case INTERFACE:
-- 
2.45.2

Re: [PATCH] net: ppp: reject claimed-as-LCP but actually malformed packets

[Simon Horman]

+ Ricardo, Eric, Jakub, and Paolo
  Please derive CC list from get_maintainers.pl my.patch

On Fri, Jul 05, 2024 at 07:08:08PM +0300, Dmitry Antipov wrote:
> Since 'ppp_async_encode()' assumes valid LCP packets (with code
> from 1 to 7 inclusive), add 'ppp_check_packet()' to ensure that
> LCP packet has an actual body beyond PPP_LCP header bytes, and
> reject claimed-as-LCP but actually malformed data otherwise.
> 
> Reported-by: syzbot+ec0723ba9605678b14bf@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ec0723ba9605678b14bf

Hi Dmitry,

As a fix, a Fixes tag should go here (no blank line between any tags).
And the patch should be targeted at the net tree:

	Subject: [PATCH net] ...

> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/ppp/ppp_generic.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 0a65b6d690fe..2c8dfeb8ca58 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -493,6 +493,18 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
>  	return ret;
>  }
>  
> +static bool ppp_check_packet(struct sk_buff *skb, size_t count)
> +{
> +	if (get_unaligned_be16(skb->data) == PPP_LCP &&
> +	    count < PPP_PROTO_LEN + 4)
> +		/* Claimed as LCP but has no actual LCP body,
> +		 * which is 4 bytes at least (code, identifier,
> +		 * and 2-byte length).
> +		 */
> +		return false;
> +	return true;
> +}

I agree that this fix is correct, that it addresses the issue at hand,
and that ppp_write() is a good place for this check for invalid input.
But I have some minor feedback on the implementation above.

1. It might be nicer to add define, say near where PPP_PROTO_LEN is
   defined, instead of using 4.

   E.g. #define PPP_LCP_HDR_LEN 4

2. I would express the boolean logic without an if condition:
   (Completely untested!)

static bool ppp_check_packet(struct sk_buff *skb, size_t count)
{
	/* LCP packets must include LCP header which 4 bytes long:
	 * 1-byte code, 1-byte identifier, and 2-byte length.
	 */
	return get_unaligned_be16(skb->data) != PPP_LCP ||
		count >= PPP_PROTO_LEN + PPP_LCP_HDR_LEN;
}

> +
>  static ssize_t ppp_write(struct file *file, const char __user *buf,
>  			 size_t count, loff_t *ppos)
>  {
> @@ -515,6 +527,11 @@ static ssize_t ppp_write(struct file *file, const char __user *buf,
>  		kfree_skb(skb);
>  		goto out;
>  	}
> +	ret = -EINVAL;
> +	if (unlikely(!ppp_check_packet(skb, count))) {
> +		kfree_skb(skb);
> +		goto out;
> +	}

FWIIW, I agree the above is in keeping with the existing flow of this function.

>  
>  	switch (pf->kind) {
>  	case INTERFACE:
> -- 
> 2.45.2
> 
> 

[PATCH net v2] ppp: reject claimed-as-LCP but actually malformed packets

[Dmitry Antipov]

Since 'ppp_async_encode()' assumes valid LCP packets (with code
from 1 to 7 inclusive), add 'ppp_check_packet()' to ensure that
LCP packet has an actual body beyond PPP_LCP header bytes, and
reject claimed-as-LCP but actually malformed data otherwise.

Reported-by: syzbot+ec0723ba9605678b14bf@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ec0723ba9605678b14bf
Fixes: 44073187990d ("ppp: ensure minimum packet size in ppp_write()")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: style, comments, and metadata adjustments suggested by Simon Horman
---
 drivers/net/ppp/ppp_generic.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 0a65b6d690fe..eb9acfcaeb09 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -70,6 +70,7 @@
 #define MPHDRLEN_SSN	4	/* ditto with short sequence numbers */
 
 #define PPP_PROTO_LEN	2
+#define PPP_LCP_HDRLEN	4
 
 /*
  * An instance of /dev/ppp can be associated with either a ppp
@@ -493,6 +494,15 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
 	return ret;
 }
 
+static bool ppp_check_packet(struct sk_buff *skb, size_t count)
+{
+	/* LCP packets must include LCP header which 4 bytes long:
+	 * 1-byte code, 1-byte identifier, and 2-byte length.
+	 */
+	return get_unaligned_be16(skb->data) != PPP_LCP ||
+		count >= PPP_PROTO_LEN + PPP_LCP_HDRLEN;
+}
+
 static ssize_t ppp_write(struct file *file, const char __user *buf,
 			 size_t count, loff_t *ppos)
 {
@@ -515,6 +525,11 @@ static ssize_t ppp_write(struct file *file, const char __user *buf,
 		kfree_skb(skb);
 		goto out;
 	}
+	ret = -EINVAL;
+	if (unlikely(!ppp_check_packet(skb, count))) {
+		kfree_skb(skb);
+		goto out;
+	}
 
 	switch (pf->kind) {
 	case INTERFACE:
-- 
2.45.2

Re: [PATCH net v2] ppp: reject claimed-as-LCP but actually malformed packets

[Simon Horman]

+ Guillaume, Jakub

On Mon, Jul 08, 2024 at 02:56:15PM +0300, Dmitry Antipov wrote:
> Since 'ppp_async_encode()' assumes valid LCP packets (with code
> from 1 to 7 inclusive), add 'ppp_check_packet()' to ensure that
> LCP packet has an actual body beyond PPP_LCP header bytes, and
> reject claimed-as-LCP but actually malformed data otherwise.
> 
> Reported-by: syzbot+ec0723ba9605678b14bf@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ec0723ba9605678b14bf
> Fixes: 44073187990d ("ppp: ensure minimum packet size in ppp_write()")

Sorry for not noticing this earlier.

I think that the cited commit is not where this problem was introduced.
What that commit does is to introduce a length check.
And what this patch does is to add another, more specific length check.
But the problem fixed by this patch existed before the cited commit.

I expect that, like the cited commit, this patch:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v2: style, comments, and metadata adjustments suggested by Simon Horman

Thanks, other than the Fixes tag, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH net v2] ppp: reject claimed-as-LCP but actually malformed packets

[Paolo Abeni]

On Tue, 2024-07-09 at 09:30 +0100, Simon Horman wrote:
> + Guillaume, Jakub
> 
> On Mon, Jul 08, 2024 at 02:56:15PM +0300, Dmitry Antipov wrote:
> > Since 'ppp_async_encode()' assumes valid LCP packets (with code
> > from 1 to 7 inclusive), add 'ppp_check_packet()' to ensure that
> > LCP packet has an actual body beyond PPP_LCP header bytes, and
> > reject claimed-as-LCP but actually malformed data otherwise.
> > 
> > Reported-by: syzbot+ec0723ba9605678b14bf@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=ec0723ba9605678b14bf
> > Fixes: 44073187990d ("ppp: ensure minimum packet size in ppp_write()")
> 
> Sorry for not noticing this earlier.
> 
> I think that the cited commit is not where this problem was introduced.
> What that commit does is to introduce a length check.
> And what this patch does is to add another, more specific length check.
> But the problem fixed by this patch existed before the cited commit.
> 
> I expect that, like the cited commit, this patch:
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

No need to resend, I'll update the tag while applying the patch.

Thanks!

Paolo

Re: [PATCH net v2] ppp: reject claimed-as-LCP but actually malformed packets

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon,  8 Jul 2024 14:56:15 +0300 you wrote:
> Since 'ppp_async_encode()' assumes valid LCP packets (with code
> from 1 to 7 inclusive), add 'ppp_check_packet()' to ensure that
> LCP packet has an actual body beyond PPP_LCP header bytes, and
> reject claimed-as-LCP but actually malformed data otherwise.
> 
> Reported-by: syzbot+ec0723ba9605678b14bf@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ec0723ba9605678b14bf
> Fixes: 44073187990d ("ppp: ensure minimum packet size in ppp_write()")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> 
> [...]

Here is the summary with links:
  - [net,v2] ppp: reject claimed-as-LCP but actually malformed packets
    https://git.kernel.org/netdev/net/c/f2aeb7306a89

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html