* Re: [PATCH net-next v2] pptp: validate available payload before parsing PPP fields
2026-09-02 4:28 [PATCH net-next v2] pptp: validate available payload before parsing PPP fields Zhixing Chen
@ 2026-09-04 10:28 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-04 10:28 UTC (permalink / raw)
To: running910
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, Chuyf26,
netdev
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
^ permalink raw reply [flat|nested] 2+ messages in thread