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

+ 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: [email protected]
> 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 <[email protected]>
> ---
>  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
> 
>