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

[Simon Horman <horms@kernel.org>]

On Thu, Aug 13, 2026 at 04:22:47PM +0800, Zhixing Chen wrote:
> pptp_rcv_core() pulls the PPTP GRE header together with the payload length
> advertised by the header, then looks into the PPP payload for two special
> cases: allowing old-sequence LCP Echo packets and stripping the PPP
> address/control fields.
> 
> Both checks read fixed PPP fields from the payload. Make sure the
> advertised payload length covers those fields before reading them, so
> malformed short payloads are rejected before their PPP contents are
> evaluated.
> 
> This keeps the receive path within the declared PPTP payload boundary.
> 
> Signed-off-by: Zhixing Chen <running910@gmail.com>
> ---
> 
> While testing PPTP stability and reviewing the PPTP driver code, I noticed
> that the receive path can reach the old-sequence LCP Echo check and the
> address/control field handling with an advertised payload length shorter
> than the PPP fields being inspected.
> 
> I exercised this path with malformed short PPTP GRE packets and confirmed
> that such packets can reach both checks. The test packets did not trigger a
> KASAN report in my setup, but my understanding is that the parser should
> not inspect bytes outside the declared PPTP payload when deciding how to
> handle PPP fields.
> 
> This is intended as a small robustness improvement for malformed PPTP GRE
> packets.
> 
> ---
>  drivers/net/ppp/pptp.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
> index a797a0606f6b..708d8fb3a900 100644
> --- a/drivers/net/ppp/pptp.c
> +++ b/drivers/net/ppp/pptp.c
> @@ -317,16 +317,18 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
>  	payload = skb->data + 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)))
> +		if (payload_len >= PPP_HDRLEN + 1 &&
> +		    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) {
> +		if (payload_len >= 2 &&
> +		    payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI) {
>  			/* chop off address/control */
>  			if (skb->len < 3)
>  				goto drop;

Hi,

There is an AI-generated review of your patch available at
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260813082247.31499-1-running910%40gmail.com

  It suggests adding a Fixes tag, and implicitly targeting net.
  I feel that is not correct as you are positioning this as a robustness
  improvement.

  However, the following portion of the review does stand out to me
  and I'd value your feedback on it:

  Both new guards bound the reads by the peer-advertised payload_len rather
  than by the bytes actually present (skb->len - headersize), which is what
  pskb_may_pull() guaranteed and what the pre-existing check just below
  already uses.  Is it intentional that one skb_pull(skb, 2) is now gated by
  two different length authorities?

  Since the skb is never trimmed to headersize + payload_len, payload_len is
  only a lower bound, so a peer that under-reports it still passes
  pskb_may_pull() and is still delivered.  For an in-sequence frame whose
  real PPP payload starts with 0xff 0x03 but whose header says
  payload_len < 2, the address/control octets are no longer stripped, and
  the frame continues down:

  drivers/net/ppp/ppp_generic.c:__ppp_decompress_proto() {
	if (ppp_skb_is_compressed_proto(skb))
		*(u8 *)skb_push(skb, 1) = 0x00;
  }

  0xff has the low bit set, so a 0x00 is pushed and ppp_input() then sees:

	proto = PPP_PROTO(skb);
	if (!ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {

  with proto == 0x00ff instead of the real protocol, so the frame never
  reaches its intended handler.  Similarly, out-of-sequence LCP Echo
  keepalives from such a peer are now dropped by the PPP_HDRLEN + 1 guard
  instead of allowed, which can tear a tunnel down.  Would using
  skb->len - headersize for both comparisons avoid this?