Re: [PATCH] ppp: pptp: fix out-of-bounds read on short payload_len in pptp_rcv_core()

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] New unconditional `if (payload_len < 2) goto drop;` in
  pptp_rcv_core() (drivers/net/ppp/pptp.c:313) narrows the set of PPP…

--

ppp: pptp: fix out-of-bounds read on short payload_len in pptp_rcv_core()

The patch adds a minimum length requirement on the GRE payload_len field in
pptp_rcv_core() and gates the LCP Echo detection on payload_len >= 5, so the
PPP header bytes inspected after pskb_may_pull() are known to be present.

> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
> index a797a0606f6b..2f979294490d 100644
> --- a/drivers/net/ppp/pptp.c
> +++ b/drivers/net/ppp/pptp.c
> @@ -310,6 +310,8 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
>  	payload_len = ntohs(header->payload_len);
>  	seq         = ntohl(header->seq);
>  
> +	if (payload_len < 2)
> +		goto drop;

[Severity: Medium]
Does this unconditional drop narrow the set of frames the PPTP channel will
deliver to ppp_input()?

A one-octet payload holding a Protocol-Field-Compressed protocol value is a
valid channel frame for the PPP core, which only requires a single byte:

drivers/net/ppp/ppp_generic.c:ppp_decompress_proto() {
	/* At least one byte should be present (if protocol is compressed) */
	if (!pskb_may_pull(skb, 1))
		return false;

	__ppp_decompress_proto(skb);

	/* Protocol field should occupy 2 bytes when not compressed */
	return pskb_may_pull(skb, 2);
}

The transmit side of this same driver produces that format in pptp_xmit():

	/* compress protocol field */
	if ((opt->ppp_flags & SC_COMP_PROT) && data[0] == 0 && !islcp)
		skb_pull(skb, 1);

	/* Put in the address/control bytes if necessary */
	if ((opt->ppp_flags & SC_COMP_AC) == 0 || islcp) {

So with payload_len == 1 a protocol-only frame used to reach ppp_input() and
is now dropped in pptp_rcv_core().

The only in-window access that needs two bytes is the address/control probe
further down:

		if (payload[0] == PPP_ALLSTATIONS && payload[1] == PPP_UI) {
			/* chop off address/control */
			if (skb->len < 3)
				goto drop;
			skb_pull(skb, 2);
		}

Would gating that test with payload_len >= 2, in the same style already used
for the payload_len >= 5 LCP test below, close the read without rejecting
one-byte frames?

>  	/* check for incomplete packet (length smaller than expected) */
>  	if (!pskb_may_pull(skb, headersize + payload_len))
>  		goto drop;
> @@ -317,9 +319,11 @@ 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 >= 5 &&
> +		    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;

[Severity: Medium]
Related question on the same change: payload_len is a peer-supplied GRE
header field that the rest of pptp_rcv_core() treats as advisory.  It is only
fed to pskb_may_pull(), the skb is never trimmed to headersize + payload_len,
and the later checks use skb->len instead.

Using it as the gate here means a peer that under-reports payload_len has its
out-of-window LCP Echo keepalives dropped rather than recognised, where the
previous code looked at the bytes actually present in the skb.

Would checking skb->len - headersize (the bytes pskb_may_pull() actually
guaranteed) be a better gate than the header field?

[ ... ]

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