[PATCH net-next v2] pptp: validate available payload before parsing PPP fields

[PATCH net-next v2] pptp: validate available payload before parsing PPP fields

[Zhixing Chen]

pptp_rcv_core() looks into the PPP payload for two special cases:
allowing old-sequence LCP Echo packets and stripping PPP address/control
fields.

Both checks read fixed PPP fields directly from skb data. Make sure the
bytes being read are present in the skb and pulled into the linear area
before dereferencing them.

Use the available skb payload length for these local checks, matching the
existing receive path which passes the remaining skb data to ppp_input()
after pulling the PPTP GRE header.

Signed-off-by: Zhixing Chen <running910@gmail.com>
---

Changes in v2:
- Use the available skb payload length instead of the GRE-advertised
  payload_len for the local PPP field reads.
- Pull the required bytes into the skb linear area before dereferencing
  them.

v1: https://lore.kernel.org/netdev/20260813082247.31499-1-running910@gmail.com/T/

---
 drivers/net/ppp/pptp.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
index a797a0606f6b..4fb94a455c57 100644
--- a/drivers/net/ppp/pptp.c
+++ b/drivers/net/ppp/pptp.c
@@ -275,6 +275,7 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
 	struct pppox_sock *po = pppox_sk(sk);
 	struct pptp_opt *opt = &po->proto.pptp;
 	int headersize, payload_len, seq;
+	unsigned int payload_avail;
 	__u8 *payload;
 	struct pptp_gre_header *header;
 
@@ -314,23 +315,34 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
 	if (!pskb_may_pull(skb, headersize + payload_len))
 		goto drop;
 
-	payload = skb->data + headersize;
+	payload_avail = skb->len - headersize;
 	/* check for expected sequence number */
 	if (seq < opt->seq_recv + 1 || WRAPPED(opt->seq_recv, seq)) {
-		if ((payload[0] == PPP_ALLSTATIONS) && (payload[1] == PPP_UI) &&
-				(PPP_PROTOCOL(payload) == PPP_LCP) &&
-				((payload[4] == PPP_LCP_ECHOREQ) || (payload[4] == PPP_LCP_ECHOREP)))
-			goto allow_packet;
+		if (payload_avail >= PPP_HDRLEN + 1) {
+			if (!pskb_may_pull(skb, headersize + PPP_HDRLEN + 1))
+				goto drop;
+
+			payload = skb->data + headersize;
+			if (payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI &&
+			    PPP_PROTOCOL(payload) == PPP_LCP &&
+			    (payload[4] == PPP_LCP_ECHOREQ || payload[4] == PPP_LCP_ECHOREP))
+				goto allow_packet;
+		}
 	} else {
 		opt->seq_recv = seq;
 allow_packet:
 		skb_pull(skb, headersize);
 
-		if (payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI) {
-			/* chop off address/control */
-			if (skb->len < 3)
+		if (payload_avail >= 2) {
+			if (!pskb_may_pull(skb, 2))
 				goto drop;
-			skb_pull(skb, 2);
+
+			if (skb->data[0] == PPP_ALLSTATIONS && skb->data[1] == PPP_UI) {
+				/* chop off address/control */
+				if (skb->len < 3)
+					goto drop;
+				skb_pull(skb, 2);
+			}
 		}
 
 		skb->ip_summed = CHECKSUM_NONE;

base-commit: 25c1f6111034aef7fc06cfbdcf1e4f0d6e5ee74b
-- 
2.34.1

Re: [PATCH net-next v2] pptp: validate available payload before parsing PPP fields

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

- [Medium] Missing impact statement and missing `Fixes:` tag / target-
  tree annotation for what is a remotely triggerable read past the…

Pre-existing issues:
- [Low] Reported concern (assessed as NOT a defect): the new
  `payload_avail = skb->len - headersize` in pptp_rcv_core() bounds the…

--

pptp: validate available payload before parsing PPP fields

This bounds the two PPP field reads in pptp_rcv_core() by the number of
payload bytes actually present in the skb, and pulls those bytes into the
linear area before dereferencing them. The address/control stripping check
now reads skb->data after the header pull instead of the stale payload
pointer.

v1: https://lore.kernel.org/netdev/20260813082247.31499-1-running910@gmail.com/T/

> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
> index a797a0606f6b4..4fb94a455c574 100644
> --- a/drivers/net/ppp/pptp.c
> +++ b/drivers/net/ppp/pptp.c

[ ... ]

> @@ -314,23 +315,34 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
>  	if (!pskb_may_pull(skb, headersize + payload_len))
>  		goto drop;
>  
> -	payload = skb->data + headersize;
> +	payload_avail = skb->len - headersize;

[Severity: Medium]
Could the changelog say what actually goes wrong without this change, and
carry a Fixes: tag?

Before this patch pptp_rcv_core() only validated headersize + payload_len
bytes, where payload_len comes straight off the wire:

	payload_len = ntohs(header->payload_len);
	...
	if (!pskb_may_pull(skb, headersize + payload_len))
		goto drop;

	payload = skb->data + headersize;

and then dereferenced payload[0], payload[1], PPP_PROTOCOL(payload)
(bytes 2 and 3) and payload[4]. With a GRE-SEQ packet whose payload_len is
0..4, aren't those reads outside the validated range, i.e. skb tailroom or,
for a non-linear skb, bytes that are not payload?

The path is remote for a connected socket:

pptp_rcv() -> sk_receive_skb() -> pptp_rcv_core()

and pptp_rcv() only guarantees the 12-byte GRE header is linear.

Those bytes also steered control flow: if they happened to read as
FF 03 C0 21 09/0A, the out-of-sequence rejection

	if (seq < opt->seq_recv + 1 || WRAPPED(opt->seq_recv, seq))

was skipped via goto allow_packet, and they also decided whether two bytes
were chopped off the frame handed to ppp_input().

git blame points both reads at the original driver commit:

00959ade36ac drivers/net/pptp.c (Dmitry Kozlov 2010-08-21 317) payload = skb->data + headersize;
00959ade36ac drivers/net/pptp.c (Dmitry Kozlov 2010-08-21 320) if ((payload[0] == PPP_ALLSTATIONS) && (payload[1] == PPP_UI) &&

so would this be appropriate?

	Fixes: 00959ade36ac ("PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)")

A target tree in the subject prefix (net or net-next) would help too, since
without a Fixes: tag and an impact description this is unlikely to be picked
up for stable.

[Severity: Low]
This isn't a bug introduced by this patch, but a note on payload_avail
versus payload_len for anyone reading it later.

payload_avail is derived from skb->len rather than from the GRE-advertised
payload_len that pptp_xmit() writes:

	hdr->payload_len = htons(len);

so the LCP Echo check can look at bytes beyond the advertised payload. That
asymmetry is pre-existing and unchanged here: the receive path never trims
the skb to headersize + payload_len, and after

		skb_pull(skb, headersize);

the whole remainder is passed to ppp_input(), both before and after this
patch. Since pskb_may_pull(skb, headersize + payload_len) already
guarantees payload_avail >= payload_len, the new guards never widen the
reads past the skb, and a peer could simply advertise payload_len = 5 to
reach the same LCP Echo exemption. So this looks like a wording preference
only, matching what the changelog already states.

>  	/* check for expected sequence number */
>  	if (seq < opt->seq_recv + 1 || WRAPPED(opt->seq_recv, seq)) {
> -		if ((payload[0] == PPP_ALLSTATIONS) && (payload[1] == PPP_UI) &&
> -				(PPP_PROTOCOL(payload) == PPP_LCP) &&
> -				((payload[4] == PPP_LCP_ECHOREQ) || (payload[4] == PPP_LCP_ECHOREP)))
> -			goto allow_packet;
> +		if (payload_avail >= PPP_HDRLEN + 1) {
> +			if (!pskb_may_pull(skb, headersize + PPP_HDRLEN + 1))
> +				goto drop;
> +
> +			payload = skb->data + headersize;
> +			if (payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI &&
> +			    PPP_PROTOCOL(payload) == PPP_LCP &&
> +			    (payload[4] == PPP_LCP_ECHOREQ || payload[4] == PPP_LCP_ECHOREP))
> +				goto allow_packet;
> +		}
>  	} else {
>  		opt->seq_recv = seq;
>  allow_packet:
>  		skb_pull(skb, headersize);
>  
> -		if (payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI) {
> -			/* chop off address/control */
> -			if (skb->len < 3)
> +		if (payload_avail >= 2) {
> +			if (!pskb_may_pull(skb, 2))
>  				goto drop;
> -			skb_pull(skb, 2);
> +
> +			if (skb->data[0] == PPP_ALLSTATIONS && skb->data[1] == PPP_UI) {
> +				/* chop off address/control */
> +				if (skb->len < 3)
> +					goto drop;
> +				skb_pull(skb, 2);
> +			}
>  		}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902042827.87534-1-running910%40gmail.com