* Re: [PATCH net-next] pptp: validate payload length before parsing PPP fields
2026-08-13 8:22 [PATCH net-next] pptp: validate payload length before parsing PPP fields Zhixing Chen
@ 2026-08-14 17:42 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2026-08-14 17:42 UTC (permalink / raw)
To: Zhixing Chen
Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev
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?
^ permalink raw reply [flat|nested] 2+ messages in thread