* Re: [PATCH] pptp: reject payloads shorter than the PPP protocol field
2026-08-18 16:21 [PATCH] pptp: reject payloads shorter than the PPP protocol field Chuyf26
@ 2026-08-21 9:07 ` Simon Horman
2026-08-21 9:42 ` Chuyf26
2026-08-21 9:26 ` Simon Horman
1 sibling, 1 reply; 5+ messages in thread
From: Simon Horman @ 2026-08-21 9:07 UTC (permalink / raw)
To: Chuyf26; +Cc: Dmitry Kozlov, netdev, Zhixing Chen
On Tue, Aug 18, 2026 at 04:21:43PM +0000, Chuyf26 wrote:
> pptp_rcv_core() dereferences the first payload bytes after pulling the
> GRE header: payload[0] and payload[1] when stripping the address/control
> field, and up to payload[4] when checking for an LCP echo request in
> the out-of-order case. Only headersize + payload_len bytes of the skb
> are pulled, so a PPTP packet with a payload_len smaller than these
> accesses makes the driver read past the end of the packet data.
>
> The path is: a GRE packet for an established PPTP channel arrives
> through the IP protocol 47 handler pptp_rcv(), which validates the GRE
> flags, looks the channel up by call id and source address and queues
> the skb to the pppox socket, where pptp_rcv_core() runs for sockets in
> PPPOX_CONNECTED state. payload_len is taken from the GRE header without
> any lower bound, so an attacker who can inject packets carrying the
> channel's call id and peer address can make pskb_may_pull() pull as
> little as the GRE header itself; payload then points at the end of the
> pulled data and the dereferences above read up to five bytes past the
> packet. The sequence number is likewise attacker controlled, so both
> the out-of-order LCP echo check and the in-order address/control
> stripping are reachable.
>
> The read lands inside the skb data allocation (tailroom of the same
> slab object), so KASAN does not report it and the access does not
> fault, but the bytes read are undefined and steer the accept/drop
> decision: garbage may let a short malformed frame through to
> ppp_input(), and the LCP echo check is meaningless for payloads
> shorter than a PPP protocol field.
>
> A PPTP payload is a PPP frame and therefore always carries at least
> the two-byte PPP protocol field, and the LCP echo check needs five
> bytes to be meaningful. Drop packets shorter than two bytes and only
> perform the LCP echo check when at least five bytes are present.
> Valid packets are unaffected.
>
> Fixes: 00959ade36ac ("PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)")
> Reported-by: Abaci <abaci@linux.alibaba.com>
> Assisted-by: abaci:qwen3.8-max
> Signed-off-by: Chuyf26 <Chuyf26@linux.alibaba.com>
> ---
> drivers/net/ppp/pptp.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
> index 3a10303eb756..ce1cd72baadf 100644
> --- a/drivers/net/ppp/pptp.c
> +++ b/drivers/net/ppp/pptp.c
> @@ -317,10 +317,18 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
> if (!pskb_may_pull(skb, headersize + payload_len))
> goto drop;
>
> + /* The payload is a PPP frame, so it always carries at least the
> + * two-byte PPP protocol field. The payload bytes are dereferenced
> + * below, reject packets too short to hold them.
> + */
> + if (payload_len < 2)
> + goto drop;
> +
> 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) &&
> + 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;
Hi,
Thanks for your patch.
Some feedback from my side.
* The CC list for patch is incomplete.
An accurate CC list can be generated with assistance from:
get_maintainers.pl this.patch
* Please target Networking patches at either net, for fixes (this case),
or net-next, for other patches.
Subject: [PATCH net] ...
* This patch seems similar to:
- [PATCH net-next] pptp: validate payload length before parsing PPP fields
https://lore.kernel.org/netdev/20260813082247.31499-1-running910@gmail.com/
And the same concern I raised in relation to that patch seems present here.
I'll copy that concern here for your consideration (although in this
case there is only one new guard).
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] 5+ messages in thread* Re: [PATCH] pptp: reject payloads shorter than the PPP protocol field
2026-08-18 16:21 [PATCH] pptp: reject payloads shorter than the PPP protocol field Chuyf26
2026-08-21 9:07 ` Simon Horman
@ 2026-08-21 9:26 ` Simon Horman
2026-08-21 9:42 ` Chuyf26
1 sibling, 1 reply; 5+ messages in thread
From: Simon Horman @ 2026-08-21 9:26 UTC (permalink / raw)
To: Chuyf26; +Cc: Dmitry Kozlov, netdev
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818162229.yOFI43Vfo0v911uVw0AFpyHQ_aOe_fsjoQXPZP70vMI@z?part=1
Simon says:
Given the first issue raised by the AI-generated review below, it seems to
me that this is more of a hardening patch - for net-next, no Fixes tag -
than a bug fix - for net, with a Fixes tag.
---
--- Patch [1]: [PATCH] x25: validate class D facility length before reading it ---
commit 37de2e4212c8268079bc40f17b99b1685565a4da
Author: Chuyf26 <Chuyf26@linux.alibaba.com>
x25: validate class D facility length before reading it
The patch adds a len < 2 test to the X25_FAC_CLASS_D case of
x25_parse_facilities() so that the two facility header bytes are known to
be present before p[1] is loaded. Classes A, B and C already validate
their fixed lengths in the same way.
No Link: tags are present in the commit header.
> x25: validate class D facility length before reading it
>
> The over-read lands one byte past the packet data but inside the skb
> data allocation, where KASAN does not report it, and the value read
> then controls how many further bytes the parser believes remain.
[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
Does the value read actually reach the parser's length arithmetic? In the
only case where p[1] is outside the validated range, len == 1, the
pre-existing check already rejected the frame:
net/x25/x25_facilities.c:x25_parse_facilities() {
unsigned int len;
...
len = skb->data[0];
if (!pskb_may_pull(skb, 1 + len))
return -1;
p = skb->data + 1;
...
case X25_FAC_CLASS_D:
if (len < p[1] + 2)
return -1;
}
len is unsigned int, p[1] is unsigned char so p[1] + 2 is an int in
[2, 257], and the comparison is done after conversion to unsigned, so
1 < p[1] + 2 holds for every possible byte value.
That means the len -= p[1] + 2; / p += p[1] + 2; statements and the
CALLING_AE/CALLED_AE memcpy() paths were unreachable for len == 1 both
before and after this change, and the new len < 2 clause returns the same
-1 for the same input.
Would it be more accurate to describe this as removing a benign one-byte
read rather than as a parser length bug, given the commit message itself
notes the read stays inside the skb head allocation? Does the
Fixes: 1da177e4c3f4 tag still make sense if there is no behavioural
change for stable trees to pick up?
> diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
> index 6dadb217e101..bfb269538984 100644
> --- a/net/x25/x25_facilities.c
> +++ b/net/x25/x25_facilities.c
> @@ -147,7 +147,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
> len -= 4;
> break;
> case X25_FAC_CLASS_D:
> - if (len < p[1] + 2)
> + if (len < 2 || len < p[1] + 2)
> return -1;
> switch (*p) {
> case X25_FAC_CALLING_AE:
[Severity: Medium]
[Finding: main-2-1]
[Sources: opus-5]
This isn't a bug introduced by this patch, but while the class D branch is
being touched: should the facility length byte and the declared semi-octet
count also be checked against each other?
The CALLING_AE and CALLED_AE cases in x25_parse_facilities() validate the
two independently:
case X25_FAC_CALLED_AE:
if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
return -1;
if (p[2] > X25_MAX_AE_LEN)
return -1;
dte_facs->called_len = p[2];
memcpy(dte_facs->called_ae, &p[3], p[1] - 1);
There is no test that p[1] - 1 >= (p[2] + 1) / 2, while the emitter
x25_create_facilities() ties them together:
if (dte_facs->calling_len && (facil_mask & X25_MASK_CALLING_AE)) {
unsigned int bytecount = (dte_facs->calling_len + 1) >> 1;
*p++ = X25_FAC_CALLING_AE;
*p++ = 1 + bytecount;
*p++ = dte_facs->calling_len;
memcpy(p, dte_facs->calling_ae, bytecount);
So a peer sending p[1] = 2 with p[2] = 40 makes the kernel store
called_len = 40 while copying only one byte into called_ae.
Two follow-on effects: userspace reading SIOCX25GDTEFACILITIES in
af_x25.c gets a semi-octet count for 39 semi-octets the peer never sent
(the buffers are memset at the top of x25_parse_facilities(), so this is
fabricated zero data rather than a memory disclosure), and on the accept
path x25_rx_call_request() clears only the calling side:
net/x25/af_x25.c:x25_rx_call_request() {
/* ensure no calling address extension on accept */
makex25->vc_facil_mask &= ~X25_MASK_CALLING_AE;
}
so the peer-supplied called_len survives into makex25->dte_facilities and
is fed back to x25_create_facilities() when x25_write_internal() builds
X25_CALL_ACCEPTED, emitting up to 20 mostly-zero AE bytes.
There is no memory-safety consequence here: the memcpy() length is bounded
by X25_MAX_DTE_FACIL_LEN - 1 = 20, which matches sizeof(called_ae), and
the worst-case emitted facilities block stays inside X25_MAX_FAC_LEN.
Would it make sense to reject encodings the emitter can never produce, in
the same spirit as the header-length check being added here?
--
pw-bot: changes-requested
^ permalink raw reply [flat|nested] 5+ messages in thread