netdev - Re: [PATCH net] pptp: ensure minimal skb length in pptp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3c785bf4-b4b3-41ee-b0c3-5c3bfb4555f1@linux.intel.com>
Date: Tue, 29 Jul 2025 14:31:35 +0200
From: Dawid Osuchowski <dawid.osuchowski@...ux.intel.com>
To: Eric Dumazet <edumazet@...gle.com>, "David S . Miller"
 <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>
Cc: Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
 eric.dumazet@...il.com, syzbot+afad90ffc8645324afe5@...kaller.appspotmail.com
Subject: Re: [PATCH net] pptp: ensure minimal skb length in pptp_xmit()

On 2025-07-29 10:02 AM, Eric Dumazet wrote:
> Commit aabc6596ffb3 ("net: ppp: Add bound checking for skb data
> on ppp_sync_txmung") fixed ppp_sync_txmunge()
> 
> We need a similar fix in pptp_xmit(), otherwise we might
> read uninit data as reported by syzbot.
> 
> BUG: KMSAN: uninit-value in pptp_xmit+0xc34/0x2720 drivers/net/ppp/pptp.c:193
>    pptp_xmit+0xc34/0x2720 drivers/net/ppp/pptp.c:193
>    ppp_channel_bridge_input drivers/net/ppp/ppp_generic.c:2290 [inline]
>    ppp_input+0x1d6/0xe60 drivers/net/ppp/ppp_generic.c:2314
>    pppoe_rcv_core+0x1e8/0x760 drivers/net/ppp/pppoe.c:379
>    sk_backlog_rcv+0x142/0x420 include/net/sock.h:1148
>    __release_sock+0x1d3/0x330 net/core/sock.c:3213
>    release_sock+0x6b/0x270 net/core/sock.c:3767
>    pppoe_sendmsg+0x15d/0xcb0 drivers/net/ppp/pppoe.c:904
>    sock_sendmsg_nosec net/socket.c:712 [inline]
>    __sock_sendmsg+0x330/0x3d0 net/socket.c:727
>    ____sys_sendmsg+0x893/0xd80 net/socket.c:2566
>    ___sys_sendmsg+0x271/0x3b0 net/socket.c:2620
>    __sys_sendmmsg+0x2d9/0x7c0 net/socket.c:2709
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+afad90ffc8645324afe5@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/68887d86.a00a0220.b12ec.00cd.GAE@google.com/T/#u
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>   drivers/net/ppp/pptp.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
> index 5feaa70b5f47e6cd33fbaff33f6715f42c4d71b5..4cd6f67bd5d3520308ee4f8d68547a1bc8a7bfd3 100644
> --- a/drivers/net/ppp/pptp.c
> +++ b/drivers/net/ppp/pptp.c
> @@ -159,9 +159,7 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>   	int len;
>   	unsigned char *data;
>   	__u32 seq_recv;

nit: the declarations do not follow RCT, but I guess it might be 
additional churn to reorder in this patch

> -
> -
> -	struct rtable *rt;
> +	struct rtable *rt = NULL;
>   	struct net_device *tdev;
>   	struct iphdr  *iph;
>   	int    max_headroom;
> @@ -179,16 +177,20 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>   
>   	if (skb_headroom(skb) < max_headroom || skb_cloned(skb) || skb_shared(skb)) {
>   		struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom);
> -		if (!new_skb) {
> -			ip_rt_put(rt);
> +
> +		if (!new_skb)
>   			goto tx_error;
> -		}
> +
>   		if (skb->sk)
>   			skb_set_owner_w(new_skb, skb->sk);
>   		consume_skb(skb);
>   		skb = new_skb;
>   	}
>   
> +	/* Ensure we can safely access protocol field and LCP code */
> +	if (!pskb_may_pull(skb, 3))
> +		goto tx_error;
> +
>   	data = skb->data;
>   	islcp = ((data[0] << 8) + data[1]) == PPP_LCP && 1 <= data[2] && data[2] <= 7;
>   
> @@ -262,6 +264,7 @@ static int pptp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>   	return 1;
>   
>   tx_error:
> +	ip_rt_put(rt);
>   	kfree_skb(skb);
>   	return 1;
>   }

Reviewed-by: Dawid Osuchowski <dawid.osuchowski@...ux.intel.com>

Thanks,
Dawid