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

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

[Zhixing Chen]

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;

base-commit: f6057f06ef7afa9893ed33603f7917fa39d237b5
-- 
2.34.1